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 <[email protected]> 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/[email protected]?part=14
