| Message ID | 20230731082029.762385-1-miquel.raynal@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | mtd: Fix refcounting with MTD_PARTITIONED_MASTER | expand |
> > The logic is way too convoluted, let's clean the kref_get/put section to > clarify what this block does, hopefully solving the refcounting issue > when using CONFIG_MTD_PARTITIONED_MASTER at the same time: > - Iterate through all the parent mtd devices > - Grab a reference over them all but the master > - Only grab the master whith CONFIG_MTD_PARTITIONED_MASTER > Same logic must apply in the put path, otherwise it would be broken. > > Cc: Tomas Winkler <tomas.winkler@intel.com> > Cc: Alexander Usyskin <alexander.usyskin@intel.com> > Cc: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> > Fixes: 19bfa9ebebb5 ("mtd: use refcount to prevent corruption") > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > Hello, > > Alexander, Zhang, please test this version, it is close to what Zhang > produced while not strictly identical. This compile-tested only, please > check on you side whether it fixes the refcounting issue with and > without PARTITIONED_MASTER, as well as with Kasan. > > Alexander, maybe there is something else to fix, in all cases the logic > here was broken so let's start by this one and see if we need something > else on top. > > Thanks, > Miquèl > --- > drivers/mtd/mtdcore.c | 20 ++++++++++---------- > 1 file changed, 10 insertions(+), 10 deletions(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 2466ea466466..9b90e1be43e7 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -1241,14 +1241,15 @@ int __get_mtd_device(struct mtd_info *mtd) > return -ENODEV; > } > > - kref_get(&mtd->refcnt); > - > - while (mtd->parent) { > - if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER) || > mtd->parent != master) > - kref_get(&mtd->parent->refcnt); > + while (mtd) { > + if (mtd != master) > + kref_get(&mtd->refcnt); > mtd = mtd->parent; > } > > + if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) > + kref_get(&master->refcnt); > + > return 0; > } > EXPORT_SYMBOL_GPL(__get_mtd_device); > @@ -1332,11 +1333,10 @@ void __put_mtd_device(struct mtd_info *mtd) > { > struct mtd_info *master = mtd_get_master(mtd); > > - while (mtd != master) { > - struct mtd_info *parent = mtd->parent; > - > - kref_put(&mtd->refcnt, mtd_device_release); > - mtd = parent; > + while (mtd) { > + if (mtd != master) > + kref_put(&mtd->refcnt, mtd_device_release); kref_put may release mtd here, so the mtd->parent should be stored as in the original patch. > + mtd = mtd->parent; > } > > if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) > -- > 2.34.1
Hi Alexander, alexander.usyskin@intel.com wrote on Mon, 31 Jul 2023 08:27:44 +0000: > > > > The logic is way too convoluted, let's clean the kref_get/put section to > > clarify what this block does, hopefully solving the refcounting issue > > when using CONFIG_MTD_PARTITIONED_MASTER at the same time: > > - Iterate through all the parent mtd devices > > - Grab a reference over them all but the master > > - Only grab the master whith CONFIG_MTD_PARTITIONED_MASTER > > Same logic must apply in the put path, otherwise it would be broken. > > > > Cc: Tomas Winkler <tomas.winkler@intel.com> > > Cc: Alexander Usyskin <alexander.usyskin@intel.com> > > Cc: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> > > Fixes: 19bfa9ebebb5 ("mtd: use refcount to prevent corruption") > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > Hello, > > > > Alexander, Zhang, please test this version, it is close to what Zhang > > produced while not strictly identical. This compile-tested only, please > > check on you side whether it fixes the refcounting issue with and > > without PARTITIONED_MASTER, as well as with Kasan. > > > > Alexander, maybe there is something else to fix, in all cases the logic > > here was broken so let's start by this one and see if we need something > > else on top. > > > > Thanks, > > Miquèl > > --- > > drivers/mtd/mtdcore.c | 20 ++++++++++---------- > > 1 file changed, 10 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > > index 2466ea466466..9b90e1be43e7 100644 > > --- a/drivers/mtd/mtdcore.c > > +++ b/drivers/mtd/mtdcore.c > > @@ -1241,14 +1241,15 @@ int __get_mtd_device(struct mtd_info *mtd) > > return -ENODEV; > > } > > > > - kref_get(&mtd->refcnt); > > - > > - while (mtd->parent) { > > - if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER) || > > mtd->parent != master) > > - kref_get(&mtd->parent->refcnt); > > + while (mtd) { > > + if (mtd != master) > > + kref_get(&mtd->refcnt); > > mtd = mtd->parent; > > } > > > > + if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) > > + kref_get(&master->refcnt); > > + > > return 0; > > } > > EXPORT_SYMBOL_GPL(__get_mtd_device); > > @@ -1332,11 +1333,10 @@ void __put_mtd_device(struct mtd_info *mtd) > > { > > struct mtd_info *master = mtd_get_master(mtd); > > > > - while (mtd != master) { > > - struct mtd_info *parent = mtd->parent; > > - > > - kref_put(&mtd->refcnt, mtd_device_release); > > - mtd = parent; > > + while (mtd) { > > + if (mtd != master) > > + kref_put(&mtd->refcnt, mtd_device_release); > kref_put may release mtd here, so the mtd->parent should be stored as in the original patch. Absolutely. I'll fix this in v2. Thanks, Miquèl
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 2466ea466466..9b90e1be43e7 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -1241,14 +1241,15 @@ int __get_mtd_device(struct mtd_info *mtd) return -ENODEV; } - kref_get(&mtd->refcnt); - - while (mtd->parent) { - if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER) || mtd->parent != master) - kref_get(&mtd->parent->refcnt); + while (mtd) { + if (mtd != master) + kref_get(&mtd->refcnt); mtd = mtd->parent; } + if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER)) + kref_get(&master->refcnt); + return 0; } EXPORT_SYMBOL_GPL(__get_mtd_device); @@ -1332,11 +1333,10 @@ void __put_mtd_device(struct mtd_info *mtd) { struct mtd_info *master = mtd_get_master(mtd); - while (mtd != master) { - struct mtd_info *parent = mtd->parent; - - kref_put(&mtd->refcnt, mtd_device_release); - mtd = parent; + while (mtd) { + if (mtd != master) + kref_put(&mtd->refcnt, mtd_device_release); + mtd = mtd->parent; } if (IS_ENABLED(CONFIG_MTD_PARTITIONED_MASTER))
The logic is way too convoluted, let's clean the kref_get/put section to clarify what this block does, hopefully solving the refcounting issue when using CONFIG_MTD_PARTITIONED_MASTER at the same time: - Iterate through all the parent mtd devices - Grab a reference over them all but the master - Only grab the master whith CONFIG_MTD_PARTITIONED_MASTER Same logic must apply in the put path, otherwise it would be broken. Cc: Tomas Winkler <tomas.winkler@intel.com> Cc: Alexander Usyskin <alexander.usyskin@intel.com> Cc: Zhang Xiaoxu <zhangxiaoxu5@huawei.com> Fixes: 19bfa9ebebb5 ("mtd: use refcount to prevent corruption") Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- Hello, Alexander, Zhang, please test this version, it is close to what Zhang produced while not strictly identical. This compile-tested only, please check on you side whether it fixes the refcounting issue with and without PARTITIONED_MASTER, as well as with Kasan. Alexander, maybe there is something else to fix, in all cases the logic here was broken so let's start by this one and see if we need something else on top. Thanks, Miquèl --- drivers/mtd/mtdcore.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)