There's a chance that the IDA allocated in mmc_alloc_host() is not freed
for some time because it's freed as part of a class' release function
(see mmc_host_classdev_release() where the IDA is freed). If another
thread is holding a reference to the class, then only once all balancing
device_put() calls (in turn calling kobject_put()) have been made will
the IDA be released and usable again.
Normally this isn't a problem because the kobject is released before
anything else that may want to use the same number tries to again, but
with CONFIG_DEBUG_KOBJECT_RELEASE=y and OF aliases it becomes pretty
easy to try to allocate an alias from the IDA twice while the first time
it was allocated is still pending a call to ida_simple_remove(). It's
also possible to trigger it by using CONFIG_DEBUG_KOBJECT_RELEASE and
probe defering a driver at boot that calls mmc_alloc_host() before
trying to get resources that may defer likes clks or regulators.
Instead of allocating from the IDA in this scenario, let's just skip it
if we know this is an OF alias. The number is already "claimed" and
devices that aren't using OF aliases won't try to use the claimed
numbers anyway (see mmc_first_nonreserved_index()). This should avoid
any issues with mmc_alloc_host() returning failures from the
ida_simple_get() in the case that we're using an OF alias.
Cc: Matthias Schiffer <[email protected]>
Cc: Sujit Kautkar <[email protected]>
Reported-by: Zubin Mithra <[email protected]>
Fixes: fa2d0aa96941 ("mmc: core: Allow setting slot index via device tree alias")
Signed-off-by: Stephen Boyd <[email protected]>
---
drivers/mmc/core/host.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 9b89a91b6b47..137b4a769f62 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -39,7 +39,8 @@ static void mmc_host_classdev_release(struct device *dev)
{
struct mmc_host *host = cls_dev_to_mmc_host(dev);
wakeup_source_unregister(host->ws);
- ida_simple_remove(&mmc_host_ida, host->index);
+ if (of_alias_get_id(host->parent->of_node, "mmc") < 0)
+ ida_simple_remove(&mmc_host_ida, host->index);
kfree(host);
}
@@ -444,7 +445,7 @@ static int mmc_first_nonreserved_index(void)
*/
struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
{
- int err;
+ int index;
struct mmc_host *host;
int alias_id, min_idx, max_idx;
@@ -457,20 +458,19 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
alias_id = of_alias_get_id(dev->of_node, "mmc");
if (alias_id >= 0) {
- min_idx = alias_id;
- max_idx = alias_id + 1;
+ index = alias_id;
} else {
min_idx = mmc_first_nonreserved_index();
max_idx = 0;
- }
- err = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
- if (err < 0) {
- kfree(host);
- return NULL;
+ index = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
+ if (index < 0) {
+ kfree(host);
+ return NULL;
+ }
}
- host->index = err;
+ host->index = index;
dev_set_name(&host->class_dev, "mmc%d", host->index);
host->ws = wakeup_source_register(NULL, dev_name(&host->class_dev));
--
https://chromeos.dev
On Tue, 13 Apr 2021 at 02:36, Stephen Boyd <[email protected]> wrote:
>
> There's a chance that the IDA allocated in mmc_alloc_host() is not freed
> for some time because it's freed as part of a class' release function
> (see mmc_host_classdev_release() where the IDA is freed). If another
> thread is holding a reference to the class, then only once all balancing
> device_put() calls (in turn calling kobject_put()) have been made will
> the IDA be released and usable again.
>
> Normally this isn't a problem because the kobject is released before
> anything else that may want to use the same number tries to again, but
> with CONFIG_DEBUG_KOBJECT_RELEASE=y and OF aliases it becomes pretty
> easy to try to allocate an alias from the IDA twice while the first time
> it was allocated is still pending a call to ida_simple_remove(). It's
> also possible to trigger it by using CONFIG_DEBUG_KOBJECT_RELEASE and
> probe defering a driver at boot that calls mmc_alloc_host() before
> trying to get resources that may defer likes clks or regulators.
Thanks for a very nice description of the problem.
>
> Instead of allocating from the IDA in this scenario, let's just skip it
> if we know this is an OF alias. The number is already "claimed" and
> devices that aren't using OF aliases won't try to use the claimed
> numbers anyway (see mmc_first_nonreserved_index()). This should avoid
> any issues with mmc_alloc_host() returning failures from the
> ida_simple_get() in the case that we're using an OF alias.
At first glance, this seems like a good idea, but I am not completely
sure, yet. See more below.
>
> Cc: Matthias Schiffer <[email protected]>
> Cc: Sujit Kautkar <[email protected]>
> Reported-by: Zubin Mithra <[email protected]>
> Fixes: fa2d0aa96941 ("mmc: core: Allow setting slot index via device tree alias")
> Signed-off-by: Stephen Boyd <[email protected]>
> ---
> drivers/mmc/core/host.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 9b89a91b6b47..137b4a769f62 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -39,7 +39,8 @@ static void mmc_host_classdev_release(struct device *dev)
> {
> struct mmc_host *host = cls_dev_to_mmc_host(dev);
> wakeup_source_unregister(host->ws);
> - ida_simple_remove(&mmc_host_ida, host->index);
> + if (of_alias_get_id(host->parent->of_node, "mmc") < 0)
> + ida_simple_remove(&mmc_host_ida, host->index);
> kfree(host);
> }
>
> @@ -444,7 +445,7 @@ static int mmc_first_nonreserved_index(void)
> */
> struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
> {
> - int err;
> + int index;
> struct mmc_host *host;
> int alias_id, min_idx, max_idx;
>
> @@ -457,20 +458,19 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
>
> alias_id = of_alias_get_id(dev->of_node, "mmc");
> if (alias_id >= 0) {
> - min_idx = alias_id;
> - max_idx = alias_id + 1;
> + index = alias_id;
> } else {
> min_idx = mmc_first_nonreserved_index();
> max_idx = 0;
> - }
>
> - err = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
> - if (err < 0) {
> - kfree(host);
> - return NULL;
> + index = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
> + if (index < 0) {
> + kfree(host);
> + return NULL;
> + }
This means that a DTB that is screwed up in a way that it has two mmc
aliases with the same index, would be allowed to use the same index.
What will happen when we continue the probe sequence in such a case?
> }
>
> - host->index = err;
> + host->index = index;
>
> dev_set_name(&host->class_dev, "mmc%d", host->index);
> host->ws = wakeup_source_register(NULL, dev_name(&host->class_dev));
Another concern that could potentially be a problem, is that the
"thread" that holds the reference that prevents ida from being
removed, how would that react to a new mmc device to become
re-registered with the same index?
I wonder if we perhaps should return -EPROBE_DEFER instead, when
ida_simple_get() fails?
Kind regards
Uffe
Quoting Ulf Hansson (2021-04-15 01:56:12)
> On Tue, 13 Apr 2021 at 02:36, Stephen Boyd <[email protected]> wrote:
> >
> > - err = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
> > - if (err < 0) {
> > - kfree(host);
> > - return NULL;
> > + index = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
> > + if (index < 0) {
> > + kfree(host);
> > + return NULL;
> > + }
>
> This means that a DTB that is screwed up in a way that it has two mmc
> aliases with the same index, would be allowed to use the same index.
>
> What will happen when we continue the probe sequence in such a case?
Yeah I thought about this after sending the patch. So the problem would
be like this right?
aliases {
mmc1 = &sdhci0;
mmc1 = &sdhci1;
};
I have good news! DT won't compile it because it saw the same alias
assigned to twice. I tried it on my sc7180 board.
arch/arm64/boot/dts/qcom/sc7180.dtsi:35.3-18:
ERROR (duplicate_property_names): /aliases:mmc1: Duplicate property name
ERROR: Input tree has errors, aborting (use -f to force output)
arch/arm64/boot/dts/qcom/sc7180-idp.dtb] Error 2
I suppose if someone forced the compilation it may be bad, but do we
really care?
TL;DR: this seems like it isn't a problem.
>
> > }
> >
> > - host->index = err;
> > + host->index = index;
> >
> > dev_set_name(&host->class_dev, "mmc%d", host->index);
> > host->ws = wakeup_source_register(NULL, dev_name(&host->class_dev));
>
> Another concern that could potentially be a problem, is that the
> "thread" that holds the reference that prevents ida from being
> removed, how would that react to a new mmc device to become
> re-registered with the same index?
>
> I wonder if we perhaps should return -EPROBE_DEFER instead, when
> ida_simple_get() fails?
>
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.
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.
(Sorry if that is long. I'm sort of stream of conciousness writing it to
you here and not rewriting it to be more concise)
On Thu, 15 Apr 2021 at 21:29, Stephen Boyd <[email protected]> wrote:
>
> Quoting Ulf Hansson (2021-04-15 01:56:12)
> > On Tue, 13 Apr 2021 at 02:36, Stephen Boyd <[email protected]> wrote:
> > >
> > > - err = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
> > > - if (err < 0) {
> > > - kfree(host);
> > > - return NULL;
> > > + index = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
> > > + if (index < 0) {
> > > + kfree(host);
> > > + return NULL;
> > > + }
> >
> > This means that a DTB that is screwed up in a way that it has two mmc
> > aliases with the same index, would be allowed to use the same index.
> >
> > What will happen when we continue the probe sequence in such a case?
>
> Yeah I thought about this after sending the patch. So the problem would
> be like this right?
>
> aliases {
> mmc1 = &sdhci0;
> mmc1 = &sdhci1;
> };
Correct.
>
> I have good news! DT won't compile it because it saw the same alias
> assigned to twice. I tried it on my sc7180 board.
>
> arch/arm64/boot/dts/qcom/sc7180.dtsi:35.3-18:
> ERROR (duplicate_property_names): /aliases:mmc1: Duplicate property name
> ERROR: Input tree has errors, aborting (use -f to force output)
> arch/arm64/boot/dts/qcom/sc7180-idp.dtb] Error 2
>
> I suppose if someone forced the compilation it may be bad, but do we
> really care?
>
> TL;DR: this seems like it isn't a problem.
Yep, I definitely tend to agree with you here. Thanks for doing the
test and sharing the result.
>
> >
> > > }
> > >
> > > - host->index = err;
> > > + host->index = index;
> > >
> > > dev_set_name(&host->class_dev, "mmc%d", host->index);
> > > host->ws = wakeup_source_register(NULL, dev_name(&host->class_dev));
> >
> > Another concern that could potentially be a problem, is that the
> > "thread" that holds the reference that prevents ida from being
> > removed, how would that react to a new mmc device to become
> > re-registered with the same index?
> >
> > I wonder if we perhaps should return -EPROBE_DEFER instead, when
> > ida_simple_get() fails?
> >
>
> 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.
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.
>
> (Sorry if that is long. I'm sort of stream of conciousness writing it to
> you here and not rewriting it to be more concise)
No worries, thanks a lot for sharing your thoughts!
Kind regards
Uffe
Quoting Ulf Hansson (2021-04-16 00:17:10)
> On Thu, 15 Apr 2021 at 21:29, Stephen Boyd <[email protected]> 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.
>
> 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.
Will you apply this patch?
On Fri, 16 Apr 2021 at 19:50, Stephen Boyd <[email protected]> wrote:
>
> Quoting Ulf Hansson (2021-04-16 00:17:10)
> > On Thu, 15 Apr 2021 at 21:29, Stephen Boyd <[email protected]> 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
Quoting Ulf Hansson (2021-04-16 16:07:11)
> On Fri, 16 Apr 2021 at 19:50, Stephen Boyd <[email protected]> wrote:
> >
> > 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.
Thanks! I'm just getting back to this mail on a Friday afternoon!
>
> /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
I see that this is a different IDA managed object, for mmc_blk_ida.
Presumably the object allocated is md?
md = kzalloc(sizeof(struct mmc_blk_data), GFP_KERNEL);
Is that the line?
> [ 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
Looking at mmc_blk_put() and mmc_blk_get() I see
static struct mmc_blk_data *mmc_blk_get(struct gendisk *disk)
{
...
md = disk->private_data;
if (md && md->usage == 0)
md = NULL;
...
}
static void mmc_blk_put(struct mmc_blk_data *md)
{
...
if (md->usage == 0) {
int devidx = mmc_get_devidx(md->disk);
blk_put_queue(md->queue.queue);
ida_simple_remove(&mmc_blk_ida, devidx);
put_disk(md->disk);
kfree(md);
}
...
}
and notice that mmc_blk_get() takes a gendisk but mmc_blk_put() takes a
mmc_blk_data. That's already weird, but then I notice that 'md' comes
from disk->private_data in mmc_blk_get() and in mmc_blk_put() we kfree
'md' if usage drops to 0. The storage for md is inside
disk->private_data, according to mmc_blk_alloc_req()
md->disk->private_data = md
so 'md' points to itself through gendisk private_data. Alright.
Either way, KASAN is telling us that 'md' got freed in mmc_blk_put() but
the gendisk is still around and valid, because it's held alive via some
kobject_get(). When we go to blkdev_open() we have that gendisk that has
private_data pointing to the freed 'md'.
This is a classic dangling pointer bug.
Given that mmc_blk_get() is checking for disk->private_data being
non-NULL, I looked to see where that is assigned to NULL, but I don't
see it. Is it ever set to NULL? We could set private_data to NULL after
freeing it, but that feels hacky. The md->usage code looks like a kref
design open-coded; probably replace that with a kref that does the code
in the if (md->usage == 0) path on the kref release function and then we
wouldn't need a mutex around these get/put APIs.
Does this patch fix it?
---8<----
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index d666e24fbe0e..c7939e8fe76f 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -203,6 +203,7 @@ static void mmc_blk_put(struct mmc_blk_data *md)
int devidx = mmc_get_devidx(md->disk);
blk_put_queue(md->queue.queue);
ida_simple_remove(&mmc_blk_ida, devidx);
+ md->disk->private_data = NULL;
put_disk(md->disk);
kfree(md);
}
> [ 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
If you're interested in the kref patch it may also clean it up a bit. I
left the mutex in place, but otherwise now the refcount is an atomic
variable, so mmc_blk_put() can happen in parallel to mmc_blk_get() and
the release part can run later. This lets mmc_blk_get() start failing
earlier and returning NULL when the last user has called mmc_blk_put()
but the disk is still hanging around. No idea if this works though.
----8<----
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index d666e24fbe0e..8a300cc2c8be 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -27,6 +27,7 @@
#include <linux/errno.h>
#include <linux/hdreg.h>
#include <linux/kdev_t.h>
+#include <linux/kref.h>
#include <linux/blkdev.h>
#include <linux/cdev.h>
#include <linux/mutex.h>
@@ -110,7 +111,7 @@ struct mmc_blk_data {
#define MMC_BLK_CMD23 (1 << 0) /* Can do SET_BLOCK_COUNT for multiblock */
#define MMC_BLK_REL_WR (1 << 1) /* MMC Reliable write support */
- unsigned int usage;
+ struct kref kref;
unsigned int read_only;
unsigned int part_type;
unsigned int reset_done;
@@ -180,10 +181,8 @@ static struct mmc_blk_data *mmc_blk_get(struct
gendisk *disk)
mutex_lock(&open_lock);
md = disk->private_data;
- if (md && md->usage == 0)
+ if (md && !kref_get_unless_zero(&md->kref))
md = NULL;
- if (md)
- md->usage++;
mutex_unlock(&open_lock);
return md;
@@ -195,20 +194,26 @@ static inline int mmc_get_devidx(struct gendisk *disk)
return devidx;
}
-static void mmc_blk_put(struct mmc_blk_data *md)
+static void mmc_blk_kref_release(struct kref *ref)
{
+ struct mmc_blk_data *md = container_of(ref, struct mmc_blk_data, kref);
+ int devidx;
+
mutex_lock(&open_lock);
- md->usage--;
- if (md->usage == 0) {
- int devidx = mmc_get_devidx(md->disk);
- blk_put_queue(md->queue.queue);
- ida_simple_remove(&mmc_blk_ida, devidx);
- put_disk(md->disk);
- kfree(md);
- }
+ devidx = mmc_get_devidx(md->disk);
+ blk_put_queue(md->queue.queue);
+ ida_simple_remove(&mmc_blk_ida, devidx);
+ md->disk->private_data = NULL;
+ put_disk(md->disk);
+ kfree(md);
mutex_unlock(&open_lock);
}
+static void mmc_blk_put(struct mmc_blk_data *md)
+{
+ kref_put(&md->kref, mmc_blk_kref_release);
+}
+
static ssize_t power_ro_lock_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -2300,7 +2305,7 @@ static struct mmc_blk_data
*mmc_blk_alloc_req(struct mmc_card *card,
INIT_LIST_HEAD(&md->part);
INIT_LIST_HEAD(&md->rpmbs);
- md->usage = 1;
+ kref_init(&md->kref);
ret = mmc_init_queue(&md->queue, card);
if (ret)