| Message ID | 20171002115918.11035-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: block: Fix bug when removing RPMB chardev | expand |
On 02/10/17 14:59, Linus Walleij wrote: > I forgot to account for the fact that the device core holds a > reference to a device added with device_initialize() that need > to be released with a corresponding put_device() to reach a 0 > refcount at the end of the lifecycle. > > This led to a NULL pointer reference when freeing the device > when e.g. unbidning the host device in sysfs. > > Fix this and use the device .release() callback to free the > IDA and free:ing the memory used by the RPMB device. > > Before this patch: > > /sys/bus/amba/drivers/mmci-pl18x$ echo 80114000.sdi4_per2 > unbind > [ 29.797332] mmc3: card 0001 removed > [ 29.810791] Unable to handle kernel NULL pointer dereference at > virtual address 00000050 > [ 29.818878] pgd = de70c000 > [ 29.821624] [00000050] *pgd=1e70a831, *pte=00000000, *ppte=00000000 > [ 29.827911] Internal error: Oops: 17 [#1] PREEMPT SMP ARM > [ 29.833282] Modules linked in: > [ 29.836334] CPU: 1 PID: 154 Comm: sh Not tainted > 4.14.0-rc3-00039-g83318e309566-dirty #736 > [ 29.844604] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support) > [ 29.851562] task: de572700 task.stack: de742000 > [ 29.856079] PC is at kernfs_find_ns+0x8/0x100 > [ 29.860443] LR is at kernfs_find_and_get_ns+0x30/0x48 > > After this patch: > > /sys/bus/amba/drivers/mmci-pl18x$ echo 80005000.sdi4_per2 > unbind > [ 20.623382] mmc3: card 0001 removed > [ 20.627288] mmc_rpmb mmcblk3rpmb: removed > > Fixes: 0ab0c7c0e65a ("mmc: block: Convert RPMB to a character device") > Reported-by: Adrian Hunter <adrian.hunter@intel.com> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/mmc/core/block.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index c29dbcec7c61..2033a71bacc3 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -2329,6 +2329,14 @@ static const struct file_operations mmc_rpmb_fileops = { > #endif > }; > > +static void mmc_blk_rpmb_device_release(struct device *dev) > +{ > + struct mmc_rpmb_data *rpmb = dev_get_drvdata(dev); > + > + dev_info(dev, "removed\n"); Do we really need this message? > + ida_simple_remove(&mmc_rpmb_ida, rpmb->id); > + kfree(rpmb); > +} > > static int mmc_blk_alloc_rpmb_part(struct mmc_card *card, > struct mmc_blk_data *md, > @@ -2371,6 +2379,8 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card, > goto out_remove_ida; > } > > + /* From this point, the .release() function cleans up the device */ > + rpmb->dev.release = mmc_blk_rpmb_device_release; This should be before device_initialize(&rpmb->dev) then the error path becomes just put_device(&rpmb->dev). But further up is: rpmb = kzalloc(sizeof(*rpmb), GFP_KERNEL); if (!rpmb) return -ENOMEM; where it looks like you are leaking the ida. > list_add(&rpmb->node, &md->rpmbs); > > string_get_size((u64)size, 512, STRING_UNITS_2, > @@ -2384,17 +2394,22 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card, > return 0; > > out_remove_ida: > + put_device(&rpmb->dev); > ida_simple_remove(&mmc_rpmb_ida, rpmb->id); > kfree(rpmb); > return ret; > } > > static void mmc_blk_remove_rpmb_part(struct mmc_rpmb_data *rpmb) > + > { > cdev_device_del(&rpmb->chrdev, &rpmb->dev); > - device_del(&rpmb->dev); > - ida_simple_remove(&mmc_rpmb_ida, rpmb->id); > - kfree(rpmb); > + /* > + * Unless something is holding a reference to the device, this > + * drops the last reference and triggers the device to cleanup > + * and calls the .remove() callback. > + */ This comment seems redundant since it is just describing how reference counting works. > + put_device(&rpmb->dev); > } > > /* MMC Physical partitions consist of two boot partitions and > Also I noticed this function: static int mmc_rpmb_chrdev_release(struct inode *inode, struct file *filp) { struct mmc_rpmb_data *rpmb = container_of(inode->i_cdev, struct mmc_rpmb_data, chrdev); put_device(&rpmb->dev); mutex_lock(&open_lock); rpmb->md->usage--; mutex_unlock(&open_lock); return 0; } What is going on with 'usage'? It looks weird. What happens if you do this: open the rpmb device unbind the host controller try to use an ioctl close the rpmb device -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 3, 2017 at 10:32 AM, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 02/10/17 14:59, Linus Walleij wrote: >> +static void mmc_blk_rpmb_device_release(struct device *dev) >> +{ >> + struct mmc_rpmb_data *rpmb = dev_get_drvdata(dev); >> + >> + dev_info(dev, "removed\n"); > > Do we really need this message? Nah. More for symmetry since we print when we add partitions we also print when we remove them so it seemed consistent to me. >> + /* From this point, the .release() function cleans up the device */ >> + rpmb->dev.release = mmc_blk_rpmb_device_release; > > This should be before device_initialize(&rpmb->dev) then the error path > becomes just put_device(&rpmb->dev). The way I dealt with it in the GPIO subsystem was to not assign this pointer until later, and manage resources in the initilializer function (such as the error path) until that point. But I rewrote it like you say. > But further up is: > rpmb = kzalloc(sizeof(*rpmb), GFP_KERNEL); > if (!rpmb) > return -ENOMEM; > where it looks like you are leaking the ida. OMG you're right. Fixed it for v2. >> - device_del(&rpmb->dev); >> - ida_simple_remove(&mmc_rpmb_ida, rpmb->id); >> - kfree(rpmb); >> + /* >> + * Unless something is holding a reference to the device, this >> + * drops the last reference and triggers the device to cleanup >> + * and calls the .remove() callback. >> + */ > > This comment seems redundant since it is just describing how reference > counting works. OK I cut the crap. :) > Also I noticed this function: > > static int mmc_rpmb_chrdev_release(struct inode *inode, struct file *filp) > { > struct mmc_rpmb_data *rpmb = container_of(inode->i_cdev, > struct mmc_rpmb_data, chrdev); > > put_device(&rpmb->dev); > mutex_lock(&open_lock); > rpmb->md->usage--; > mutex_unlock(&open_lock); > > return 0; > } > > What is going on with 'usage'? It looks weird. It is the same reference counting as was present before the patch converting the block device to a character device. Not my invention. It's been there since the conception of the MMC stack. While the get_device()/put_device() is reference counting the RPMB device per se, this is refcounting the block device that is backing the RPMB char device, so that the block device cannot be removed if the character device is using it for RPMB access. > What happens if you do this: > open the rpmb device > unbind the host controller > try to use an ioctl > close the rpmb device It's analogous to the similar usecase I had in GPIO: What if you're holding (in userspace) a reference to a resource and the hardware backing the resource goes away? In GPIO I chose to "numb" the device so that any further ioctl()s would just be silently ignored but for RPMB I guess we should simply start returning errors. Since I'm still supporting the old refcounting with md->usage as described above, it should behave the same as the old codebase, which might not be very good behaviour but atleast it is not a regression. Posting the v2 soon. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/10/17 13:00, Linus Walleij wrote: > On Tue, Oct 3, 2017 at 10:32 AM, Adrian Hunter <adrian.hunter@intel.com> wrote: >> Also I noticed this function: >> >> static int mmc_rpmb_chrdev_release(struct inode *inode, struct file *filp) >> { >> struct mmc_rpmb_data *rpmb = container_of(inode->i_cdev, >> struct mmc_rpmb_data, chrdev); >> >> put_device(&rpmb->dev); >> mutex_lock(&open_lock); >> rpmb->md->usage--; >> mutex_unlock(&open_lock); >> >> return 0; >> } >> >> What is going on with 'usage'? It looks weird. > > It is the same reference counting as was present before the patch > converting the block device to a character device. Not my invention. > It's been there since the conception of the MMC stack. > > While the get_device()/put_device() is reference counting the > RPMB device per se, this is refcounting the block device that > is backing the RPMB char device, so that the block device > cannot be removed if the character device is using it for > RPMB access. > >> What happens if you do this: >> open the rpmb device >> unbind the host controller >> try to use an ioctl >> close the rpmb device > > It's analogous to the similar usecase I had in GPIO: > > What if you're holding (in userspace) a reference to a resource and > the hardware backing the resource goes away? > > In GPIO I chose to "numb" the device so that any further ioctl()s > would just be silently ignored but for RPMB I guess we should > simply start returning errors. > > Since I'm still supporting the old refcounting with md->usage > as described above, it should behave the same as the old > codebase, which might not be very good behaviour but atleast > it is not a regression. How do you know you are not dropping the last reference when you do md->usage-- ? -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 3, 2017 at 1:07 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 03/10/17 13:00, Linus Walleij wrote: >> Since I'm still supporting the old refcounting with md->usage >> as described above, it should behave the same as the old >> codebase, which might not be very good behaviour but atleast >> it is not a regression. > > How do you know you are not dropping the last reference when you do > md->usage-- ? Ah dammit what was I thinking... OK converting to mmc_blk_get() +mmc_blk_put() that should do it. Sorry for silly mistakes. :/ Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 3, 2017 at 11:57 PM, Avri Altman <Avri.Altman@wdc.com> wrote: > Multiple simultaneous accesses is forbidden. > So maybe direct recounting is better for controlling it? That is already handled by patches: commit 614f0388f580c436d2cf6dc0855de91d13ddc23d "mmc: block: move single ioctl() commands to block requests" commit 3ecd8cf23f88d5df1c545a5c04217987abb28575 "mmc: block: move multi-ioctl() to use block layer" Since all ioctl()s are not sent in through the block queue they get serialized by the very nature of how block requests work, i.e. one at a time. I have even tried to send RPMB commands from two consoles while a huge dd work was already working in the background. No problems with that. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Oct 4, 2017 at 8:14 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, Oct 3, 2017 at 11:57 PM, Avri Altman <Avri.Altman@wdc.com> wrote: > >> Multiple simultaneous accesses is forbidden. >> So maybe direct recounting is better for controlling it? > > That is already handled by patches: > > commit 614f0388f580c436d2cf6dc0855de91d13ddc23d > "mmc: block: move single ioctl() commands to block requests" > commit 3ecd8cf23f88d5df1c545a5c04217987abb28575 > "mmc: block: move multi-ioctl() to use block layer" > > Since all ioctl()s are not sent in through the block queue Since all ioctl()s are NOW sent in through the block queue > they get serialized by the very nature of how block requests > work, i.e. one at a time. I have even tried to send RPMB > commands from two consoles while a huge dd work was already > working in the background. No problems with that. Sorry for spellling mistakes. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c index c29dbcec7c61..2033a71bacc3 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -2329,6 +2329,14 @@ static const struct file_operations mmc_rpmb_fileops = { #endif }; +static void mmc_blk_rpmb_device_release(struct device *dev) +{ + struct mmc_rpmb_data *rpmb = dev_get_drvdata(dev); + + dev_info(dev, "removed\n"); + ida_simple_remove(&mmc_rpmb_ida, rpmb->id); + kfree(rpmb); +} static int mmc_blk_alloc_rpmb_part(struct mmc_card *card, struct mmc_blk_data *md, @@ -2371,6 +2379,8 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card, goto out_remove_ida; } + /* From this point, the .release() function cleans up the device */ + rpmb->dev.release = mmc_blk_rpmb_device_release; list_add(&rpmb->node, &md->rpmbs); string_get_size((u64)size, 512, STRING_UNITS_2, @@ -2384,17 +2394,22 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card, return 0; out_remove_ida: + put_device(&rpmb->dev); ida_simple_remove(&mmc_rpmb_ida, rpmb->id); kfree(rpmb); return ret; } static void mmc_blk_remove_rpmb_part(struct mmc_rpmb_data *rpmb) + { cdev_device_del(&rpmb->chrdev, &rpmb->dev); - device_del(&rpmb->dev); - ida_simple_remove(&mmc_rpmb_ida, rpmb->id); - kfree(rpmb); + /* + * Unless something is holding a reference to the device, this + * drops the last reference and triggers the device to cleanup + * and calls the .remove() callback. + */ + put_device(&rpmb->dev); } /* MMC Physical partitions consist of two boot partitions and
I forgot to account for the fact that the device core holds a reference to a device added with device_initialize() that need to be released with a corresponding put_device() to reach a 0 refcount at the end of the lifecycle. This led to a NULL pointer reference when freeing the device when e.g. unbidning the host device in sysfs. Fix this and use the device .release() callback to free the IDA and free:ing the memory used by the RPMB device. Before this patch: /sys/bus/amba/drivers/mmci-pl18x$ echo 80114000.sdi4_per2 > unbind [ 29.797332] mmc3: card 0001 removed [ 29.810791] Unable to handle kernel NULL pointer dereference at virtual address 00000050 [ 29.818878] pgd = de70c000 [ 29.821624] [00000050] *pgd=1e70a831, *pte=00000000, *ppte=00000000 [ 29.827911] Internal error: Oops: 17 [#1] PREEMPT SMP ARM [ 29.833282] Modules linked in: [ 29.836334] CPU: 1 PID: 154 Comm: sh Not tainted 4.14.0-rc3-00039-g83318e309566-dirty #736 [ 29.844604] Hardware name: ST-Ericsson Ux5x0 platform (Device Tree Support) [ 29.851562] task: de572700 task.stack: de742000 [ 29.856079] PC is at kernfs_find_ns+0x8/0x100 [ 29.860443] LR is at kernfs_find_and_get_ns+0x30/0x48 After this patch: /sys/bus/amba/drivers/mmci-pl18x$ echo 80005000.sdi4_per2 > unbind [ 20.623382] mmc3: card 0001 removed [ 20.627288] mmc_rpmb mmcblk3rpmb: removed Fixes: 0ab0c7c0e65a ("mmc: block: Convert RPMB to a character device") Reported-by: Adrian Hunter <adrian.hunter@intel.com> Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- drivers/mmc/core/block.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) -- 2.13.5 -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html