| Message ID | 20171003135123.4934-1-linus.walleij@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/10/17 16:51, 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 > > 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> > --- > ChangeLog v2->v3: > - Fix the block device reference counting: instead of manipulating > md->usage directly, call the proper refcounting functions > mmc_blk_get() and mmc_blk_put(). > ChangeLog v1->v2: > - Drop the print from removing the RPMB partition > - Refactor the error path of the RPMB to use refcounting > - Drop the verbose comment on refcounts > --- > drivers/mmc/core/block.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > > diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c > index c29dbcec7c61..46ecbaec8035 100644 > --- a/drivers/mmc/core/block.c > +++ b/drivers/mmc/core/block.c > @@ -2299,7 +2299,7 @@ static int mmc_rpmb_chrdev_open(struct inode *inode, struct file *filp) > get_device(&rpmb->dev); > filp->private_data = rpmb; > mutex_lock(&open_lock); > - rpmb->md->usage++; > + mmc_blk_get(rpmb->md->disk); mmc_blk_get() also tries to mutex_lock(&open_lock) But how do you know md still exists? Looks like you need to make the disk the parent of the rpmb device and then use the disk to get a reference to md. Another thing that looks odd: mmc_blk_ioctl() checks for capable(CAP_SYS_RAWIO) but mmc_rpmb_ioctl() does not. Is that intentional? > mutex_unlock(&open_lock); > > return nonseekable_open(inode, filp); > @@ -2312,7 +2312,7 @@ static int mmc_rpmb_chrdev_release(struct inode *inode, struct file *filp) > > put_device(&rpmb->dev); > mutex_lock(&open_lock); > - rpmb->md->usage--; > + mmc_blk_put(rpmb->md); > mutex_unlock(&open_lock); > > return 0; > @@ -2329,6 +2329,13 @@ 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); > + > + 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, > @@ -2347,8 +2354,10 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card, > return devidx; > > rpmb = kzalloc(sizeof(*rpmb), GFP_KERNEL); > - if (!rpmb) > + if (!rpmb) { > + ida_simple_remove(&mmc_rpmb_ida, devidx); > return -ENOMEM; > + } > > snprintf(rpmb_name, sizeof(rpmb_name), > "mmcblk%u%s", card->host->index, subname ? subname : ""); > @@ -2359,6 +2368,7 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card, > rpmb->dev.bus = &mmc_rpmb_bus_type; > rpmb->dev.devt = MKDEV(MAJOR(mmc_rpmb_devt), rpmb->id); > rpmb->dev.parent = &card->dev; > + rpmb->dev.release = mmc_blk_rpmb_device_release; > device_initialize(&rpmb->dev); > dev_set_drvdata(&rpmb->dev, rpmb); > rpmb->md = md; > @@ -2368,7 +2378,7 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card, > ret = cdev_device_add(&rpmb->chrdev, &rpmb->dev); > if (ret) { > pr_err("%s: could not add character device\n", rpmb_name); > - goto out_remove_ida; > + goto out_put_device; > } > > list_add(&rpmb->node, &md->rpmbs); > @@ -2383,18 +2393,16 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card, > > return 0; > > -out_remove_ida: > - ida_simple_remove(&mmc_rpmb_ida, rpmb->id); > - kfree(rpmb); > +out_put_device: > + put_device(&rpmb->dev); > 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); > + put_device(&rpmb->dev); > } > > /* MMC Physical partitions consist of two boot partitions and > -- 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 9:39 AM, Adrian Hunter <adrian.hunter@intel.com> wrote: >> @@ -2299,7 +2299,7 @@ static int mmc_rpmb_chrdev_open(struct inode *inode, struct file *filp) >> get_device(&rpmb->dev); >> filp->private_data = rpmb; >> mutex_lock(&open_lock); >> - rpmb->md->usage++; >> + mmc_blk_get(rpmb->md->disk); > > mmc_blk_get() also tries to mutex_lock(&open_lock) Yeah :/ I fix. Sorry for this flunky (too)... > But how do you know md still exists? I was thinking that the reference counting is exactly for making sure it exists across open()/close()? > Looks like you need > to make the disk the parent of the rpmb device and then > use the disk to get a reference to md. That sounds like it can be done relatively easily. Sounds like it should be a separate patch though, let me iterate this one first so we plug the biggest holes and then I try to come up with something for that too. 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 04/10/17 12:09, Linus Walleij wrote: > On Wed, Oct 4, 2017 at 9:39 AM, Adrian Hunter <adrian.hunter@intel.com> wrote: > >>> @@ -2299,7 +2299,7 @@ static int mmc_rpmb_chrdev_open(struct inode *inode, struct file *filp) >>> get_device(&rpmb->dev); >>> filp->private_data = rpmb; >>> mutex_lock(&open_lock); >>> - rpmb->md->usage++; >>> + mmc_blk_get(rpmb->md->disk); >> >> mmc_blk_get() also tries to mutex_lock(&open_lock) > > Yeah :/ I fix. > Sorry for this flunky (too)... > >> But how do you know md still exists? > > I was thinking that the reference counting is exactly for making sure > it exists across open()/close()? Previously the device being opened was the block device which had to exist. But I think it is OK to assume md exists in mmc_rpmb_chrdev_open() because rpmb->chrdev is removed before the main disk. > >> Looks like you need >> to make the disk the parent of the rpmb device and then >> use the disk to get a reference to md. > > That sounds like it can be done relatively easily. Sounds like it should be a > separate patch though, let me iterate this one first so we plug the biggest > holes and then I try to come up with something for that too. OK. What about the "capable(CAP_SYS_RAWIO)" question? -- 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 1:58 PM, Adrian Hunter <adrian.hunter@intel.com> wrote: > On 04/10/17 12:09, Linus Walleij wrote: >>> But how do you know md still exists? >> >> I was thinking that the reference counting is exactly for making sure >> it exists across open()/close()? > > Previously the device being opened was the block device which had to exist. > > But I think it is OK to assume md exists in mmc_rpmb_chrdev_open() because > rpmb->chrdev is removed before the main disk. That's my thought too. But thinking it over there could be a race if e.g. you write a script with a loop hammering the device in/out with bind/unbind at the same time opening and closing the RPMB chardev and holding a handle to it. It is not a very realistic usecase but I will try to provoke it anyways. > OK. What about the "capable(CAP_SYS_RAWIO)" question? The flag has a very strange definition in include/uapi/linux/capability.h: /* Allow sending USB messages to any device via /dev/bus/usb */ #define CAP_SYS_RAWIO 17 So I read in the codebase how this flag is actuallt used. It seems to universally apply for cases where userspace is allowed to read or write directly into device registers. So I dropped that since the character device is only able to do ioctl()s, no raw I/O. RPMB is and always was strictly ioctl-based. We should simply plug the hole and not letting people shoot themselves in the foot with this. I think it is bogus that we have it even for the block device, but it may be argued that the stuff returned from the ioctl reading card info is "kind of" RAWIO. I guess that is why it's there. But we are using well structured ioctls so it is really irrelevant. The flag is not needed or used for anything (the subsystems using it is checking for it explicitly, it is not checked in the core fs code or anything), we could just drop it from the block devices too. 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..46ecbaec8035 100644 --- a/drivers/mmc/core/block.c +++ b/drivers/mmc/core/block.c @@ -2299,7 +2299,7 @@ static int mmc_rpmb_chrdev_open(struct inode *inode, struct file *filp) get_device(&rpmb->dev); filp->private_data = rpmb; mutex_lock(&open_lock); - rpmb->md->usage++; + mmc_blk_get(rpmb->md->disk); mutex_unlock(&open_lock); return nonseekable_open(inode, filp); @@ -2312,7 +2312,7 @@ static int mmc_rpmb_chrdev_release(struct inode *inode, struct file *filp) put_device(&rpmb->dev); mutex_lock(&open_lock); - rpmb->md->usage--; + mmc_blk_put(rpmb->md); mutex_unlock(&open_lock); return 0; @@ -2329,6 +2329,13 @@ 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); + + 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, @@ -2347,8 +2354,10 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card, return devidx; rpmb = kzalloc(sizeof(*rpmb), GFP_KERNEL); - if (!rpmb) + if (!rpmb) { + ida_simple_remove(&mmc_rpmb_ida, devidx); return -ENOMEM; + } snprintf(rpmb_name, sizeof(rpmb_name), "mmcblk%u%s", card->host->index, subname ? subname : ""); @@ -2359,6 +2368,7 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card, rpmb->dev.bus = &mmc_rpmb_bus_type; rpmb->dev.devt = MKDEV(MAJOR(mmc_rpmb_devt), rpmb->id); rpmb->dev.parent = &card->dev; + rpmb->dev.release = mmc_blk_rpmb_device_release; device_initialize(&rpmb->dev); dev_set_drvdata(&rpmb->dev, rpmb); rpmb->md = md; @@ -2368,7 +2378,7 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card, ret = cdev_device_add(&rpmb->chrdev, &rpmb->dev); if (ret) { pr_err("%s: could not add character device\n", rpmb_name); - goto out_remove_ida; + goto out_put_device; } list_add(&rpmb->node, &md->rpmbs); @@ -2383,18 +2393,16 @@ static int mmc_blk_alloc_rpmb_part(struct mmc_card *card, return 0; -out_remove_ida: - ida_simple_remove(&mmc_rpmb_ida, rpmb->id); - kfree(rpmb); +out_put_device: + put_device(&rpmb->dev); 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); + 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 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> --- ChangeLog v2->v3: - Fix the block device reference counting: instead of manipulating md->usage directly, call the proper refcounting functions mmc_blk_get() and mmc_blk_put(). ChangeLog v1->v2: - Drop the print from removing the RPMB partition - Refactor the error path of the RPMB to use refcounting - Drop the verbose comment on refcounts --- drivers/mmc/core/block.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)