Re: struct dev_pagemap corruption

[Robin Murphy <robin.murphy@arm.com>]

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.