Re: [PATCH v2] mtd: Fix refcounting with MTD_PARTITIONED_MASTER

[Miquel Raynal <miquel.raynal@bootlin.com>]

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

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/