[PATCH] net: ti: icssg-prueth: Fix device_node refcount leak in prueth_probe()

[PATCH] net: ti: icssg-prueth: Fix device_node refcount leak in prueth_probe()

[laixiangwu]

prueth_probe() takes references on the eth0_node/eth1_node device nodes
with of_node_get() and stores them in prueth->eth_node[]. If either of
the "ti,mii-g-rt" or "ti,mii-rt" syscon regmap lookups fails, probe()
returns -ENODEV without dropping those references, leaking the device
nodes.

Jump to the existing put_cores label so the error path releases the
eth0_node/eth1_node references like all the other failure paths in the
function do.

Fixes: 128d5874c082 ("net: ti: icssg-prueth: Add ICSSG ethernet driver")
Signed-off-by: laixiangwu <laixiangwu@cmss.chinamobile.com>
---
 drivers/net/ethernet/ti/icssg/icssg_prueth.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
index 591be5c80..9b971d1df 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
@@ -2258,13 +2258,15 @@ static int prueth_probe(struct platform_device *pdev)
 	prueth->miig_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-g-rt");
 	if (IS_ERR(prueth->miig_rt)) {
 		dev_err(dev, "couldn't get ti,mii-g-rt syscon regmap\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto put_cores;
 	}
 
 	prueth->mii_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-rt");
 	if (IS_ERR(prueth->mii_rt)) {
 		dev_err(dev, "couldn't get ti,mii-rt syscon regmap\n");
-		return -ENODEV;
+		ret = -ENODEV;
+		goto put_cores;
 	}
 
 	prueth->pa_stats = syscon_regmap_lookup_by_phandle(np, "ti,pa-stats");

Re: [PATCH] net: ti: icssg-prueth: Fix device_node refcount leak in prueth_probe()

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] net: ti: icssg-prueth: Fix device_node refcount leak in prueth_probe()

[Xiangwu Lai]

Hi,

Please find below the additional information netdev-bot requested
for this series.

How the issue was discovered: manual code inspection of the error
paths in prueth_probe() while auditing the driver for probe-time
resource leaks.

Whether the issue was actually triggered: no, it is purely
theoretical. The affected branches only run when the "ti,mii-g-rt"
or "ti,mii-rt" syscon regmap lookup fails, which happens only with a
malformed or incomplete device tree. On a valid DT these branches
are never taken. When they are, probe() fails with -ENODEV and the
of_node_get() references taken on eth0_node/eth1_node earlier in
the function are leaked silently; there is no stack trace or error
message pointing at the leak itself.

Hardware the change was tested on: none, it was not tested on real
hardware. The change only re-routes two error branches to the
existing put_cores label, which is already exercised by the other
failure paths in the same function (e.g. prueth_get_cores()
failures), so no new cleanup code is introduced.

Regarding the Sashiko findings:

- The leak in the SR1.0 sibling driver (icssg_prueth_sr1.c) is real,
  and it is fixed by a companion patch submitted at the same time as
  this one:

  https://lore.kernel.org/netdev/20261030085851.2019-1-laixiangwu@cmss.chinamobile.com/

- The port-node loop issue is acknowledged as pre-existing and
  separate from what this patch fixes. References taken inside the
  port scan loop can also be lost on unusual DT input (a duplicate
  port reg value overwrites eth0_node/eth1_node and leaks the
  previous reference, and port nodes with a reg value other than 0
  or 1 keep a reference that is never stored or released). Fixing
  that requires a wider rework of the port parsing logic, so it is
  deliberately left out of this patch to keep the fix minimal. I
  can send a follow-up patch for it if reviewers consider it worth
  fixing.

Best regards,
Xiangwu Lai

Re: [PATCH] net: ti: icssg-prueth: Fix device_node refcount leak in prueth_probe()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 0 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The SR1.0 sibling driver keeps the same device_node refcount
  leak, and this patch does not touch it.

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the port-
  node loop in prueth_probe()…
- [Low] This problem wasn't introduced by this patch, but the DT binding
  and the driver disagree on whether "ti,mii-rt" is required.

--

net: ti: icssg-prueth: Fix device_node refcount leak in prueth_probe()

In icssg_prueth.c, prueth_probe() returned directly with -ENODEV when the
ti,mii-g-rt or ti,mii-rt syscon lookup failed. This patch changes both
returns to goto put_cores, so the eth0_node/eth1_node references are
dropped on those error paths.

> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 591be5c8056b4..9b971d1dfd8cf 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -2258,13 +2258,15 @@ static int prueth_probe(struct platform_device *pdev)
>  	prueth->miig_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-g-rt");
>  	if (IS_ERR(prueth->miig_rt)) {
>  		dev_err(dev, "couldn't get ti,mii-g-rt syscon regmap\n");
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto put_cores;
>  	}

[Severity: Medium]
The subject says this fixes the device_node refcount leak in
prueth_probe(). Does the SR1.0 variant of the driver still have the same
leak?

drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c has its own
prueth_probe(). Its port loop takes the same of_node_get() references on
eth0_node/eth1_node, and it still returns directly here:

drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c:prueth_probe() {
    ...
	prueth->miig_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-g-rt");
	if (IS_ERR(prueth->miig_rt)) {
		dev_err(dev, "couldn't get ti,mii-g-rt syscon regmap\n");
		return -ENODEV;
	}

	prueth->mii_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-rt");
	if (IS_ERR(prueth->mii_rt)) {
		dev_err(dev, "couldn't get ti,mii-rt syscon regmap\n");
		return -ENODEV;
	}
    ...
}

That file already has a put_cores label that calls of_node_put() on both
nodes. prueth_put_cores() checks each pru/rtu/txpru pointer for NULL, and
at this point those pointers are still zero from devm_kzalloc(). So the
same "ret = -ENODEV; goto put_cores;" change looks like it would work
there too.

CONFIG_TI_ICSSG_PRUETH_SR1 builds the SR1 driver, and in-tree DTs use it,
for example k3-am65-iot2050-common-pg1.dtsi with compatible
"ti,am654-sr1-icssg-prueth".

Should the SR1 driver get the same fix, perhaps as a companion patch with
its own Fixes: tag?

>  
>  	prueth->mii_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-rt");
>  	if (IS_ERR(prueth->mii_rt)) {
>  		dev_err(dev, "couldn't get ti,mii-rt syscon regmap\n");
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto put_cores;
>  	}

[Severity: Low]
This is a pre-existing issue and was not introduced by this patch. The
driver treats ti,mii-rt as mandatory here, but the DT binding does not.

In Documentation/devicetree/bindings/net/ti,icssg-prueth.yaml, ti,mii-rt
is only listed under properties. The top-level required list has
ti,mii-g-rt but not ti,mii-rt:

required:
  - compatible
  - sram
  - dmas
  - dma-names
  - ethernet-ports
  - ti,mii-g-rt
  - interrupts
  - interrupt-names

The binding's example node sets ti,mii-g-rt, ti,pa-stats and ti,iep, but
not ti,mii-rt. A DT written from that example would pass schema
validation and then fail here with "couldn't get ti,mii-rt syscon
regmap".

All in-tree users (k3-am642-*, k3-am654-*, k3-am65-iot2050-common.dtsi)
provide ti,mii-rt, so in-tree boards are not affected.

Should ti,mii-rt be added to the binding's required list and to its
example?

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch. While
looking at the eth0_node/eth1_node references, the port loop earlier in
prueth_probe() also seems to have a few related problems:

drivers/net/ethernet/ti/icssg/icssg_prueth.c:prueth_probe() {
    ...
	for_each_child_of_node(eth_ports_node, eth_node) {
		u32 reg;

		if (strcmp(eth_node->name, "port"))
			continue;
		ret = of_property_read_u32(eth_node, "reg", &reg);
		if (ret < 0) {
			dev_err(dev, "%pOF error reading port_id %d\n",
				eth_node, ret);
		}

		of_node_get(eth_node);

		if (reg == 0) {
			eth0_node = eth_node;
			...
		} else if (reg == 1) {
			eth1_node = eth_node;
			...
		} else {
			dev_err(dev, "port reg should be 0 or 1\n");
		}
	}
    ...
}

When of_property_read_u32() fails, the code only logs the error and
keeps going. Can the reg == 0 and reg == 1 checks then read an
uninitialized reg?

In the else branch, is the reference taken by of_node_get(eth_node) ever
dropped?

If two available port nodes both have reg = <0> (or both have <1>), the
second assignment to eth0_node (or eth1_node) overwrites the first. Does
the first node's reference leak in that case?

Related to that, can this check ever fire?

	if (eth0_node == eth1_node) {
		dev_err(dev, "port0 and port1 can't have same reg\n");
		of_node_put(eth0_node);
		return -ENODEV;
	}

