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

Thierry Reding <[email protected]> Mon, 27 Jul 2026 18:24:04 +0200

Newsgroups	org.kernel.vger.linux-tegra,dev.linux.lists.linux-staging,org.kernel.vger.linux-devicetree,org.kernel.vger.linux-kernel,org.kernel.vger.linux-media
Message-ID	<ameBr5oM8pp4PpiQ@orome>

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 <[email protected]>

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

signature.asc (application/pgp-signature, 833 B)