[PATCH] mtd: Fix possible refcounting issue when going through partition nodes

[PATCH] mtd: Fix possible refcounting issue when going through partition nodes

[Miquel Raynal]

Under "normal" conditions, the loop goes over all the partitions, and
'breaks' when the relevant partition is found. After the break and
outside the loop, of_node_put() is called to release the 'partitions'
of_node. However if no partition matches (I'm not sure this is a
real-world use case), the loop terminates normally and of_node_put()
gets called on the head of the list, meaning of_node_put() will be
called twice on the loop header, which is not appropriate.

Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Julia Lawall <julia.lawall@inria.fr>
Closes: https://lore.kernel.org/r/202312250546.ISzglvM2-lkp@intel.com/
Cc: Christian Marangi <ansuelsmth@gmail.com>
Cc: Rafał Miłecki <rafal@milecki.pl>
Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
This is compile-tested only.
---
 drivers/mtd/mtdcore.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
index bb0759ca12f1..1049d8223898 100644
--- a/drivers/mtd/mtdcore.c
+++ b/drivers/mtd/mtdcore.c
@@ -620,11 +620,11 @@ static void mtd_check_of_node(struct mtd_info *mtd)
 		if (plen == mtd_name_len &&
 		    !strncmp(mtd->name, pname + offset, plen)) {
 			mtd_set_of_node(mtd, mtd_dn);
+			of_node_put(partitions);
 			break;
 		}
 	}
 
-	of_node_put(partitions);
 exit_parent:
 	of_node_put(parent_dn);
 }
-- 
2.34.1


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

Re: [PATCH] mtd: Fix possible refcounting issue when going through partition nodes

[Miquel Raynal]

Hello,

miquel.raynal@bootlin.com wrote on Wed,  3 Jan 2024 16:35:49 +0100:

> Under "normal" conditions, the loop goes over all the partitions, and
> 'breaks' when the relevant partition is found. After the break and
> outside the loop, of_node_put() is called to release the 'partitions'
> of_node. However if no partition matches (I'm not sure this is a
> real-world use case), the loop terminates normally and of_node_put()
> gets called on the head of the list, meaning of_node_put() will be
> called twice on the loop header, which is not appropriate.

No, this is wrong. I got mislead by the report which does not specify
where the leak is. The problem is likely over 'mtd_dn' rather than
'partitions'. But we indeed need to put the 'mtd_dn' node before the
break. In practice the core calls of_node_get() right after
mtd_check_of_node() returns, so the refcounter of the node will be
incremented back, but it is probably more future-proof to do it this
way.

> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Julia Lawall <julia.lawall@inria.fr>
> Closes: https://lore.kernel.org/r/202312250546.ISzglvM2-lkp@intel.com/
> Cc: Christian Marangi <ansuelsmth@gmail.com>
> Cc: Rafał Miłecki <rafal@milecki.pl>
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
> This is compile-tested only.
> ---
>  drivers/mtd/mtdcore.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index bb0759ca12f1..1049d8223898 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -620,11 +620,11 @@ static void mtd_check_of_node(struct mtd_info *mtd)
>  		if (plen == mtd_name_len &&
>  		    !strncmp(mtd->name, pname + offset, plen)) {
>  			mtd_set_of_node(mtd, mtd_dn);
> +			of_node_put(partitions);
>  			break;
>  		}
>  	}
>  
> -	of_node_put(partitions);
>  exit_parent:
>  	of_node_put(parent_dn);
>  }


Thanks,
Miquèl

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