Re: [PATCH] net: ethernet: sunplus: Fix ethernet-ports node leak in spl2sw_probe()

[netdev-bot+sashiko]

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

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

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] Incomplete fix: the 'port@i' OF node references obtained in
  spl2sw_probe() are still leaked.
- [Medium] The patch mixes goto-based unwinding with scope-based cleanup
  in the same function, which include/linux/cleanup.h explicitly forbids.

Pre-existing issues:
- [Medium] phy-handle OF node references are leaked on spl2sw_probe()
  failure paths.
- [Medium] DMA descriptor memory and preallocated RX skbs are leaked on
  all late spl2sw_probe() failure paths.

--

net: ethernet: sunplus: Fix ethernet-ports node leak in spl2sw_probe()

This changes the eth_ports_np local in spl2sw_probe() to be declared with
__free(device_node) so the reference taken by of_get_child_by_name() on the
"ethernet-ports" child node is dropped when the function returns. No other
code in the function is changed.

> diff --git a/drivers/net/ethernet/sunplus/spl2sw_driver.c b/drivers/net/ethernet/sunplus/spl2sw_driver.c
> index 5e0e4c9ecbb07..fdc9c31967381 100644
> --- a/drivers/net/ethernet/sunplus/spl2sw_driver.c
> +++ b/drivers/net/ethernet/sunplus/spl2sw_driver.c
> @@ -319,7 +319,7 @@ static struct device_node *spl2sw_get_eth_child_node(struct device_node *ether_n
>  
>  static int spl2sw_probe(struct platform_device *pdev)
>  {
> -	struct device_node *eth_ports_np;
> +	struct device_node *eth_ports_np __free(device_node) = NULL;
>  	struct device_node *port_np;

[Severity: Medium]
Does this leave the larger sibling leak in the same function unfixed?  The
port_np pointer right below also holds a reference.  spl2sw_get_eth_child_node()
returns the matching child straight out of the iterator:

drivers/net/ethernet/sunplus/spl2sw_driver.c:spl2sw_get_eth_child_node() {
	for_each_child_of_node(ether_np, port_np) {
		...
		if (port_id == id)
			return port_np;
	}

for_each_child_of_node() only drops the reference when the loop advances, so
the early return hands a held reference to the caller.  In spl2sw_probe() that
reference lands in a plain local:

		port_np = spl2sw_get_eth_child_node(eth_ports_np, i);
		if (!port_np)
			continue;

and I can't find any of_node_put(port_np) in spl2sw_driver.c.  Does this leak
one port@i node reference per iteration on the two "continue" paths
(of_get_phy_mode() failure and missing phy-handle), on every goto to
out_unregister_dev / out_free_mdio, and on the successful return as well?

Would declaring port_np with __free(device_node) too, or switching the helper
to for_each_child_of_node_scoped() and returning the node with an explicit
of_node_get(), cover that case?

[Severity: Medium]
Is mixing __free() with the existing goto unwinding here in line with what
cleanup.h asks for?  spl2sw_probe() keeps three unwind labels and eight goto
statements:

out_unregister_dev:
	for (i = 0; i < MAX_NETDEV_NUM; i++)
		if (comm->ndev[i])
			unregister_netdev(comm->ndev[i]);

out_free_mdio:
	spl2sw_mdio_remove(comm);

out_clk_disable:
	clk_disable_unprepare(comm->clk);
	return ret;

include/linux/cleanup.h says:

 * expectation is that usage of "goto" and cleanup helpers is never
 * mixed in the same function.

There is no functional failure today, since of_node_put(NULL) is a no-op for
the gotos taken before the assignment, but the implicit release now runs after
all of the label code and is not visible at any goto site.

The same file also recommends defining and assigning in one statement.  Here
the declaration is "= NULL" at the top of the function while the acquisition
happens much later:

	eth_ports_np = of_get_child_by_name(pdev->dev.of_node, "ethernet-ports");

Would an explicit of_node_put() on the three exits, or converting all of the
resources in this function to scope-based cleanup, fit better than converting
just this one?

[Severity: Medium]
This isn't a bug introduced by this patch, but the phy-handle node references
look like they leak on the probe error paths too, and they are in the same
family of probe-time OF leaks the commit message describes.

of_parse_phandle() takes a reference:

		phy_np = of_parse_phandle(port_np, "phy-handle", 0);

If spl2sw_nvmem_get_mac_address() returns -EPROBE_DEFER, or spl2sw_init_netdev()
fails, control leaves before mac->phy_node = phy_np runs:

		ret = spl2sw_nvmem_get_mac_address(&pdev->dev, port_np, mac_addr);
		if (ret == -EPROBE_DEFER) {
			goto out_unregister_dev;

so the only handle on that reference is lost.  For failures after the transfer
(spl2sw_mac_addr_add(), a later loop iteration, spl2sw_phy_connect()), the
out_unregister_dev label only calls unregister_netdev().  The single
of_node_put(mac->phy_node) in the driver lives in spl2sw_phy_remove():

drivers/net/ethernet/sunplus/spl2sw_phy.c:spl2sw_phy_remove() {
			phy_disconnect(ndev->phydev);
			of_node_put(mac->phy_node);

and that is only reached from spl2sw_remove(), which does not run when probe
fails.  Does the -EPROBE_DEFER case then leak another reference on every
deferred-probe retry?

[Severity: Medium]
This is a pre-existing issue, but the descriptor memory looks like it leaks on
exactly the out_free_mdio and out_unregister_dev paths the commit message
mentions.

spl2sw_descs_init() allocates non-devm memory:

drivers/net/ethernet/sunplus/spl2sw_desc.c:spl2sw_descs_alloc() {
	comm->desc_base = dma_alloc_coherent(&comm->pdev->dev, desc_size, &comm->desc_dma,
					     GFP_KERNEL);

plus the RX skbs and their DMA mappings in spl2sw_rx_descs_init().  These are
released only by spl2sw_descs_free():

drivers/net/ethernet/sunplus/spl2sw_desc.c:spl2sw_descs_free() {
		dma_free_coherent(&comm->pdev->dev, comm->desc_size, comm->desc_base,
				  comm->desc_dma);

In spl2sw_probe(), spl2sw_descs_free() is called only in the "if (ret)" block
immediately after spl2sw_descs_init().  None of out_unregister_dev,
out_free_mdio or out_clk_disable calls it, and spl2sw_remove() does not run
when probe returns an error.

Does that mean a device tree without an "ethernet-ports" child (goto
out_free_mdio) or an -EPROBE_DEFER from spl2sw_nvmem_get_mac_address() (goto
out_unregister_dev) leaks the coherent descriptor buffer, the 32 RX skbs and
their streaming DMA mappings, with the -EPROBE_DEFER case repeating on each
retry?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917114118.2149439-1-vulab%40iscas.ac.cn