| Message ID | 20230731090903.770277-1-miquel.raynal@bootlin.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] mtd: Fix refcounting with MTD_PARTITIONED_MASTER | expand |
Hi Miquel, > -----Original Message----- > From: Miquel Raynal <miquel.raynal@bootlin.com> > Sent: Monday, July 31, 2023 12:09 > To: Richard Weinberger <richard@nod.at>; Vignesh Raghavendra > <vigneshr@ti.com>; Tudor Ambarus <tudor.ambarus@linaro.org>; Pratyush > Yadav <pratyush@kernel.org>; Michael Walle <michael@walle.cc>; linux- > mtd@lists.infradead.org > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Winkler, Tomas > <tomas.winkler@intel.com>; Usyskin, Alexander > <alexander.usyskin@intel.com>; Zhang Xiaoxu <zhangxiaoxu5@huawei.com> > Subject: [PATCH v2] mtd: Fix refcounting with 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> > I've put it on top of my "[PATCH v2] mtd: fix use-after-free in mtd release", looks ok, but I have had no problems with refcounting before... IMO, we need both this patch and my patch as they fix different issues. Tested-by: Alexander Usyskin <alexander.usyskin@intel.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. > > Changes in v2: > * Grab the parent before calling kref_put() to avoid accessing a freed > mtd pointer. > > Thanks, > Miquèl > --- > drivers/mtd/mtdcore.c | 17 ++++++++++------- > 1 file changed, 10 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 2466ea466466..9e8ff3a999de 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,10 +1333,12 @@ void __put_mtd_device(struct mtd_info *mtd) > { > struct mtd_info *master = mtd_get_master(mtd); > > - while (mtd != master) { > + while (mtd) { > + /* kref_put() can relese mtd, so keep a reference mtd->parent > */ > struct mtd_info *parent = mtd->parent; > > - kref_put(&mtd->refcnt, mtd_device_release); > + if (mtd != master) > + kref_put(&mtd->refcnt, mtd_device_release); > mtd = parent; > } > > -- > 2.34.1
Hi Alexander, alexander.usyskin@intel.com wrote on Tue, 1 Aug 2023 08:45:47 +0000: > Hi Miquel, > > > -----Original Message----- > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > Sent: Monday, July 31, 2023 12:09 > > To: Richard Weinberger <richard@nod.at>; Vignesh Raghavendra > > <vigneshr@ti.com>; Tudor Ambarus <tudor.ambarus@linaro.org>; Pratyush > > Yadav <pratyush@kernel.org>; Michael Walle <michael@walle.cc>; linux- > > mtd@lists.infradead.org > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Winkler, Tomas > > <tomas.winkler@intel.com>; Usyskin, Alexander > > <alexander.usyskin@intel.com>; Zhang Xiaoxu <zhangxiaoxu5@huawei.com> > > Subject: [PATCH v2] mtd: Fix refcounting with 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> > > > > I've put it on top of my "[PATCH v2] mtd: fix use-after-free in mtd release", > looks ok, but I have had no problems with refcounting before... > IMO, we need both this patch and my patch as they fix different issues. Likely, yes, let's wait for Zhang's feedback; Zhang, does Kasan still complain? > Tested-by: Alexander Usyskin <alexander.usyskin@intel.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. > > > > Changes in v2: > > * Grab the parent before calling kref_put() to avoid accessing a freed > > mtd pointer. > > > > Thanks, > > Miquèl > > --- > > drivers/mtd/mtdcore.c | 17 ++++++++++------- > > 1 file changed, 10 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > > index 2466ea466466..9e8ff3a999de 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,10 +1333,12 @@ void __put_mtd_device(struct mtd_info *mtd) > > { > > struct mtd_info *master = mtd_get_master(mtd); > > > > - while (mtd != master) { > > + while (mtd) { > > + /* kref_put() can relese mtd, so keep a reference mtd->parent > > */ > > struct mtd_info *parent = mtd->parent; > > > > - kref_put(&mtd->refcnt, mtd_device_release); > > + if (mtd != master) > > + kref_put(&mtd->refcnt, mtd_device_release); > > mtd = parent; > > } > > > > -- > > 2.34.1 > Thanks, Miquèl
Hi Alexander, alexander.usyskin@intel.com wrote on Tue, 1 Aug 2023 08:45:47 +0000: > Hi Miquel, > > > -----Original Message----- > > From: Miquel Raynal <miquel.raynal@bootlin.com> > > Sent: Monday, July 31, 2023 12:09 > > To: Richard Weinberger <richard@nod.at>; Vignesh Raghavendra > > <vigneshr@ti.com>; Tudor Ambarus <tudor.ambarus@linaro.org>; Pratyush > > Yadav <pratyush@kernel.org>; Michael Walle <michael@walle.cc>; linux- > > mtd@lists.infradead.org > > Cc: Miquel Raynal <miquel.raynal@bootlin.com>; Winkler, Tomas > > <tomas.winkler@intel.com>; Usyskin, Alexander > > <alexander.usyskin@intel.com>; Zhang Xiaoxu <zhangxiaoxu5@huawei.com> > > Subject: [PATCH v2] mtd: Fix refcounting with 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> > > > > I've put it on top of my "[PATCH v2] mtd: fix use-after-free in mtd release", > looks ok, but I have had no problems with refcounting before... > IMO, we need both this patch and my patch as they fix different issues. > > Tested-by: Alexander Usyskin <alexander.usyskin@intel.com> > Actually I don't think this patch fixes anything, besides the simplification of the helpers which are (IMHO) more readable now. Zhang, are you sure you test on the latest mtd/next branch? Because I see no refcounting issue here, with/without PARTITIONED_MASTER. Thanks, Miquèl
On Mon, 2023-07-31 at 09:09:03 UTC, Miquel Raynal wrote: > 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> > Tested-by: Alexander Usyskin <alexander.usyskin@intel.com> Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next. Miquel
diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 2466ea466466..9e8ff3a999de 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,10 +1333,12 @@ void __put_mtd_device(struct mtd_info *mtd) { struct mtd_info *master = mtd_get_master(mtd); - while (mtd != master) { + while (mtd) { + /* kref_put() can relese mtd, so keep a reference mtd->parent */ struct mtd_info *parent = mtd->parent; - kref_put(&mtd->refcnt, mtd_device_release); + if (mtd != master) + kref_put(&mtd->refcnt, mtd_device_release); mtd = parent; }
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. Changes in v2: * Grab the parent before calling kref_put() to avoid accessing a freed mtd pointer. Thanks, Miquèl --- drivers/mtd/mtdcore.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)