[PATCH] mtd: Fix refcounting with MTD_PARTITIONED_MASTER

[Miquel Raynal]

Hi Alexander,

alexander.usyskin at 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 at intel.com>
> > Cc: Alexander Usyskin <alexander.usyskin at intel.com>
> > Cc: Zhang Xiaoxu <zhangxiaoxu5 at huawei.com>
> > Fixes: 19bfa9ebebb5 ("mtd: use refcount to prevent corruption")
> > Signed-off-by: Miquel Raynal <miquel.raynal at 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