On 25 June 2018 at 18:07, Alexander Potapenko <glider@xxxxxxxxxx> wrote:
> On Mon, Jun 25, 2018 at 5:37 PM Ard Biesheuvel
> <ard.biesheuvel@xxxxxxxxxx> wrote:
>>
>> On 25 June 2018 at 16:59, Alexander Potapenko <glider@xxxxxxxxxx> wrote:
>> > Hi Ard, Mark, Andrew and others,
>> >
>> > AFAIU, commit 029c54b09599573015a5c18dbe59cbdf42742237 ("mm/vmalloc.c:
>> > huge-vmap: fail gracefully on unexpected huge vmap mappings") was
>> > supposed to make vmalloc_to_page() return NULL for pointers not
>> > returned by vmalloc().
>> > But when I call vmalloc_to_page() for the pointer returned by
>> > acpi_os_ioremap() (see the patch below) I see that the resulting
>> > `struct page *` points to unmapped memory:
>> >
>>
>> Why do you assume it maps memory? It could map a device's MMIO
>> registers as well, which don't have struct pages associated with them.
> I might have been unclear. I'm just assuming that vmalloc_to_page()
> returns either a valid struct page or NULL for a valid pointer
> belonging to vmalloc area.
> In this case vmalloc_to_page() returns a wild pointer.
>
is_vmalloc_addr() only checks whether the mapping is within the
boundaries of the VMALLOC region, and does not check whether it is in
fact a VM_ALLOC or VM_MAP mapping, and vmalloc_to_page() should
probably only be called on mappings of that type. The reason it is
implemented like this may well be the issue that you highlight, i.e.,
that this cannot be done from every context.
As for the patch, it was intended to ensure that vmalloc_to_page()
does not blindly assume that VM_MAP regions are mapped down to pages,
which is not the case for mappings of the kernel image on arm64.
>> > ============================
>> > ACPI: Enabled 2 GPEs in block 00 to 0F
>> > phys: 00000000fed00000, vmalloc: ffffc9000019a000, page:
>>
>> Isn't that phys address something like the HPET on a x86 system?
> Yes, probably. I just came across it while trying to instrument every
> memory access with code doing something along the lines of:
> if (is_vmalloc_addr(addr))
> return vmalloc_to_page(addr)->metadata;
> else
> if (virt_to_page(addr))
> return virt_to_page(addr)->metadata;
>
> , I don't think there's anything specific to that physical address.
>> > ffff8800fed00000 [ffffea0003fb4000]
>> > BUG: unable to handle kernel paging request at ffffea0003fb4000
>> > PGD 3f7d5067 P4D 3f7d5067 PUD 3f7d4067 PMD 0
>> > Oops: 0000 [#1] SMP PTI
>> > CPU: 0 PID: 1 Comm: swapper/0 Not tainted 4.18.0-rc2+ #1325
>> > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/=
>> > 2014
>> > RIP: 0010:acpi_os_map_iomem+0x1c5/0x210 ??:?
>> > Code: 00 88 ff ff 4d 89 f8 48 c1 f9 06 4c 89 f6 48 c7 c7 60 5f 01 82
>> > 48 c1 e1 0c 48 01 c1 e8 6d 42 70 ff 4d 85 ff 0f 84 14 ff ff ff <49> 8b
>> > 37 48 c7 c7 d2 61 01 82 e8 55 42 70 ff e9 00 ff ff ff 48 c7
>> > RSP: 0000:ffff88003e253840 EFLAGS: 00010286
>> > RAX: 000000000000005c RBX: ffff88003d857b80 RCX: ffffffff82245d38
>> > RDX: 0000000000000000 RSI: 0000000000000096 RDI: ffffffff8288e86c
>> > RBP: 00000000fed00000 R08: 00000000000000ae R09: 0000000000000007
>> > R10: 0000000000000000 R11: ffffffff828908ad R12: 0000000000001000
>> > R13: ffffc9000019a000 R14: 00000000fed00000 R15: ffffea0003fb4000
>> > FS: 0000000000000000(0000) GS:ffff88003fc00000(0000) knlGS:000000000000000=
>> > 0
>> > CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> > CR2: ffffea0003fb4000 CR3: 000000000220a000 CR4: 00000000000006f0
>> > Call Trace:
>> > acpi_ex_system_memory_space_handler+0xca/0x19f ??:?
>> > ============================
>> >
>> > For memory error detection purposes I'm trying to map the addresses
>> > from the vmalloc range to valid struct pages, or at least make sure
>> > there's no struct page for a given address.
>> > Looking up the vmap_area_root rbtree isn't an option, as this must be
>> > done from instrumented code, including interrupt handlers.
>> > I've been trying to employ vmalloc_to_page(), but looks like it
>> > doesn't work for ioremapped addresses.
>> > Is this at all possible?
>> >
>> > Patch showing the problem follows. I'm building using GCC 7.1.1 on a
>> > defconfig for x86_64.
>> >
>> > --- a/drivers/acpi/osl.c
>> > +++ b/drivers/acpi/osl.c
>> > @@ -279,14 +279,23 @@ acpi_map_lookup_virt(void __iomem *virt, acpi_size si=
>> > ze)
>> > static void __iomem *acpi_map(acpi_physical_address pg_off, unsigned
>> > long pg_sz)
>> > {
>> > unsigned long pfn;
>> > + void __iomem *ret;
>> > + struct page *page;
>> >
>> > pfn =3D pg_off >> PAGE_SHIFT;
>> > if (should_use_kmap(pfn)) {
>> > if (pg_sz > PAGE_SIZE)
>> > return NULL;
>> > return (void __iomem __force *)kmap(pfn_to_page(pfn));
>> > - } else
>> > - return acpi_os_ioremap(pg_off, pg_sz);
>> > + } else {
>> > + ret =3D acpi_os_ioremap(pg_off, pg_sz);
>> > + BUG_ON(!is_vmalloc_addr(ret));
>> > + page =3D vmalloc_to_page(ret);
>> > + pr_err("phys: %px, vmalloc: %px, page: %px [%px]\n",
>> > pg_off, ret, page_address(page), page);
>> > + if (page)
>> > + pr_err("flags: %d\n", page->flags);
>> > + return ret;
>> > + }
>> > }
>> >
>> > Thanks,
>> > Alexander Potapenko
>> > Software Engineer
>> >
>> > Google Germany GmbH
>> > Erika-Mann-Straße, 33
>> > 80636 München
>> >
>> > Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
>> > Registergericht und -nummer: Hamburg, HRB 86891
>> > Sitz der Gesellschaft: Hamburg
>
>
>
> --
> Alexander Potapenko
> Software Engineer
>
> Google Germany GmbH
> Erika-Mann-Straße, 33
> 80636 München
>
> Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
> Registergericht und -nummer: Hamburg, HRB 86891
> Sitz der Gesellschaft: Hamburg
