| Message ID | 7885dce0-edbe-db04-b5ec-bd271c9a0612@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | struct dev_pagemap corruption | expand |
On 05/04/2019 05:40, Anshuman Khandual wrote: > Hello, > > On arm64 platform "struct dev_pagemap" is getting corrupted during ZONE_DEVICE > unmapping path through device_destroy(). Its device memory range end address > (pgmap->res.end) which is getting corrupted in this particular case. AFAICS > pgmap which gets initialized by the driver and mapped with devm_memremap_pages() > should retain it's values during the unmapping path as well. Is this assumption > right ? > > [ 62.779412] Call trace: > [ 62.779808] dump_backtrace+0x0/0x118 > [ 62.780460] show_stack+0x14/0x20 > [ 62.781204] dump_stack+0xa8/0xcc > [ 62.781941] devm_memremap_pages_release+0x24/0x1d8 > [ 62.783021] devm_action_release+0x10/0x18 > [ 62.783911] release_nodes+0x1b0/0x220 > [ 62.784732] devres_release_all+0x34/0x50 > [ 62.785623] device_release+0x24/0x90 > [ 62.786454] kobject_put+0x74/0xe8 > [ 62.787214] device_destroy+0x48/0x58 > [ 62.788041] zone_device_public_altmap_init+0x404/0x42c [zone_device_public_altmap] > [ 62.789675] do_one_initcall+0x74/0x190 > [ 62.790528] do_init_module+0x50/0x1c0 > [ 62.791346] load_module+0x1be4/0x2140 > [ 62.792192] __se_sys_finit_module+0xb8/0xc8 > [ 62.793128] __arm64_sys_finit_module+0x18/0x20 > [ 62.794128] el0_svc_handler+0x88/0x100 > [ 62.794989] el0_svc+0x8/0xc > > The problem can be traced down here. > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > index e038e2b3b7ea..2a410c88c596 100644 > --- a/drivers/base/devres.c > +++ b/drivers/base/devres.c > @@ -33,7 +33,7 @@ struct devres { > * Thus we use ARCH_KMALLOC_MINALIGN here and get exactly the same > * buffer alignment as if it was allocated by plain kmalloc(). > */ > - u8 __aligned(ARCH_KMALLOC_MINALIGN) data[]; > + u8 __aligned(__alignof__(unsigned long long)) data[]; > }; > > On arm64 ARCH_KMALLOC_MINALIGN -> ARCH_DMA_MINALIGN -> 128 > > dev_pagemap being added: > > #define ZONE_DEVICE_PHYS_START 0x680000000UL > #define ZONE_DEVICE_PHYS_END 0x6BFFFFFFFUL > #define ALTMAP_FREE 4096 > #define ALTMAP_RESV 1024 > > pgmap->type = MEMORY_DEVICE_PUBLIC; Given that what seems to ultimately get corrupted is the memory pointed to by pgmap here, how is *that* being allocated? Robin. > pgmap->res.start = ZONE_DEVICE_PHYS_START; > pgmap->res.end = ZONE_DEVICE_PHYS_END; > pgmap->ref = ref; > pgmap->kill = zone_device_percpu_kill; > pgmap->dev = dev; > > memset(&pgmap->altmap, 0, sizeof(struct vmem_altmap)); > pgmap->altmap.free = ALTMAP_FREE; > pgmap->altmap.alloc = 0; > pgmap->altmap.align = 0; > pgmap->altmap_valid = 1; > > tmp = (unsigned long *)&pgmap->altmap.base_pfn; > tmp1 = (unsigned long *)&pgmap->altmap.reserve; > > *tmp = pgmap->res.start >> PAGE_SHIFT; > *tmp1 = ALTMAP_RESV; > > With the patch: > > [ 53.027865] XXX: zone_device_public_altmap_init pgmap ffff8005de634218 resource ffff8005de634250 res->start 680000000 res->end 6bfffffff size 40000000 > [ 53.029840] XXX: devm_memremap_pages_release pgmap ffff8005de634218 resource ffff8005de634250 res->start 680000000 res->end 6bfffffff size 40000000 > > Without the patch: > > [ 34.326066] XXX: zone_device_public_altmap_init pgmap ffff8005de530a80 resource ffff8005de530ab8 res->start 680000000 res->end 6bfffffff size 40000000 > [ 34.328063] XXX: devm_memremap_pages_release pgmap ffff8005de530a80 resource ffff8005de530ab8 res->start 680000000 res->end 0 size fffffff980000001 > > Though this prevents the above corruption I wonder what was causing it in the > first place and how we can address the problem. > > - Anshuman > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >
Hi Anshuman, On Fri, Apr 05, 2019 at 10:10:22AM +0530, Anshuman Khandual wrote: > On arm64 platform "struct dev_pagemap" is getting corrupted during ZONE_DEVICE > unmapping path through device_destroy(). Its device memory range end address > (pgmap->res.end) which is getting corrupted in this particular case. AFAICS > pgmap which gets initialized by the driver and mapped with devm_memremap_pages() > should retain it's values during the unmapping path as well. Is this assumption > right ? [...] > The problem can be traced down here. > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > index e038e2b3b7ea..2a410c88c596 100644 > --- a/drivers/base/devres.c > +++ b/drivers/base/devres.c > @@ -33,7 +33,7 @@ struct devres { > * Thus we use ARCH_KMALLOC_MINALIGN here and get exactly the same > * buffer alignment as if it was allocated by plain kmalloc(). > */ > - u8 __aligned(ARCH_KMALLOC_MINALIGN) data[]; > + u8 __aligned(__alignof__(unsigned long long)) data[]; > }; [...] > With the patch: > > [ 53.027865] XXX: zone_device_public_altmap_init pgmap ffff8005de634218 resource ffff8005de634250 res->start 680000000 res->end 6bfffffff size 40000000 > [ 53.029840] XXX: devm_memremap_pages_release pgmap ffff8005de634218 resource ffff8005de634250 res->start 680000000 res->end 6bfffffff size 40000000 > > Without the patch: > > [ 34.326066] XXX: zone_device_public_altmap_init pgmap ffff8005de530a80 resource ffff8005de530ab8 res->start 680000000 res->end 6bfffffff size 40000000 > [ 34.328063] XXX: devm_memremap_pages_release pgmap ffff8005de530a80 resource ffff8005de530ab8 res->start 680000000 res->end 0 size fffffff980000001 OK, so without this patch pgmap->res.end becomes 0 while it should stay at 0x6bfffffff. Is it easy to reproduce with mainline? What's zone_device_public_altmap_init? I couldn't grep it in mainline. How's the pgmap allocated? I'd suggest you enable kasan and see if it spots anything.
On Fri, Apr 05, 2019 at 10:10:22AM +0530, Anshuman Khandual wrote: > Hello, > > On arm64 platform "struct dev_pagemap" is getting corrupted during ZONE_DEVICE > unmapping path through device_destroy(). Its device memory range end address > (pgmap->res.end) which is getting corrupted in this particular case. AFAICS > pgmap which gets initialized by the driver and mapped with devm_memremap_pages() > should retain it's values during the unmapping path as well. Is this assumption > right ? > > [ 62.779412] Call trace: > [ 62.779808] dump_backtrace+0x0/0x118 > [ 62.780460] show_stack+0x14/0x20 > [ 62.781204] dump_stack+0xa8/0xcc > [ 62.781941] devm_memremap_pages_release+0x24/0x1d8 > [ 62.783021] devm_action_release+0x10/0x18 > [ 62.783911] release_nodes+0x1b0/0x220 > [ 62.784732] devres_release_all+0x34/0x50 > [ 62.785623] device_release+0x24/0x90 > [ 62.786454] kobject_put+0x74/0xe8 > [ 62.787214] device_destroy+0x48/0x58 > [ 62.788041] zone_device_public_altmap_init+0x404/0x42c [zone_device_public_altmap] > [ 62.789675] do_one_initcall+0x74/0x190 > [ 62.790528] do_init_module+0x50/0x1c0 > [ 62.791346] load_module+0x1be4/0x2140 > [ 62.792192] __se_sys_finit_module+0xb8/0xc8 > [ 62.793128] __arm64_sys_finit_module+0x18/0x20 > [ 62.794128] el0_svc_handler+0x88/0x100 > [ 62.794989] el0_svc+0x8/0xc > > The problem can be traced down here. > > diff --git a/drivers/base/devres.c b/drivers/base/devres.c > index e038e2b3b7ea..2a410c88c596 100644 > --- a/drivers/base/devres.c > +++ b/drivers/base/devres.c > @@ -33,7 +33,7 @@ struct devres { > * Thus we use ARCH_KMALLOC_MINALIGN here and get exactly the same > * buffer alignment as if it was allocated by plain kmalloc(). > */ > - u8 __aligned(ARCH_KMALLOC_MINALIGN) data[]; > + u8 __aligned(__alignof__(unsigned long long)) data[]; > }; I doubt that pgmap->res.end get corrupted during device_destroy() but given that the above changes fix the issue it kind of boggle the mind. If i where to debug this i would probably run a kernel with qemu -s to get a gdbserver and then attach gdb and set breakpoint on devm_memremap_pages() then when that trigger i would set memory watch on the pgmap->res.end (there use to be way to use memory as pmem through kernel boot option). A printk alternative solution is, assuming you only have one pgmap, add a global static struct pgmap *debug_pgmap = NULL; in memremap.c set that in devm_memremap_pages() and add an helper function: void debug_pgmap(const char *file, unsigned line) { printk(... file, line); printk(... debug_pmap->res.end); } In a header: #define DEBUG_PGMAP debug_pgmap(__FILE__, __LINE__); Then sprinkle DEBUG_PGMAP within device_destroy(), device_unregister(), device_release() and see when it get corrupted. gdb would be faster but sometime i got issue with memory watchpoint and virt. Cheers, Jérôme
On 04/05/2019 07:07 PM, Robin Murphy wrote: > On 05/04/2019 05:40, Anshuman Khandual wrote: >> Hello, >> >> On arm64 platform "struct dev_pagemap" is getting corrupted during ZONE_DEVICE >> unmapping path through device_destroy(). Its device memory range end address >> (pgmap->res.end) which is getting corrupted in this particular case. AFAICS >> pgmap which gets initialized by the driver and mapped with devm_memremap_pages() >> should retain it's values during the unmapping path as well. Is this assumption >> right ? >> >> [ 62.779412] Call trace: >> [ 62.779808] dump_backtrace+0x0/0x118 >> [ 62.780460] show_stack+0x14/0x20 >> [ 62.781204] dump_stack+0xa8/0xcc >> [ 62.781941] devm_memremap_pages_release+0x24/0x1d8 >> [ 62.783021] devm_action_release+0x10/0x18 >> [ 62.783911] release_nodes+0x1b0/0x220 >> [ 62.784732] devres_release_all+0x34/0x50 >> [ 62.785623] device_release+0x24/0x90 >> [ 62.786454] kobject_put+0x74/0xe8 >> [ 62.787214] device_destroy+0x48/0x58 >> [ 62.788041] zone_device_public_altmap_init+0x404/0x42c [zone_device_public_altmap] >> [ 62.789675] do_one_initcall+0x74/0x190 >> [ 62.790528] do_init_module+0x50/0x1c0 >> [ 62.791346] load_module+0x1be4/0x2140 >> [ 62.792192] __se_sys_finit_module+0xb8/0xc8 >> [ 62.793128] __arm64_sys_finit_module+0x18/0x20 >> [ 62.794128] el0_svc_handler+0x88/0x100 >> [ 62.794989] el0_svc+0x8/0xc >> >> The problem can be traced down here. >> >> diff --git a/drivers/base/devres.c b/drivers/base/devres.c >> index e038e2b3b7ea..2a410c88c596 100644 >> --- a/drivers/base/devres.c >> +++ b/drivers/base/devres.c >> @@ -33,7 +33,7 @@ struct devres { >> * Thus we use ARCH_KMALLOC_MINALIGN here and get exactly the same >> * buffer alignment as if it was allocated by plain kmalloc(). >> */ >> - u8 __aligned(ARCH_KMALLOC_MINALIGN) data[]; >> + u8 __aligned(__alignof__(unsigned long long)) data[]; >> }; >> >> On arm64 ARCH_KMALLOC_MINALIGN -> ARCH_DMA_MINALIGN -> 128 >> >> dev_pagemap being added: >> >> #define ZONE_DEVICE_PHYS_START 0x680000000UL >> #define ZONE_DEVICE_PHYS_END 0x6BFFFFFFFUL >> #define ALTMAP_FREE 4096 >> #define ALTMAP_RESV 1024 >> >> pgmap->type = MEMORY_DEVICE_PUBLIC; > > Given that what seems to ultimately get corrupted is the memory pointed to by pgmap here, how is *that* being allocated? struct dev_pagemap *pgmap; pgmap = devm_kzalloc(dev, sizeof(struct dev_pagemap), GFP_KERNEL); Is it problematic to use dev_kzalloc here instead of generic kmalloc/kzalloc functions ? kzalloc() seems to solve the problem. Will do some more testing tomorrow.
On 04/05/2019 07:12 PM, Catalin Marinas wrote: > Hi Anshuman, > > On Fri, Apr 05, 2019 at 10:10:22AM +0530, Anshuman Khandual wrote: >> On arm64 platform "struct dev_pagemap" is getting corrupted during ZONE_DEVICE >> unmapping path through device_destroy(). Its device memory range end address >> (pgmap->res.end) which is getting corrupted in this particular case. AFAICS >> pgmap which gets initialized by the driver and mapped with devm_memremap_pages() >> should retain it's values during the unmapping path as well. Is this assumption >> right ? > [...] >> The problem can be traced down here. >> >> diff --git a/drivers/base/devres.c b/drivers/base/devres.c >> index e038e2b3b7ea..2a410c88c596 100644 >> --- a/drivers/base/devres.c >> +++ b/drivers/base/devres.c >> @@ -33,7 +33,7 @@ struct devres { >> * Thus we use ARCH_KMALLOC_MINALIGN here and get exactly the same >> * buffer alignment as if it was allocated by plain kmalloc(). >> */ >> - u8 __aligned(ARCH_KMALLOC_MINALIGN) data[]; >> + u8 __aligned(__alignof__(unsigned long long)) data[]; >> }; > [...] >> With the patch: >> >> [ 53.027865] XXX: zone_device_public_altmap_init pgmap ffff8005de634218 resource ffff8005de634250 res->start 680000000 res->end 6bfffffff size 40000000 >> [ 53.029840] XXX: devm_memremap_pages_release pgmap ffff8005de634218 resource ffff8005de634250 res->start 680000000 res->end 6bfffffff size 40000000 >> >> Without the patch: >> >> [ 34.326066] XXX: zone_device_public_altmap_init pgmap ffff8005de530a80 resource ffff8005de530ab8 res->start 680000000 res->end 6bfffffff size 40000000 >> [ 34.328063] XXX: devm_memremap_pages_release pgmap ffff8005de530a80 resource ffff8005de530ab8 res->start 680000000 res->end 0 size fffffff980000001 > > OK, so without this patch pgmap->res.end becomes 0 while it should stay > at 0x6bfffffff. Is it easy to reproduce with mainline? Yeah but with some ZONE_DEVICE series patches. > > What's zone_device_public_altmap_init? I couldn't grep it in mainline. Test module inti function (should have mentioned it clearly). > How's the pgmap allocated? devm_kzalloc() but the problem seems to be gone with kzalloc(). > > I'd suggest you enable kasan and see if it spots anything. > Sure.
On 05/04/2019 15:54, Anshuman Khandual wrote: > > > On 04/05/2019 07:07 PM, Robin Murphy wrote: >> On 05/04/2019 05:40, Anshuman Khandual wrote: >>> Hello, >>> >>> On arm64 platform "struct dev_pagemap" is getting corrupted during ZONE_DEVICE >>> unmapping path through device_destroy(). Its device memory range end address >>> (pgmap->res.end) which is getting corrupted in this particular case. AFAICS >>> pgmap which gets initialized by the driver and mapped with devm_memremap_pages() >>> should retain it's values during the unmapping path as well. Is this assumption >>> right ? >>> >>> [ 62.779412] Call trace: >>> [ 62.779808] dump_backtrace+0x0/0x118 >>> [ 62.780460] show_stack+0x14/0x20 >>> [ 62.781204] dump_stack+0xa8/0xcc >>> [ 62.781941] devm_memremap_pages_release+0x24/0x1d8 >>> [ 62.783021] devm_action_release+0x10/0x18 >>> [ 62.783911] release_nodes+0x1b0/0x220 >>> [ 62.784732] devres_release_all+0x34/0x50 >>> [ 62.785623] device_release+0x24/0x90 >>> [ 62.786454] kobject_put+0x74/0xe8 >>> [ 62.787214] device_destroy+0x48/0x58 >>> [ 62.788041] zone_device_public_altmap_init+0x404/0x42c [zone_device_public_altmap] >>> [ 62.789675] do_one_initcall+0x74/0x190 >>> [ 62.790528] do_init_module+0x50/0x1c0 >>> [ 62.791346] load_module+0x1be4/0x2140 >>> [ 62.792192] __se_sys_finit_module+0xb8/0xc8 >>> [ 62.793128] __arm64_sys_finit_module+0x18/0x20 >>> [ 62.794128] el0_svc_handler+0x88/0x100 >>> [ 62.794989] el0_svc+0x8/0xc >>> >>> The problem can be traced down here. >>> >>> diff --git a/drivers/base/devres.c b/drivers/base/devres.c >>> index e038e2b3b7ea..2a410c88c596 100644 >>> --- a/drivers/base/devres.c >>> +++ b/drivers/base/devres.c >>> @@ -33,7 +33,7 @@ struct devres { >>> * Thus we use ARCH_KMALLOC_MINALIGN here and get exactly the same >>> * buffer alignment as if it was allocated by plain kmalloc(). >>> */ >>> - u8 __aligned(ARCH_KMALLOC_MINALIGN) data[]; >>> + u8 __aligned(__alignof__(unsigned long long)) data[]; >>> }; >>> >>> On arm64 ARCH_KMALLOC_MINALIGN -> ARCH_DMA_MINALIGN -> 128 >>> >>> dev_pagemap being added: >>> >>> #define ZONE_DEVICE_PHYS_START 0x680000000UL >>> #define ZONE_DEVICE_PHYS_END 0x6BFFFFFFFUL >>> #define ALTMAP_FREE 4096 >>> #define ALTMAP_RESV 1024 >>> >>> pgmap->type = MEMORY_DEVICE_PUBLIC; >> >> Given that what seems to ultimately get corrupted is the memory pointed to by pgmap here, how is *that* being allocated? > > struct dev_pagemap *pgmap; > > pgmap = devm_kzalloc(dev, sizeof(struct dev_pagemap), GFP_KERNEL); > > Is it problematic to use dev_kzalloc here instead of generic kmalloc/kzalloc > functions ? kzalloc() seems to solve the problem. Will do some more testing > tomorrow. The important point is that your pgmap is a devres allocation itself, thus changing the alignment of devres::data is moving the thing which was getting corrupted to a different relative location: (gdb) p sizeof(struct devres) + sizeof(struct dev_pagemap) $1 = 296 (gdb) p sizeof(struct devres) + (size_t)&(((struct dev_pagemap*)0)->res.end) $2 = 192 vs. (gdb) p sizeof(struct devres_node) + sizeof(struct dev_pagemap) $3 = 192 (gdb) p sizeof(struct devres_node) + (size_t)&(((struct dev_pagemap*)0)->res.end) $4 = 88 The fact that the corruption of the 128-byte-aligned devres::data occurs exactly where the 8-byte-aligned devres::data would have ended is particularly suspicious... Robin.
On Fri, Apr 5, 2019 at 7:54 AM Anshuman Khandual <anshuman.khandual@arm.com> wrote: [..] > > Given that what seems to ultimately get corrupted is the memory pointed to by pgmap here, how is *that* being allocated? > > struct dev_pagemap *pgmap; > > pgmap = devm_kzalloc(dev, sizeof(struct dev_pagemap), GFP_KERNEL); > > Is it problematic to use dev_kzalloc here instead of generic kmalloc/kzalloc > functions ? On this specific question, no. devm_kzalloc() is how the pmem and device-dax drivers allocate the pgmap passed to devm_memremap_pages(). The unwind order of the devres resources ensures that the devm_memremap_pages() devres actions occur before the release of the pgmap allocation.
diff --git a/drivers/base/devres.c b/drivers/base/devres.c index e038e2b3b7ea..2a410c88c596 100644 --- a/drivers/base/devres.c +++ b/drivers/base/devres.c @@ -33,7 +33,7 @@ struct devres { * Thus we use ARCH_KMALLOC_MINALIGN here and get exactly the same * buffer alignment as if it was allocated by plain kmalloc(). */ - u8 __aligned(ARCH_KMALLOC_MINALIGN) data[]; + u8 __aligned(__alignof__(unsigned long long)) data[]; }; On arm64 ARCH_KMALLOC_MINALIGN -> ARCH_DMA_MINALIGN -> 128