Re: [PATCH 1/2] mtd: use refcount to prevent corruption

From: Miquel Raynal
Date: Sat Jul 15 2023 - 11:41:27 EST

Next message: Zhang Shurong: "[PATCH] dmaengine: mv_xor: fix to avoid potential NULL pointer dereference"
Previous message: Konrad Dybcio: "[PATCH v2] media: camss: Intepret OF graph connections more sensibly"
In reply to: Fabrizio Castro: "RE: [PATCH 1/2] mtd: use refcount to prevent corruption"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Fabrizio,

fabrizio.castro.jz@xxxxxxxxxxx wrote on Fri, 14 Jul 2023 16:10:45 +0000:

> Dear All,
>
> I am sorry for reopening this topic, but as it turns out (after bisecting
> linux-next/master) this patch is interfering with a use case I am working
> on.
>
> I am using a Renesas RZ/V2M EVK v2.0 platform, I have an SPI NOR memory
> ("micron,mt25ql256a") wired up to a connector on the platform, the SPI
> master is using driver (built as module):
> drivers/spi/spi-rzv2m-csi.c
>
> Although the board device tree in mainline does not reflect the connection
> of CSI4 (which is the SPI master) from the SoC to the "micron,mt25ql256a"
> (SPI slave device), my local device tree comes with the necessary definitions.
>
> Without this patch, when I load up the module, I get the below 3 devices:
> /dev/mtd0
> /dev/mtd0ro
> /dev/mtdblock0
>
> They get cleaned up correctly upon module removal.
> I can reload the same module, and everything works just fine.
>
> With this patch applied, when I load up the module, I get the same 3
> devices:
> /dev/mtd0
> /dev/mtd0ro
> /dev/mtdblock0
>
> Upon removal, the below 2 devices still hang around:
> /dev/mtd0
> /dev/mtd0ro

Looks like the refcounting change is still not even in some cases, can
you investigate and come up with a proper patch? You can either improve
the existing patch or revert it and try your own approach if deemed
better.

Thanks,
Miquèl

