Re: [PATCH v3 14/20] net: mv643xx: use platform_device_set_of_node()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Dynamically allocated platform devices are leaked during driver removal due to missing `platform_device_put()`.
- [High] The static global `port_platdev` array is accessed without synchronization, leading to probe races and cross-device teardown corruption.
- [High] Missing `phy_disconnect()` on the error path of `mv643xx_eth_probe()`, leading to a use-after-free of the `net_device`.
--

commit 286e3bdd9b38ee5d56ee5ff7b8525f2effcb4c53
Author: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

net: mv643xx: use platform_device_set_of_node()
    
Ahead of reworking the reference counting logic for platform devices,
encapsulate the assignment of the OF node for dynamically allocated
platform devices with the provided helper.

> diff --git a/drivers/net/ethernet/marvell/mv643xx_eth.c b/drivers/net/ethernet/marvell/mv643xx_eth.c
> index 1881583be5ce2..9caa1e47c174c 100644
> --- a/drivers/net/ethernet/marvell/mv643xx_eth.c
> +++ b/drivers/net/ethernet/marvell/mv643xx_eth.c

[ ... ]

> @@ -2780,7 +2780,7 @@ static int mv643xx_eth_shared_of_add_port(struct platform_device *pdev,
>  		goto put_err;
>  	}
>  	ppdev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
> -	ppdev->dev.of_node = of_node_get(pnp);
> +	platform_device_set_of_node(ppdev, pnp);

[Severity: High]
This isn't a bug introduced by this patch, but does this dynamically allocated
platform device leak during driver removal?

Earlier in mv643xx_eth_shared_of_add_port(), platform_device_alloc() sets the
initial reference count to 1. During removal, mv643xx_eth_shared_of_remove()
calls platform_device_del(), which removes the device from the hierarchy but
fails to drop the reference count. 

Without a matching platform_device_put(), the device and its associated data
leak on unbind or probe failure.

[Severity: High]
This is also a pre-existing issue, but could this global port array logic
cause cross-device teardown corruption?

This function searches for a free slot in the global port_platdev array
without synchronization:

mv643xx_eth_shared_of_add_port() {
    ...
    while (dev_num < 3 && port_platdev[dev_num])
        dev_num++;
    ...
}

If controllers probe concurrently, they could race on this global state.
Additionally, if one controller is unbound via sysfs,
mv643xx_eth_shared_of_remove() unconditionally iterates over the global
array and deletes all ports:

mv643xx_eth_shared_of_remove() {
    ...
    for (n = 0; n < 3; n++) {
        if (!port_platdev[n])
            continue;
        platform_device_del(port_platdev[n]);
        port_platdev[n] = NULL;
    }
}

Wouldn't this destroy active network ports belonging to completely independent
controllers sharing the driver?

[Severity: High]
This is another pre-existing issue, but does the probe error path leave a
dangling pointer in the PHY subsystem?

In mv643xx_eth_probe(), the driver connects the PHY (which attaches it and
stores a pointer to the net_device in phydev->attached_dev). If a subsequent
operation in the probe fails, the error path frees the net_device but never
disconnects the PHY:

mv643xx_eth_probe() {
    ...
out:
    if (!IS_ERR(mp->clk))
        clk_disable_unprepare(mp->clk);
    free_netdev(dev);
    
    return err;
}

Can this lead to a use-after-free if the PHY device remains active and
is later accessed via sysfs or core PHY polling routines?

>  
>  	ret = platform_device_add_resources(ppdev, &res, 1);
>  	if (ret)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706-pdev-fwnode-ref-v3-0-1ff028e33779@oss.qualcomm.com?part=14