Each iteration stores a different node into at most one slot. The two
pointers can only be equal when both are NULL, and the earlier
!eth0_node && !eth1_node check already returns in that case. So a DT with
duplicate reg values would probe successfully instead of failing.

The SR1 copy of this loop in icssg_prueth_sr1.c has the same pattern.

>  
>  	prueth->pa_stats = syscon_regmap_lookup_by_phandle(np, "ti,pa-stats");

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030085651.1944-1-laixiangwu%40cmss.chinamobile.com

Re: [PATCH] net: ti: icssg-prueth: Fix device_node refcount leak in prueth_probe()

[Simon Horman]

On Fri, Oct 30, 2026 at 04:56:51PM +0800, laixiangwu wrote:
> prueth_probe() takes references on the eth0_node/eth1_node device nodes
> with of_node_get() and stores them in prueth->eth_node[]. If either of
> the "ti,mii-g-rt" or "ti,mii-rt" syscon regmap lookups fails, probe()
> returns -ENODEV without dropping those references, leaking the device
> nodes.
> 
> Jump to the existing put_cores label so the error path releases the
> eth0_node/eth1_node references like all the other failure paths in the
> function do.
> 
> Fixes: 128d5874c082 ("net: ti: icssg-prueth: Add ICSSG ethernet driver")
> Signed-off-by: laixiangwu <laixiangwu@cmss.chinamobile.com>
> ---
>  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> index 591be5c80..9b971d1df 100644
> --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> @@ -2258,13 +2258,15 @@ static int prueth_probe(struct platform_device *pdev)
>  	prueth->miig_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-g-rt");
>  	if (IS_ERR(prueth->miig_rt)) {
>  		dev_err(dev, "couldn't get ti,mii-g-rt syscon regmap\n");
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto put_cores;
>  	}
>  
>  	prueth->mii_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-rt");
>  	if (IS_ERR(prueth->mii_rt)) {
>  		dev_err(dev, "couldn't get ti,mii-rt syscon regmap\n");
> -		return -ENODEV;
> +		ret = -ENODEV;
> +		goto put_cores;
>  	}
>  
>  	prueth->pa_stats = syscon_regmap_lookup_by_phandle(np, "ti,pa-stats");

Hi Laixianwu,

Thanks, this patch looks correct to me.
And I appreciate your feedback to the AI bots.

But I wonder if this nearby check also leaks a reference,
because while it drops one, wouldn't there be two if the
condition is met?

        if (eth0_node == eth1_node) {
                dev_err(dev, "port0 and port1 can't have same reg\n");
                of_node_put(eth0_node);
                return -ENODEV;
        }

Also, for future reference, it would probably be better to put
two similar fixes for related drivers in a two-patch patch-set,
rather than posting them separately.

Re: [PATCH] net: ti: icssg-prueth: Fix device_node refcount leak in prueth_probe()

[Simon Horman]

On Mon, Nov 09, 2026 at 04:52:35PM +0000, Simon Horman wrote:
> On Fri, Oct 30, 2026 at 04:56:51PM +0800, laixiangwu wrote:
> > prueth_probe() takes references on the eth0_node/eth1_node device nodes
> > with of_node_get() and stores them in prueth->eth_node[]. If either of
> > the "ti,mii-g-rt" or "ti,mii-rt" syscon regmap lookups fails, probe()
> > returns -ENODEV without dropping those references, leaking the device
> > nodes.
> > 
> > Jump to the existing put_cores label so the error path releases the
> > eth0_node/eth1_node references like all the other failure paths in the
> > function do.
> > 
> > Fixes: 128d5874c082 ("net: ti: icssg-prueth: Add ICSSG ethernet driver")
> > Signed-off-by: laixiangwu <laixiangwu@cmss.chinamobile.com>
> > ---
> >  drivers/net/ethernet/ti/icssg/icssg_prueth.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth.c b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> > index 591be5c80..9b971d1df 100644
> > --- a/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> > +++ b/drivers/net/ethernet/ti/icssg/icssg_prueth.c
> > @@ -2258,13 +2258,15 @@ static int prueth_probe(struct platform_device *pdev)
> >  	prueth->miig_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-g-rt");
> >  	if (IS_ERR(prueth->miig_rt)) {
> >  		dev_err(dev, "couldn't get ti,mii-g-rt syscon regmap\n");
> > -		return -ENODEV;
> > +		ret = -ENODEV;
> > +		goto put_cores;
> >  	}
> >  
> >  	prueth->mii_rt = syscon_regmap_lookup_by_phandle(np, "ti,mii-rt");
> >  	if (IS_ERR(prueth->mii_rt)) {
> >  		dev_err(dev, "couldn't get ti,mii-rt syscon regmap\n");
> > -		return -ENODEV;
> > +		ret = -ENODEV;
> > +		goto put_cores;
> >  	}
> >  
> >  	prueth->pa_stats = syscon_regmap_lookup_by_phandle(np, "ti,pa-stats");
> 
> Hi Laixianwu,
> 
> Thanks, this patch looks correct to me.
> And I appreciate your feedback to the AI bots.
> 
> But I wonder if this nearby check also leaks a reference,
> because while it drops one, wouldn't there be two if the
> condition is met?
> 
>         if (eth0_node == eth1_node) {
>                 dev_err(dev, "port0 and port1 can't have same reg\n");
>                 of_node_put(eth0_node);
>                 return -ENODEV;
>         }

