| Message ID | 20240103153549.106681-1-miquel.raynal@bootlin.com |
|---|---|
| State | New |
| Headers | show |
| Series | mtd: Fix possible refcounting issue when going through partition nodes | expand |
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
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); }
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(-)