Message-ID: <CAPDyKFryT63Jc7+DXWSpAC19qpZRqFr1orxwYGMuSqx247O8cQ@mail.gmail.com>
Date:   Sat, 17 Apr 2021 01:07:11 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-mmc <linux-mmc@...r.kernel.org>,
        Matthias Schiffer <matthias.schiffer@...tq-group.com>,
        Sujit Kautkar <sujitka@...omium.org>,
        Zubin Mithra <zsm@...omium.org>
Subject: Re: [PATCH] mmc: core: Don't allocate IDA for OF aliases

On Fri, 16 Apr 2021 at 19:50, Stephen Boyd <swboyd@...omium.org> wrote:
>
> Quoting Ulf Hansson (2021-04-16 00:17:10)
> > On Thu, 15 Apr 2021 at 21:29, Stephen Boyd <swboyd@...omium.org> 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

Powered by blists - more mailing lists