[PATCH v2] mtd: Fix refcounting with MTD_PARTITIONED_MASTER

* [PATCH v2] mtd: Fix refcounting with MTD_PARTITIONED_MASTER
@ 2023-07-31  9:09 Miquel Raynal
  2023-08-01  8:45 ` Usyskin, Alexander
  2023-08-04  7:03 ` Miquel Raynal
  0 siblings, 2 replies; 5+ messages in thread
From: Miquel Raynal @ 2023-07-31  9:09 UTC (permalink / raw)
  To: Richard Weinberger, Vignesh Raghavendra, Tudor Ambarus,
	Pratyush Yadav, Michael Walle, linux-mtd
  Cc: Miquel Raynal, Tomas Winkler, Alexander Usyskin, Zhang Xiaoxu

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(-)

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


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

^ permalink raw reply related	[flat|nested] 5+ messages in thread