| Message ID | 20210127200319.662842-1-tomas.winkler@intel.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | mtd: use refcount to prevent corruption | expand |
Tomas, ----- Ursprüngliche Mail ----- > Von: "Tomas Winkler" <tomas.winkler@intel.com> > An: "Miquel Raynal" <miquel.raynal@bootlin.com>, "richard" <richard@nod.at>, "Vignesh Raghavendra" <vigneshr@ti.com>, > "linux-mtd" <linux-mtd@lists.infradead.org>, "linux-kernel" <linux-kernel@vger.kernel.org> > CC: "Tomas Winkler" <tomas.winkler@intel.com> > Gesendet: Mittwoch, 27. Januar 2021 21:03:19 > Betreff: [PATCH] mtd: use refcount to prevent corruption > When underlying device is removed mtd core will crash > in case user space is still holding an open handle to a mtd device node. > A proper refcounting is needed so device is release only when a > partition has no active users. The current simple counter is not > sufficient. Can you please explain a little more what devices are involved? Does it implement _get_device() and _put_device()? Thanks, //richard
> Subject: Re: [PATCH] mtd: use refcount to prevent corruption > > Tomas, > > ----- Ursprüngliche Mail ----- > > Von: "Tomas Winkler" <tomas.winkler@intel.com> > > An: "Miquel Raynal" <miquel.raynal@bootlin.com>, "richard" > > <richard@nod.at>, "Vignesh Raghavendra" <vigneshr@ti.com>, "linux-mtd" > > <linux-mtd@lists.infradead.org>, "linux-kernel" > > <linux-kernel@vger.kernel.org> > > CC: "Tomas Winkler" <tomas.winkler@intel.com> > > Gesendet: Mittwoch, 27. Januar 2021 21:03:19 > > Betreff: [PATCH] mtd: use refcount to prevent corruption > > > When underlying device is removed mtd core will crash in case user > > space is still holding an open handle to a mtd device node. > > A proper refcounting is needed so device is release only when a > > partition has no active users. The current simple counter is not > > sufficient. > > Can you please explain a little more what devices are involved? > Does it implement _get_device() and _put_device()? No this is not connected to those handlers of the underlying device and those won't help. I have a spi device provided by MFD framework so it can go away anytime. My solution tries to replace the current simple partition reference counting. In previous solution it will return -EBUSY on partition that is held but will remove the actual parent device, leading to crash. Also w/o reference counting there is no process to actually remove the partition that was previously busy. Thanks Tomas > > Thanks, > //richard
----- Ursprüngliche Mail ----- >> > When underlying device is removed mtd core will crash in case user >> > space is still holding an open handle to a mtd device node. >> > A proper refcounting is needed so device is release only when a >> > partition has no active users. The current simple counter is not >> > sufficient. >> >> Can you please explain a little more what devices are involved? >> Does it implement _get_device() and _put_device()? > No this is not connected to those handlers of the underlying device and those > won't help. > I have a spi device provided by MFD framework so it can go away anytime. Can it go away physically or just in software? Usually the pattern is that you make sure in the device driver that nobody can orphan the MTD while it is in use. e.g. drivers/mtd/ubi/gluebi.c does so. In _get_device() it grabs a reference on the underlying UBI volume to make sure it cannot go away while the MTD (on top of UBI) is in use. Thanks, //richard
> > ----- Ursprüngliche Mail ----- > >> > When underlying device is removed mtd core will crash in case user > >> > space is still holding an open handle to a mtd device node. > >> > A proper refcounting is needed so device is release only when a > >> > partition has no active users. The current simple counter is not > >> > sufficient. > >> > >> Can you please explain a little more what devices are involved? > >> Does it implement _get_device() and _put_device()? > > No this is not connected to those handlers of the underlying device > > and those won't help. > > I have a spi device provided by MFD framework so it can go away anytime. > > Can it go away physically or just in software? Software, but since this is mfd it's basically hotplug. The kernel is crashing when I simulate hardware failure. > > Usually the pattern is that you make sure in the device driver that nobody can > orphan the MTD while it is in use. > e.g. drivers/mtd/ubi/gluebi.c does so. In _get_device() it grabs a reference on > the underlying UBI volume to make sure it cannot go away while the MTD (on > top of UBI) is in use. I can try that if it helps, because we are simulating possible lower level crash. In an case I believe that the proper refcouting is much more robust solution, than the current one. I'd appreciate if someone can review the actual implementation. Thanks Tomas
Tomas, ----- Ursprüngliche Mail ----- >> >> Can you please explain a little more what devices are involved? >> >> Does it implement _get_device() and _put_device()? >> > No this is not connected to those handlers of the underlying device >> > and those won't help. >> > I have a spi device provided by MFD framework so it can go away anytime. >> >> Can it go away physically or just in software? > Software, but since this is mfd it's basically hotplug. The kernel is crashing > when I simulate hardware failure. >> >> Usually the pattern is that you make sure in the device driver that nobody can >> orphan the MTD while it is in use. >> e.g. drivers/mtd/ubi/gluebi.c does so. In _get_device() it grabs a reference on >> the underlying UBI volume to make sure it cannot go away while the MTD (on >> top of UBI) is in use. > > I can try that if it helps, because we are simulating possible lower level > crash. > In an case I believe that the proper refcouting is much more robust solution, > than the current one. > I'd appreciate if someone can review the actual implementation. This happens right now, I try to understand why exactly the current way is not good in enough. :-) Your approach makes sure that the MTD itself does not go away while it has users but how does this help in the case where the underlying MFD just vanishes? The MTD can be in use and the MFD can go away while e.g. mtd_read() or such takes place. Thanks, //richard
> Tomas, > > ----- Ursprüngliche Mail ----- > >> >> Can you please explain a little more what devices are involved? > >> >> Does it implement _get_device() and _put_device()? > >> > No this is not connected to those handlers of the underlying device > >> > and those won't help. > >> > I have a spi device provided by MFD framework so it can go away anytime. > >> > >> Can it go away physically or just in software? > > Software, but since this is mfd it's basically hotplug. The kernel is > > crashing when I simulate hardware failure. > >> > >> Usually the pattern is that you make sure in the device driver that > >> nobody can orphan the MTD while it is in use. > >> e.g. drivers/mtd/ubi/gluebi.c does so. In _get_device() it grabs a > >> reference on the underlying UBI volume to make sure it cannot go away > >> while the MTD (on top of UBI) is in use. > > > > I can try that if it helps, because we are simulating possible lower > > level crash. > > In an case I believe that the proper refcouting is much more robust > > solution, than the current one. > > I'd appreciate if someone can review the actual implementation. > > This happens right now, I try to understand why exactly the current way is not > good in enough. :-) > > Your approach makes sure that the MTD itself does not go away while it has > users but how does this help in the case where the underlying MFD just > vanishes? > The MTD can be in use and the MFD can go away while e.g. mtd_read() or such > takes place. Read will fail, but kernel won't crash on access to memory that was freed. Thanks Tomas
Hi Tomas, "Winkler, Tomas" <tomas.winkler@intel.com> wrote on Thu, 28 Jan 2021 08:53:43 +0000: > > Tomas, > > > > ----- Ursprüngliche Mail ----- > > >> >> Can you please explain a little more what devices are involved? > > >> >> Does it implement _get_device() and _put_device()? > > >> > No this is not connected to those handlers of the underlying device > > >> > and those won't help. > > >> > I have a spi device provided by MFD framework so it can go away anytime. > > >> > > >> Can it go away physically or just in software? > > > Software, but since this is mfd it's basically hotplug. The kernel is > > > crashing when I simulate hardware failure. > > >> > > >> Usually the pattern is that you make sure in the device driver that > > >> nobody can orphan the MTD while it is in use. > > >> e.g. drivers/mtd/ubi/gluebi.c does so. In _get_device() it grabs a > > >> reference on the underlying UBI volume to make sure it cannot go away > > >> while the MTD (on top of UBI) is in use. > > > > > > I can try that if it helps, because we are simulating possible lower > > > level crash. > > > In an case I believe that the proper refcouting is much more robust > > > solution, than the current one. > > > I'd appreciate if someone can review the actual implementation. > > > > This happens right now, I try to understand why exactly the current way is not > > good in enough. :-) > > > > Your approach makes sure that the MTD itself does not go away while it has > > users but how does this help in the case where the underlying MFD just > > vanishes? > > The MTD can be in use and the MFD can go away while e.g. mtd_read() or such > > takes place. > > Read will fail, but kernel won't crash on access to memory that was freed. As Richard was saying, we are really open to enhance MTD refcounting. However, the issue you are facing is, IMHO, not related to MTD but to MFD. There should be a way to avoid MFD to vanish by taking a reference of it through mtd->_get_device(). I don't think addressing the case where MFD vanishes while MTD (as a user) is still active is the right approach. Thanks, Miquèl
> Hi Tomas, > > "Winkler, Tomas" <tomas.winkler@intel.com> wrote on Thu, 28 Jan 2021 > 08:53:43 +0000: > > > > Tomas, > > > > > > ----- Ursprüngliche Mail ----- > > > >> >> Can you please explain a little more what devices are involved? > > > >> >> Does it implement _get_device() and _put_device()? > > > >> > No this is not connected to those handlers of the underlying > > > >> > device and those won't help. > > > >> > I have a spi device provided by MFD framework so it can go away > anytime. > > > >> > > > >> Can it go away physically or just in software? > > > > Software, but since this is mfd it's basically hotplug. The kernel > > > > is crashing when I simulate hardware failure. > > > >> > > > >> Usually the pattern is that you make sure in the device driver > > > >> that nobody can orphan the MTD while it is in use. > > > >> e.g. drivers/mtd/ubi/gluebi.c does so. In _get_device() it grabs > > > >> a reference on the underlying UBI volume to make sure it cannot > > > >> go away while the MTD (on top of UBI) is in use. > > > > > > > > I can try that if it helps, because we are simulating possible > > > > lower level crash. > > > > In an case I believe that the proper refcouting is much more > > > > robust solution, than the current one. > > > > I'd appreciate if someone can review the actual implementation. > > > > > > This happens right now, I try to understand why exactly the current > > > way is not good in enough. :-) > > > > > > Your approach makes sure that the MTD itself does not go away while > > > it has users but how does this help in the case where the underlying > > > MFD just vanishes? > > > The MTD can be in use and the MFD can go away while e.g. mtd_read() > > > or such takes place. > > > > Read will fail, but kernel won't crash on access to memory that was freed. > > As Richard was saying, we are really open to enhance MTD refcounting. > > However, the issue you are facing is, IMHO, not related to MTD but to MFD. > There should be a way to avoid MFD to vanish by taking a reference of it > through mtd->_get_device(). I don't think addressing the case where MFD > vanishes while MTD (as a user) is still active is the right approach. I think it won't work because MFD sub-driver remove() is called and it must succeed because the main device is not accessible unlike glueubi which just returns -EBUSY. so we postpone the mtd unregister to mtd_info->_put_device() but it that state we have nothing to hold on as the device is gone in remove() User will fail anyway, as the underlying device is not functional in that state. Anyway I've tried your suggestion, the kernel is crashing, hope I haven't done some silly bug. Thanks Tomas
Tomas, ----- Ursprüngliche Mail ----- >> As Richard was saying, we are really open to enhance MTD refcounting. >> >> However, the issue you are facing is, IMHO, not related to MTD but to MFD. >> There should be a way to avoid MFD to vanish by taking a reference of it >> through mtd->_get_device(). I don't think addressing the case where MFD >> vanishes while MTD (as a user) is still active is the right approach. > > I think it won't work because MFD sub-driver remove() is called and it must > succeed because the main device is not accessible unlike glueubi which just > returns -EBUSY. Well, the trick in glubi (and other MTDs with "hotplug" support) is not to reject removal of the sub-device. ->_put_device() is of return type void. The key is grabbing a reference on the sub-device in ->_get_device() such that the layer below doesn't even try to remove while the MTD is in use. > so we postpone the mtd unregister to mtd_info->_put_device() but it that state > we have nothing to hold > on as the device is gone in remove() > User will fail anyway, as the underlying device is not functional in that state. > Anyway I've tried your suggestion, the kernel is crashing, hope I haven't done > some silly bug. Can you point us to the affected code? This would help a lot to understand the issue better. I'm sure we can find a solution. Thanks, //richard
> > Tomas, > > ----- Ursprüngliche Mail ----- > >> As Richard was saying, we are really open to enhance MTD refcounting. > >> > >> However, the issue you are facing is, IMHO, not related to MTD but to MFD. > >> There should be a way to avoid MFD to vanish by taking a reference of > >> it through mtd->_get_device(). I don't think addressing the case > >> where MFD vanishes while MTD (as a user) is still active is the right > approach. > > > > I think it won't work because MFD sub-driver remove() is called and it must > > succeed because the main device is not accessible unlike glueubi > > which just returns -EBUSY. > > Well, the trick in glubi (and other MTDs with "hotplug" support) is not to reject > removal of the sub-device. ->_put_device() is of return type void. > The key is grabbing a reference on the sub-device in ->_get_device() such that > the layer below doesn't even try to remove while the MTD is in use. > > > so we postpone the mtd unregister to mtd_info->_put_device() but it > > that state we have nothing to hold on as the device is gone in > > remove() User will fail anyway, as the underlying device is not > > functional in that state. > > Anyway I've tried your suggestion, the kernel is crashing, hope I > > haven't done some silly bug. > > Can you point us to the affected code? > This would help a lot to understand the issue better. Below is approximately the snippet > I'm sure we can find a solution. Okay though I've already found a working solution. struct my_spi { struct mtd_info mtd; struct kref refcnt; bool init; }; static int my_spi_get_mtd(struct mtd_info *mtd) { struct my_spi *spi = container_of(mtd, struct my_spi, mtd); if (!spi->init) return -ENODEV; kref_get(&spi->refcnt); spi->init = true; return 0; } static void my_spi_unregister_mtd(struct kref *kref) { struct my_spi *spi = container_of(kref, struct my_spi, refcnt); mtd_device_unregister(&spi->mtd); kfree(spi); } static int my_spi_put_mtd(struct mtd_info *mtd) { struct my_spi *spi = container_of(mtd, struct my_spi, mtd); kref_put(&spi->refcnt, my_spi_unregister_mtd); return 0; } static int my_spi_init_mtd(static my_spi *spi) { ... spi->mtd._get_device = my_spi_get_mtd; spi->mtd._get_device = my_spi_put_mtd; kref_init(&spi->refcnt); ... } static int my_spi_probe(struct platform_device *platdev) { ... spi = kzalloc(size, GFP_KERNEL); spi->init = true; my_spi_init_mtd(spi); platform_set_drvdata(platdev, spi); ... return 0; } static int my_spi_remove(struct platform_device *platdev) { struct my_spi *spi = platform_get_drvdata(platdev); spi->init = false; my_spi_put_mtd(&spi->mtd); platform_set_drvdata(platdev, NULL); }
> > Tomas, > > ----- Ursprüngliche Mail ----- > >> As Richard was saying, we are really open to enhance MTD refcounting. > >> > >> However, the issue you are facing is, IMHO, not related to MTD but to > MFD. > >> There should be a way to avoid MFD to vanish by taking a reference of > >> it through mtd->_get_device(). I don't think addressing the case > >> where MFD vanishes while MTD (as a user) is still active is the right > approach. > > > > I think it won't work because MFD sub-driver remove() is called and it > must > > succeed because the main device is not accessible unlike glueubi > > which just returns -EBUSY. > > Well, the trick in glubi (and other MTDs with "hotplug" support) is not to > reject removal of the sub-device. ->_put_device() is of return type void. > The key is grabbing a reference on the sub-device in ->_get_device() such > that the layer below doesn't even try to remove while the MTD is in use. I understand that. But in that case the kernel is in the mercy of user space to close the handle, the whole perception here is that of hotplug that the device is physically removed it cannot wait for the user space to complete. What's the fix is trying to do is to bail out gracefully. > > so we postpone the mtd unregister to mtd_info->_put_device() but it > > that state we have nothing to hold on as the device is gone in > > remove() User will fail anyway, as the underlying device is not > > functional in that state. > > Anyway I've tried your suggestion, the kernel is crashing, hope I > > haven't done some silly bug. > > Can you point us to the affected code? > This would help a lot to understand the issue better. > I'm sure we can find a solution. Got green light on releasing the patches will send soon. Thanks Tomas
On Sat, Feb 13, 2021 at 6:12 PM Winkler, Tomas <tomas.winkler@intel.com> wrote: > > Well, the trick in glubi (and other MTDs with "hotplug" support) is not to > > reject removal of the sub-device. ->_put_device() is of return type void. > > The key is grabbing a reference on the sub-device in ->_get_device() such > > that the layer below doesn't even try to remove while the MTD is in use. > > I understand that. But in that case the kernel is in the mercy of user space to close the handle, > the whole perception here is that of hotplug that the device is physically removed it cannot wait > for the user space to complete. What's the fix is trying to do is to bail out gracefully. > > > > so we postpone the mtd unregister to mtd_info->_put_device() but it > > > that state we have nothing to hold on as the device is gone in > > > remove() User will fail anyway, as the underlying device is not > > > functional in that state. > > > Anyway I've tried your suggestion, the kernel is crashing, hope I > > > haven't done some silly bug. > > > > Can you point us to the affected code? > > This would help a lot to understand the issue better. > > I'm sure we can find a solution. > > Got green light on releasing the patches will send soon. I'm eager to see them. :-) As said, I'm sure we can find a nice solution.
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 2d6423d89a17..db5167eacaa4 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -93,9 +93,29 @@ static void mtd_release(struct device *dev) dev_t index = MTD_DEVT(mtd->index); /* remove /dev/mtdXro node */ + if (mtd_is_partition(mtd)) + release_mtd_partition(mtd); + device_destroy(&mtd_class, index + 1); } +static void mtd_device_release(struct kref *kref) +{ + struct mtd_info *mtd = container_of(kref, struct mtd_info, refcnt); + + if (mtd->nvmem) { + nvmem_unregister(mtd->nvmem); + mtd->nvmem = NULL; + } + + idr_remove(&mtd_idr, mtd->index); + of_node_put(mtd_get_of_node(mtd)); + + device_unregister(&mtd->dev); + + module_put(THIS_MODULE); +} + static ssize_t mtd_type_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -619,7 +639,7 @@ int add_mtd_device(struct mtd_info *mtd) } mtd->index = i; - mtd->usecount = 0; + kref_init(&mtd->refcnt); /* default value if not set by driver */ if (mtd->bitflip_threshold == 0) @@ -733,23 +753,8 @@ int del_mtd_device(struct mtd_info *mtd) list_for_each_entry(not, &mtd_notifiers, list) not->remove(mtd); - if (mtd->usecount) { - printk(KERN_NOTICE "Removing MTD device #%d (%s) with use count %d\n", - mtd->index, mtd->name, mtd->usecount); - ret = -EBUSY; - } else { - /* Try to remove the NVMEM provider */ - if (mtd->nvmem) - nvmem_unregister(mtd->nvmem); - - device_unregister(&mtd->dev); - - idr_remove(&mtd_idr, mtd->index); - of_node_put(mtd_get_of_node(mtd)); - - module_put(THIS_MODULE); - ret = 0; - } + kref_put(&mtd->refcnt, mtd_device_release); + ret = 0; out_error: mutex_unlock(&mtd_table_mutex); @@ -984,20 +989,21 @@ int __get_mtd_device(struct mtd_info *mtd) if (!try_module_get(master->owner)) return -ENODEV; + kref_get(&mtd->refcnt); + if (master->_get_device) { err = master->_get_device(mtd); if (err) { + kref_put(&mtd->refcnt, mtd_device_release); module_put(master->owner); return err; } } - master->usecount++; - while (mtd->parent) { - mtd->usecount++; mtd = mtd->parent; + kref_get(&mtd->refcnt); } return 0; @@ -1055,14 +1061,13 @@ void __put_mtd_device(struct mtd_info *mtd) { struct mtd_info *master = mtd_get_master(mtd); + kref_put(&mtd->refcnt, mtd_device_release); + while (mtd->parent) { - --mtd->usecount; - BUG_ON(mtd->usecount < 0); mtd = mtd->parent; + kref_put(&mtd->refcnt, mtd_device_release); } - master->usecount--; - if (master->_put_device) master->_put_device(master); diff --git a/drivers/mtd/mtdcore.h b/drivers/mtd/mtdcore.h index b5eefeabf310..b014861a06a6 100644 --- a/drivers/mtd/mtdcore.h +++ b/drivers/mtd/mtdcore.h @@ -12,6 +12,7 @@ int __must_check add_mtd_device(struct mtd_info *mtd); int del_mtd_device(struct mtd_info *mtd); int add_mtd_partitions(struct mtd_info *, const struct mtd_partition *, int); int del_mtd_partitions(struct mtd_info *); +void release_mtd_partition(struct mtd_info *mtd); struct mtd_partitions; diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c index 12ca4f19cb14..8175f6d9c790 100644 --- a/drivers/mtd/mtdpart.c +++ b/drivers/mtd/mtdpart.c @@ -31,6 +31,12 @@ static inline void free_partition(struct mtd_info *mtd) kfree(mtd); } +void release_mtd_partition(struct mtd_info *mtd) +{ + list_del_init(&mtd->part.node); + free_partition(mtd); +} + static struct mtd_info *allocate_partition(struct mtd_info *parent, const struct mtd_partition *part, int partno, uint64_t cur_offset) @@ -313,9 +319,6 @@ static int __mtd_del_partition(struct mtd_info *mtd) if (err) return err; - list_del(&child->part.node); - free_partition(mtd); - return 0; } @@ -341,9 +344,6 @@ static int __del_mtd_partitions(struct mtd_info *mtd) err = ret; continue; } - - list_del(&child->part.node); - free_partition(child); } return err; diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index 157357ec1441..1217c9d8d69d 100644 --- a/include/linux/mtd/mtd.h +++ b/include/linux/mtd/mtd.h @@ -373,7 +373,7 @@ struct mtd_info { struct module *owner; struct device dev; - int usecount; + struct kref refcnt; struct mtd_debug_info dbg; struct nvmem_device *nvmem;
When underlying device is removed mtd core will crash in case user space is still holding an open handle to a mtd device node. A proper refcounting is needed so device is release only when a partition has no active users. The current simple counter is not sufficient. Signed-off-by: Tomas Winkler <tomas.winkler@intel.com> --- drivers/mtd/mtdcore.c | 55 ++++++++++++++++++++++------------------- drivers/mtd/mtdcore.h | 1 + drivers/mtd/mtdpart.c | 12 ++++----- include/linux/mtd/mtd.h | 2 +- 4 files changed, 38 insertions(+), 32 deletions(-)