[PATCH v2] mtd: Fix refcounting with MTD_PARTITIONED_MASTER

[Usyskin, Alexander]

Hi Miquel,

> -----Original Message-----
> From: Miquel Raynal <miquel.raynal at bootlin.com>
> Sent: Monday, July 31, 2023 12:09
> To: Richard Weinberger <richard at nod.at>; Vignesh Raghavendra
> <vigneshr at ti.com>; Tudor Ambarus <tudor.ambarus at linaro.org>; Pratyush
> Yadav <pratyush at kernel.org>; Michael Walle <michael at walle.cc>; linux-
> mtd at lists.infradead.org
> Cc: Miquel Raynal <miquel.raynal at bootlin.com>; Winkler, Tomas
> <tomas.winkler at intel.com>; Usyskin, Alexander
> <alexander.usyskin at intel.com>; Zhang Xiaoxu <zhangxiaoxu5 at 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 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>
> 

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 at 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