[PATCH V3 1/2] amba: bus: fix refcount leak

[PATCH V3 1/2] amba: bus: fix refcount leak

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

commit 5de1540b7bc4 ("drivers/amba: create devices from device tree")
increases the refcount of of_node, but not releases it in
amba_device_release, so there is refcount leak. By using of_node_put
to avoid refcount leak.

Fixes: 5de1540b7bc4 ("drivers/amba: create devices from device tree")
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

Note: Code inspection, not tested.

V3:
 Move this out from patch V2 1/1

 drivers/amba/bus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index ce88af9eb562..09e72967b8ab 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -528,6 +528,7 @@ static void amba_device_release(struct device *dev)
 {
 	struct amba_device *d = to_amba_device(dev);
 
+	of_node_put(d->dev.of_node);
 	if (d->res.parent)
 		release_resource(&d->res);
 	mutex_destroy(&d->periphid_lock);
-- 
2.37.1

[PATCH V3 2/2] of/platform: increase refcount of fwnode

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

commit 0f8e5651095b
("of/platform: Propagate firmware node by calling device_set_node()")
use of_fwnode_handle to replace of_node_get, which introduces a side
effect that the refcount is not increased. Then the out of tree
jailhouse hypervisor enable/disable test will trigger kernel dump in
of_overlay_remove, with the following sequence
"
   of_changeset_revert(&overlay_changeset);
   of_changeset_destroy(&overlay_changeset);
   of_overlay_remove(&overlay_id);
"

So increase the refcount to avoid issues.

This patch also release the refcount when releasing amba device to avoid
refcount leakage.

Fixes: 0f8e5651095b ("of/platform: Propagate firmware node by calling device_set_node()")
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V3:
 - Move amba device changes to patch 1/1

V2:
 - Per Andy's comment, use of_fwnode_handle(of_node_get(np))
 - release amba device of_node refcount when releasing amba device, this is
   done from code inspection, no test.

 drivers/of/platform.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 267d8c9a5612..d328bbb679c7 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -175,7 +175,7 @@ struct platform_device *of_device_alloc(struct device_node *np,
 	}
 
 	/* setup generic device info */
-	device_set_node(&dev->dev, of_fwnode_handle(np));
+	device_set_node(&dev->dev, of_fwnode_handle(of_node_get(np)));
 	dev->dev.parent = parent ? : &platform_bus;
 
 	if (bus_id)
@@ -273,7 +273,7 @@ static struct amba_device *of_amba_device_create(struct device_node *node,
 	dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
 
 	/* setup generic device info */
-	device_set_node(&dev->dev, of_fwnode_handle(node));
+	device_set_node(&dev->dev, of_fwnode_handle(of_node_get(node)));
 	dev->dev.parent = parent ? : &platform_bus;
 	dev->dev.platform_data = platform_data;
 	if (bus_id)
-- 
2.37.1

Re: [PATCH V3 1/2] amba: bus: fix refcount leak

[Andy Shevchenko]

On Mon, Aug 21, 2023 at 10:39:27AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> commit 5de1540b7bc4 ("drivers/amba: create devices from device tree")
> increases the refcount of of_node, but not releases it in
> amba_device_release, so there is refcount leak. By using of_node_put
> to avoid refcount leak.

LGTM,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH V3 2/2] of/platform: increase refcount of fwnode

[Rob Herring]

On Mon, 21 Aug 2023 10:39:28 +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> commit 0f8e5651095b
> ("of/platform: Propagate firmware node by calling device_set_node()")
> use of_fwnode_handle to replace of_node_get, which introduces a side
> effect that the refcount is not increased. Then the out of tree
> jailhouse hypervisor enable/disable test will trigger kernel dump in
> of_overlay_remove, with the following sequence
> "
>    of_changeset_revert(&overlay_changeset);
>    of_changeset_destroy(&overlay_changeset);
>    of_overlay_remove(&overlay_id);
> "
> 
> So increase the refcount to avoid issues.
> 
> This patch also release the refcount when releasing amba device to avoid
> refcount leakage.
> 
> Fixes: 0f8e5651095b ("of/platform: Propagate firmware node by calling device_set_node()")
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V3:
>  - Move amba device changes to patch 1/1
> 
> V2:
>  - Per Andy's comment, use of_fwnode_handle(of_node_get(np))
>  - release amba device of_node refcount when releasing amba device, this is
>    done from code inspection, no test.
> 
>  drivers/of/platform.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Applied, thanks!

Re: [PATCH V3 2/2] of/platform: increase refcount of fwnode

[Geert Uytterhoeven]

Hi Rob,

On Tue, Aug 22, 2023 at 1:33 AM Rob Herring <robh@kernel.org> wrote:
> On Mon, 21 Aug 2023 10:39:28 +0800, Peng Fan (OSS) wrote:
> > From: Peng Fan <peng.fan@nxp.com>
> >
> > commit 0f8e5651095b
> > ("of/platform: Propagate firmware node by calling device_set_node()")
> > use of_fwnode_handle to replace of_node_get, which introduces a side
> > effect that the refcount is not increased. Then the out of tree
> > jailhouse hypervisor enable/disable test will trigger kernel dump in
> > of_overlay_remove, with the following sequence
> > "
> >    of_changeset_revert(&overlay_changeset);
> >    of_changeset_destroy(&overlay_changeset);
> >    of_overlay_remove(&overlay_id);
> > "
> >
> > So increase the refcount to avoid issues.
> >
> > This patch also release the refcount when releasing amba device to avoid
> > refcount leakage.
> >
> > Fixes: 0f8e5651095b ("of/platform: Propagate firmware node by calling device_set_node()")
> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V3:
> >  - Move amba device changes to patch 1/1
> >
> > V2:
> >  - Per Andy's comment, use of_fwnode_handle(of_node_get(np))
> >  - release amba device of_node refcount when releasing amba device, this is
> >    done from code inspection, no test.
> >
> >  drivers/of/platform.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
>
> Applied, thanks!

Good to see this got applied!

I had missed this fix, and spent quite some time investigating why multiple
overlay add/removal sequences caused a crash.  Only after I had
written a reproducer unittest[1], I managed to bisect the issue,
and found Peng's fix. Thanks!

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

[1] "[PATCH] of: unittest: Run overlay apply/revert sequence three times"
    https://lore.kernel.org/all/a9fb4eb560c58d11a7f167bc78a137b46e76cf15.1692699743.git.geert+renesas@glider.be/

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH V3 1/2] amba: bus: fix refcount leak

[Linus Walleij]

On Mon, Aug 21, 2023 at 4:34 AM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:

> From: Peng Fan <peng.fan@nxp.com>
>
> commit 5de1540b7bc4 ("drivers/amba: create devices from device tree")
> increases the refcount of of_node, but not releases it in
> amba_device_release, so there is refcount leak. By using of_node_put
> to avoid refcount leak.
>
> Fixes: 5de1540b7bc4 ("drivers/amba: create devices from device tree")
> Signed-off-by: Peng Fan <peng.fan@nxp.com>

Makes sense.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij