[PATCH] staging: media: tegra-video: fix of_node_put() on VIP parse errors

[PATCH] staging: media: tegra-video: fix of_node_put() on VIP parse errors

[Hao-Qun Huang]

tegra_vip_channel_of_parse() initializes np from dev->of_node without
taking a reference, but its error paths drop one through the
err_node_put label. This underflows the refcount of the VIP device's
OF node when endpoint parsing fails on a malformed device tree.

The only reference the function takes on np is the success-path
of_node_get() stored in vip->chan.of_node, and that one is already
released by the tegra_vip_init() error path and by tegra_vip_exit().

Return errors directly instead of jumping to the bogus cleanup label.

Fixes: e740d199cf0f ("staging: media: tegra-video: add support for Tegra20 parallel input")
Assisted-by: Claude:claude-fable-5
Signed-off-by: Hao-Qun Huang <alvinhuang0603@gmail.com>
---
diff --git a/drivers/staging/media/tegra-video/vip.c b/drivers/staging/media/tegra-video/vip.c
index 9ff1f1750a15..5fba11e31e1d 100644
--- a/drivers/staging/media/tegra-video/vip.c
+++ b/drivers/staging/media/tegra-video/vip.c
@@ -126,7 +126,7 @@ static int tegra_vip_channel_of_parse(struct tegra_vip *vip)
 	if (!ep) {
 		err = -EINVAL;
 		dev_err_probe(dev, err, "%pOF: error getting endpoint node\n", np);
-		goto err_node_put;
+		return err;
 	}
 
 	fwh = of_fwnode_handle(ep);
@@ -134,14 +134,14 @@ static int tegra_vip_channel_of_parse(struct tegra_vip *vip)
 	of_node_put(ep);
 	if (err) {
 		dev_err_probe(dev, err, "%pOF: failed to parse v4l2 endpoint\n", np);
-		goto err_node_put;
+		return err;
 	}
 
 	num_pads = of_graph_get_endpoint_count(np);
 	if (num_pads != TEGRA_VIP_PADS_NUM) {
 		err = -EINVAL;
 		dev_err_probe(dev, err, "%pOF: need 2 pads, got %d\n", np, num_pads);
-		goto err_node_put;
+		return err;
 	}
 
 	vip->chan.of_node = of_node_get(np);
@@ -149,10 +149,6 @@ static int tegra_vip_channel_of_parse(struct tegra_vip *vip)
 	vip->chan.pads[TEGRA_VIP_PAD_SOURCE].flags = MEDIA_PAD_FL_SOURCE;
 
 	return 0;
-
-err_node_put:
-	of_node_put(np);
-	return err;
 }
 
 static int tegra_vip_channel_init(struct tegra_vip *vip)

Re: [PATCH] staging: media: tegra-video: fix of_node_put() on VIP parse errors

[Luca Ceresoli]

On Tue Jul 7, 2026 at 5:03 PM CEST, Hao-Qun Huang wrote:
> tegra_vip_channel_of_parse() initializes np from dev->of_node without
> taking a reference, but its error paths drop one through the
> err_node_put label. This underflows the refcount of the VIP device's
> OF node when endpoint parsing fails on a malformed device tree.
>
> The only reference the function takes on np is the success-path
> of_node_get() stored in vip->chan.of_node, and that one is already
> released by the tegra_vip_init() error path and by tegra_vip_exit().
>
> Return errors directly instead of jumping to the bogus cleanup label.
>
> Fixes: e740d199cf0f ("staging: media: tegra-video: add support for Tegra20 parallel input")
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Hao-Qun Huang <alvinhuang0603@gmail.com>

Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>

--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] staging: media: tegra-video: fix of_node_put() on VIP parse errors

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2021 bytes --]

On Tue, Jul 07, 2026 at 11:03:26PM +0800, Hao-Qun Huang wrote:
> tegra_vip_channel_of_parse() initializes np from dev->of_node without
> taking a reference, but its error paths drop one through the
> err_node_put label. This underflows the refcount of the VIP device's
> OF node when endpoint parsing fails on a malformed device tree.
> 
> The only reference the function takes on np is the success-path
> of_node_get() stored in vip->chan.of_node, and that one is already
> released by the tegra_vip_init() error path and by tegra_vip_exit().
> 
> Return errors directly instead of jumping to the bogus cleanup label.
> 
> Fixes: e740d199cf0f ("staging: media: tegra-video: add support for Tegra20 parallel input")
> Assisted-by: Claude:claude-fable-5
> Signed-off-by: Hao-Qun Huang <alvinhuang0603@gmail.com>

I suppose this is a common pattern, but isn't this technically broken?
If we were really pedantic I think we'd need to grab a reference to the
node very early in the function, like so:

	struct device_node *np = of_node_get(dev->of_node);

And then make sure to release it on error (and hold on to it on success
and release it on remove). For kobject/kref, unless you hold a reference
to them they can disappear at any time.

That said, of_node_get() doesn't fully support this, it seems. It would
need to do kobject_get_unless_zero() to be fully sure we get a valid OF
node.

Rob, I have seen these kinds of reference leak fixes a number of times
now but they just don't seem to be fully correct. I know that OF_DYNAMIC
probably isn't a very common configuration and so we might not run into
most of these issues. However, I wonder if there generally is a problem,
or if I just don't understand this correctly and there's some other
mechanism that makes sure these references stay around while the code
uses them without their own reference. I guess maybe as long as we run
code as part of probe we can rely on the struct device hanging on to its
reference?

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]