Re: [PATCH] mmc: core: Don't allocate IDA for OF aliases

[Ulf Hansson]

On Fri, 16 Apr 2021 at 19:50, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
>
> Quoting Ulf Hansson (2021-04-16 00:17:10)
> > On Thu, 15 Apr 2021 at 21:29, Stephen Boyd <swboyd@xxxxxxxxxxxx> wrote:
> > >
> > >
> > > Don't think so. The device (with the kobject inside) is removed, and
> > > thus the mmc1 device will be removed, but the kobject's release function
> > > is delayed due to the config. This means that
> > > mmc_host_classdev_release() is called at a later time. The only thing
> > > inside that function is the IDA removal and the kfree of the container
> > > object. Given that nothing else is in that release function I believe it
> > > is safe to skip IDA allocation as it won't be blocking anything in the
> > > reserved alias case.
> > >
> > > Furthermore, when the device is deleted in mmc_remove_host() there could
> > > be other users of the device that haven't called put_device() yet.
> > > Either way, those other users are keeping the device memory alive, but
> > > otherwise device_del() has unlinked it from the various driver core
> > > lists and sysfs has removed it too so it's in a state where code may be
> > > referencing it but it's on the way out so users of the device will not
> > > be able to do much with it during this time.
> >
> > Right, but see more below.
> >
> > >
> > > This sort of problem (if it exists which I don't think it does) would
> > > have been there all along and can't be fixed at this level. When a
> > > device that has an alias calls the mmc_alloc_host() function twice it
> > > gets two different device structures created so there are two distinct
> > > kobjects that will need to be released at some point. The index is
> > > usually different for those two kobjects, but with aliases it turns out
> > > it is the same. When it comes to registering that device with the same
> > > name the second one will fail because a device with that name already
> > > exists on the bus. This would be really hard to do given that it would
> > > need to be the same aliased device in DT calling the mmc_add_host()
> > > function without calling mmc_remove_host() for the first one it added in
> > > between.
> >
> > In fact, we have a few rare corner cases that can trigger KASAN splats
> > when mmc_remove_host() gets executed. Similar splats can be triggered
> > by just doing a sudden card removal. It's especially related to the
> > cases, where a thread holds a reference to the card/host object when
> > it's being removed. I am working on patches to fix these cases, but
> > haven't yet decided on the best solution.
>
> Ok. Can you share the KASAN reports? The memory allocated for this class
> object and associated index from the IDA will be unique for each call
> the mmc_alloc_host() so I don't see how KASAN could be noticing
> something unless a reference to the host is leaking out without the
> proper get_device() call being made to keep the underlying memory from
> being freed.

Removing the host, also means removing the card. Although, as I said,
I need some more time to think of the best solution.

Here's a report, triggered with some manual hacks and by using the
mmc-utils usesland tool.

/mmc status get /dev/mmcblk1
[   95.905913] DEBUG: mmc_blk_open: Let's sleep for 10s..
[   98.806639] mmc1: card 0007 removed
[  105.972139] BUG: KASAN: use-after-free in mmc_blk_get+0x58/0xb8
[  105.979144] Read of size 4 at addr ffff00000a394a28 by task mmc/180
[  105.984945]
[  105.991209] CPU: 2 PID: 180 Comm: mmc Not tainted
5.10.0-rc4-00069-gcc758c8c7127-dirty #5
[  105.992943] Hardware name: Qualcomm Technologies, Inc. APQ 8016 SBC (DT)
[  106.001010] Call trace:
[  106.007799]  dump_backtrace+0x0/0x2b4
[  106.009965]  show_stack+0x18/0x6c
[  106.013779]  dump_stack+0xfc/0x168
[  106.017083]  print_address_description.constprop.0+0x6c/0x488
[  106.020384]  kasan_report+0x118/0x210
[  106.026193]  __asan_load4+0x94/0xd0
[  106.029844]  mmc_blk_get+0x58/0xb8
[  106.033141]  mmc_blk_open+0x7c/0xdc
[  106.036614]  __blkdev_get+0x3b4/0x964
[  106.039996]  blkdev_get+0x64/0x100
[  106.043815]  blkdev_open+0xe8/0x104
[  106.047114]  do_dentry_open+0x234/0x61c
[  106.050498]  vfs_open+0x54/0x64
[  106.054324]  path_openat+0xe04/0x1584
[  106.057450]  do_filp_open+0xe8/0x1e4
[  106.061263]  do_sys_openat2+0x120/0x230
[  106.064911]  __arm64_sys_openat+0xf0/0x15c
[  106.068477]  el0_svc_common.constprop.0+0xac/0x234
[  106.072639]  do_el0_svc+0x84/0xa0
[  106.077410]  el0_sync_handler+0x264/0x270
[  106.080790]  el0_sync+0x174/0x180
[  106.084768]
[  106.088070] Allocated by task 33:
[  106.089647]  stack_trace_save+0x9c/0xdc
[  106.092858]  kasan_save_stack+0x28/0x60
[  106.096506]  __kasan_kmalloc.constprop.0+0xc8/0xf0
[  106.100325]  kasan_kmalloc+0x10/0x20
[  106.105183]  mmc_blk_alloc_req+0x94/0x4b0
[  106.108913]  mmc_blk_probe+0x2d4/0xaa4
[  106.112829]  mmc_bus_probe+0x34/0x4c
[  106.116471]  really_probe+0x148/0x6e0
[  106.120202]  driver_probe_device+0x78/0xec
[  106.123764]  __device_attach_driver+0x108/0x16c
[  106.127754]  bus_for_each_drv+0xf4/0x15c
[  106.132180]  __device_attach+0x168/0x240
[  106.136349]  device_initial_probe+0x14/0x20
[  106.140253]  bus_probe_device+0xec/0x100
[  106.144167]  device_add+0x55c/0xaf0
[  106.148332]  mmc_add_card+0x288/0x380
[  106.151540]  mmc_attach_sd+0x18c/0x22c
[  106.155363]  mmc_rescan+0x444/0x4f0
[  106.159014]  process_one_work+0x3b8/0x650
[  106.162396]  worker_thread+0xa0/0x724
[  106.166556]  kthread+0x218/0x220
[  106.170201]  ret_from_fork+0x10/0x38
[  106.173482]
[  106.177045] Freed by task 33:
[  106.178533]  stack_trace_save+0x9c/0xdc
[  106.181399]  kasan_save_stack+0x28/0x60
[  106.185045]  kasan_set_track+0x28/0x40
[  106.188868]  kasan_set_free_info+0x24/0x4c
[  106.192684]  __kasan_slab_free+0x100/0x180
[  106.196764]  kasan_slab_free+0x14/0x20
[  106.200838]  kfree+0xb8/0x46c
[  106.204583]  mmc_blk_put+0xe4/0x11c
[  106.207624]  mmc_blk_remove_req.part.0+0x6c/0xe4
[  106.210914]  mmc_blk_remove+0x368/0x370
[  106.215778]  mmc_bus_remove+0x34/0x50
[  106.219336]  __device_release_driver+0x228/0x31c
[  106.223155]  device_release_driver+0x2c/0x44
[  106.227840]  bus_remove_device+0x1e4/0x200
[  106.232100]  device_del+0x2b0/0x770
[  106.236005]  mmc_remove_card+0xf0/0x150
[  106.239383]  mmc_sd_detect+0x9c/0x150
[  106.243207]  mmc_rescan+0x110/0x4f0
[  106.247032]  process_one_work+0x3b8/0x650
[  106.250329]  worker_thread+0xa0/0x724
[  106.254488]  kthread+0x218/0x220
[  106.258134]  ret_from_fork+0x10/0x38
[  106.261416]
[  106.264986] The buggy address belongs to the object at ffff00000a394800
[  106.264986]  which belongs to the cache kmalloc-1k of size 1024
[  106.266485] The buggy address is located 552 bytes inside of
[  106.266485]  1024-byte region [ffff00000a394800, ffff00000a394c00)
[  106.278710] The buggy address belongs to the page:
[  106.290520] page:00000000ff84ed53 refcount:1 mapcount:0
mapping:0000000000000000 index:0x0 pfn:0x8a390
[  106.295381] head:00000000ff84ed53 order:3 compound_mapcount:0
compound_pincount:0
[  106.304669] flags: 0x3fffc0000010200(slab|head)
[  106.312234] raw: 03fffc0000010200 dead000000000100 dead000000000122
ffff000009f03800
[  106.316571] raw: 0000000000000000 0000000000100010 00000001ffffffff
0000000000000000
[  106.324537] page dumped because: kasan: bad access detected
[  106.332254]
[  106.337543] Memory state around the buggy address:
[  106.339302]  ffff00000a394900: fb fb fb fb fb fb fb fb fb fb fb fb
fb fb fb fb
[  106.343907]  ffff00000a394980: fb fb fb fb fb fb fb fb fb fb fb fb
fb fb fb fb
[  106.351111] >ffff00000a394a00: fb fb fb fb fb fb fb fb fb fb fb fb
fb fb fb fb
[  106.358303]                                   ^
[  106.365515]  ffff00000a394a80: fb fb fb fb fb fb fb fb fb fb fb fb
fb fb fb fb
[  106.369948]  ffff00000a394b00: fb fb fb fb fb fb fb fb fb fb fb fb
fb fb fb fb
[  106.377225] ==================================================================
[  106.384429] Disabling lock debugging due to kernel taint
open: No such device or address

>
> >
> > That's the reason why I was thinking that maybe returning
> > -EPROBE_DEFER could be an option, but certainly I need to think more
> > about this.
>
> I was hoping that you wouldn't need to think more about returning
> -EPROBE_DEFER after my email. :( Returning EPROBE_DEFER looks like it's
> a bandaid for bigger problems with reference counting the pointer
> allocated in mmc_alloc_host(). I hope I convinced you that there isn't
> any danger in reusing the IDA in the case of an alias because the only
> way that is a problem is if the same device calls mmc_alloc_host() twice
> without calling mmc_remove_host() in between. That should be a pretty
> obvious problem in driver code? The check to see if that same device has
> tried to alloc a host twice would still effectively happen after this
> patch because when mmc_add_host() tries to add that second device to the
> driver core it will complain about duplicate device names and fail.

You may very well be correct in you reasoning above, but I just need
to convince myself about it.

>
> Will you apply this patch?

It's likely, but allow me some more time to think about it. To make
sure we do the right thing.

Kind regards
Uffe