> Preventing the module from being (re)loaded correctly:
> rzv2m_csi a4020200.spi: error -EBUSY: register controller failed
> rzv2m_csi: probe of a4020200.spi failed with error -16
>
> Are you guys aware of this sort of side effect?
>
> Thanks,
> Fab
>
> > From: Alexander Usyskin <alexander.usyskin@xxxxxxxxx>
> > Subject: [PATCH 1/2] mtd: use refcount to prevent corruption
> >
> > From: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> >
> > When underlying device is removed mtd core will crash
> > in case user space is holding open handle.
> > Need to use proper refcounting so device is release
> > only when has no users.
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler@xxxxxxxxx>
> > Signed-off-by: Alexander Usyskin <alexander.usyskin@xxxxxxxxx>
> > ---
> > drivers/mtd/mtdcore.c | 72 ++++++++++++++++++++++------------------
> > -
> > drivers/mtd/mtdcore.h | 1 +
> > drivers/mtd/mtdpart.c | 14 ++++----
> > include/linux/mtd/mtd.h | 2 +-
> > 4 files changed, 49 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> > index abf4cb58a8ab..84bd1878367d 100644
> > --- a/drivers/mtd/mtdcore.c
> > +++ b/drivers/mtd/mtdcore.c
> > @@ -93,10 +93,33 @@ static void mtd_release(struct device *dev)
> > struct mtd_info *mtd = dev_get_drvdata(dev);
> > dev_t index = MTD_DEVT(mtd->index);
> >
> > + if (mtd_is_partition(mtd))
> > + release_mtd_partition(mtd);
> > +
> > /* remove /dev/mtdXro node */
> > 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);
> > +
> > + debugfs_remove_recursive(mtd->dbg.dfs_dir);
> > +
> > + /* Try to remove the NVMEM provider */
> > + nvmem_unregister(mtd->nvmem);
> > +
> > + device_unregister(&mtd->dev);
> > +
> > + /* Clear dev so mtd can be safely re-registered later if desired
> > */
> > + memset(&mtd->dev, 0, sizeof(mtd->dev));
> > +
> > + idr_remove(&mtd_idr, mtd->index);
> > + of_node_put(mtd_get_of_node(mtd));
> > +
> > + module_put(THIS_MODULE);
> > +}
> > +
> > #define MTD_DEVICE_ATTR_RO(name) \
> > static DEVICE_ATTR(name, 0444, mtd_##name##_show, NULL)
> >
> > @@ -666,7 +689,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)
> > @@ -779,7 +802,6 @@ int del_mtd_device(struct mtd_info *mtd)
> > {
> > int ret;
> > struct mtd_notifier *not;
> > - struct device_node *mtd_of_node;
> >
> > mutex_lock(&mtd_table_mutex);
> >
> > @@ -793,28 +815,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 {
> > - mtd_of_node = mtd_get_of_node(mtd);
> > - debugfs_remove_recursive(mtd->dbg.dfs_dir);
> > -
> > - /* Try to remove the NVMEM provider */
> > - nvmem_unregister(mtd->nvmem);
> > -
> > - device_unregister(&mtd->dev);
> > -
> > - /* Clear dev so mtd can be safely re-registered later if
> > desired */
> > - memset(&mtd->dev, 0, sizeof(mtd->dev));
> > -
> > - idr_remove(&mtd_idr, mtd->index);
> > - of_node_put(mtd_of_node);
> > -
> > - module_put(THIS_MODULE);
> > - ret = 0;
> > - }
> > + kref_put(&mtd->refcnt, mtd_device_release);
> > + ret = 0;
> >
> > out_error:
> > mutex_unlock(&mtd_table_mutex);
> > @@ -1228,19 +1230,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++;
> > + if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER) || mtd-
> > >parent != master)
> > + kref_get(&mtd->parent->refcnt);
> > mtd = mtd->parent;
> > }
> >
> > @@ -1327,18 +1331,20 @@ void __put_mtd_device(struct mtd_info *mtd)
> > {
> > struct mtd_info *master = mtd_get_master(mtd);
> >
> > - while (mtd->parent) {
> > - --mtd->usecount;
> > - BUG_ON(mtd->usecount < 0);
> > - mtd = mtd->parent;
> > - }
> > + while (mtd != master) {
> > + struct mtd_info *parent = mtd->parent;
> >
> > - master->usecount--;
> > + kref_put(&mtd->refcnt, mtd_device_release);
> > + mtd = parent;
> > + }
> >
> > if (master->_put_device)
> > master->_put_device(master);
> >
> > module_put(master->owner);
> > +
> > + if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
> > + kref_put(&master->refcnt, mtd_device_release);
> > }
> > EXPORT_SYMBOL_GPL(__put_mtd_device);
> >
> > 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 a46affbb037d..23483db8f30c 100644
> > --- a/drivers/mtd/mtdpart.c
> > +++ b/drivers/mtd/mtdpart.c
> > @@ -32,6 +32,12 @@ static inline void free_partition(struct mtd_info
> > *mtd)
> > kfree(mtd);
> > }
> >
> > +void release_mtd_partition(struct mtd_info *mtd)
> > +{
> > + WARN_ON(!list_empty(&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)
> > @@ -309,13 +315,11 @@ static int __mtd_del_partition(struct mtd_info
> > *mtd)
> >
> > sysfs_remove_files(&mtd->dev.kobj, mtd_partition_attrs);
> >
> > + list_del_init(&mtd->part.node);
> > err = del_mtd_device(mtd);
> > if (err)
> > return err;
> >
> > - list_del(&mtd->part.node);
> > - free_partition(mtd);
> > -
> > return 0;
> > }
> >
> > @@ -333,6 +337,7 @@ static int __del_mtd_partitions(struct mtd_info
> > *mtd)
> > __del_mtd_partitions(child);
> >
> > pr_info("Deleting %s MTD partition\n", child->name);
> > + list_del_init(&child->part.node);
> > ret = del_mtd_device(child);
> > if (ret < 0) {
> > pr_err("Error when deleting partition \"%s\" (%d)\n",
> > @@ -340,9 +345,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 7c58c44662b8..914a9f974baa 100644
> > --- a/include/linux/mtd/mtd.h
> > +++ b/include/linux/mtd/mtd.h
> > @@ -379,7 +379,7 @@ struct mtd_info {
> >
> > struct module *owner;
> > struct device dev;
> > - int usecount;
> > + struct kref refcnt;
> > struct mtd_debug_info dbg;
> > struct nvmem_device *nvmem;
> > struct nvmem_device *otp_user_nvmem;
> > --
> > 2.34.1
>

Next message: Zhang Shurong: "[PATCH] dmaengine: mv_xor: fix to avoid potential NULL pointer dereference"
Previous message: Konrad Dybcio: "[PATCH v2] media: camss: Intepret OF graph connections more sensibly"
In reply to: Fabrizio Castro: "RE: [PATCH 1/2] mtd: use refcount to prevent corruption"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]