Sorry, one more thing. Which is not strictly related.  But as we're on the
topic of error paths in this function, I am not convinced of the validity
of the value of reg being used even when the call to of_property_read_u32
fails in this loop:

        for_each_child_of_node(eth_ports_node, eth_node) {
                u32 reg;

                if (strcmp(eth_node->name, "port"))
                        continue;
                ret = of_property_read_u32(eth_node, "reg", &reg);
                if (ret < 0) {
                        dev_err(dev, "%pOF error reading port_id %d\n",         
                                eth_node, ret);
                }

                of_node_get(eth_node);

                if (reg == 0) {
                        eth0_node = eth_node;
                        if (!of_device_is_available(eth0_node)) {
                                of_node_put(eth0_node);
                                eth0_node = NULL;
                        }
                } else if (reg == 1) {
                        eth1_node = eth_node;
                        if (!of_device_is_available(eth1_node)) {
                                of_node_put(eth1_node);
                                eth1_node = NULL;
                        }
                } else {
                        dev_err(dev, "port reg should be 0 or 1\n");
                }
        }

> 
> Also, for future reference, it would probably be better to put
> two similar fixes for related drivers in a two-patch patch-set,
> rather than posting them separately.

Re: [PATCH] net: ti: icssg-prueth: Fix device_node refcount leak in prueth_probe()

[Xiangwu Lai]

Hi Simon,

Thanks for the review, and for looking into the neighbouring error
paths.

On Mon, Nov 09, 2026 at 04:52:35PM +0000, Simon Horman wrote:
> But I wonder if this nearby check also leaks a reference,
> because while it drops one, wouldn't there be two if the
> condition is met?

I don't think that branch can be entered at all, so it does not
leak in practice. eth0_node and eth1_node are both initialised to
NULL, and each iteration of the port scan loop assigns at most one
of them (the reg == 0 and reg == 1 cases are mutually exclusive),
while for_each_child_of_node() visits each child exactly once. So
the same node can never end up stored in both slots, and the
both-NULL case is already handled by the preceding "neither port0
nor port1 node available" check. As far as I can tell, the equality
comparison is dead code.

The DT error the check describes, two port nodes carrying the same
reg value, does leak a reference though, just not through that
branch: if both ports have reg = <0>, the second assignment simply
overwrites eth0_node, the reference taken on the first node is
lost, and eth1_node stays NULL, so the equality check never fires.

On Mon, Nov 09, 2026 at 04:59:02PM +0000, Simon Horman wrote:
> Sorry, one more thing. Which is not strictly related. But as we're on the
> topic of error paths in this function, I am not convinced of the validity
> of the value of reg being used even when the call to of_property_read_u32
> fails in this loop:

You are right, the error is only logged and the uninitialised value
of reg is then used to select eth0_node or eth1_node. Depending on
the garbage, the node is routed into one of the slots (including
the overwrite case above) or, when the value is neither 0 nor 1,
the reference taken by the of_node_get() right before is leaked, as
the node is not stored anywhere. This is the pre-existing loop
issue I mentioned in my reply to the bots: the "port reg should be
0 or 1" branch leaks the reference even for a valid out-of-range
reg value.

Understood on the patch-set, and thanks for the hint. I will send a
v2 that combines the fixes for the two drivers, keeping them
unchanged and adding a per-driver patch for the port scan loop that
skips a port when reading its reg property fails, only takes a
reference for reg values 0 and 1, and rejects duplicate reg values,
which is what the existing check seems to intend but never actually
catches. Happy to adjust the split if you would prefer the loop
changes as a separate series.

Best regards,
Xiangwu Lai