Re: [PATCH net-next v4 09/16] net: ethernet: oa_tc6: read, write interface with MMS option

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Teardown race condition in `lan865x_remove` allows `multicast_work` to execute after `priv` is freed.
- [High] The multicast work handler iterates over the network device's multicast list (`netdev->mc.list`) without acquiring the required `netif_addr_lock_bh` lock, leading to data races and potential list traversal crashes.
--

commit dcc33c9ce5d46f4194d94d68b486ac998fa7e8c7
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: oa_tc6: read, write interface with MMS option

This commit introduces new read and write APIs to the oa_tc6 framework that
accept the Memory Map Selector (MMS) as a parameter. It updates the lan8650
driver to use these new interfaces to avoid manually encoding the MMS into
the register address before making the API call.

> diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> index 0277d9737369a..3b555ee698041 100644
> --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
> +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
[ ... ]
> @@ -146,14 +147,16 @@ static int lan865x_set_specific_multicast_addr(struct lan865x_priv *priv)
>  	}
>  

[Severity: High]
This is a pre-existing issue, but does lan865x_set_specific_multicast_addr()
safely iterate over the multicast list?

lan865x_multicast_work_handler() runs asynchronously in a workqueue and calls
lan865x_set_specific_multicast_addr(), which iterates over priv->netdev->mc.list
using netdev_for_each_mc_addr().

However, the multicast list is protected by the netif_addr_lock_bh(netdev)
spinlock, which doesn't appear to be held here.

Could concurrent changes from the network stack alter the list pointers,
causing the driver to read invalid pointers?

>  	/* Enabling specific multicast addresses */
> -	ret = oa_tc6_write_register(priv->tc6, LAN865X_REG_MAC_H_HASH, hash_hi);
> +	ret = oa_tc6_write_register_mms(priv->tc6, LAN865X_REG_MAC_H_HASH,
> +					OA_TC6_PHY_C45_MAC_MMS1, hash_hi);
>  	if (ret) {
>  		netdev_err(priv->netdev, "Failed to write reg_hashh: %d\n",
>  			   ret);
>  		return ret;
>  	}
[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but could the workqueue execution
race with device teardown?

In lan865x_remove(), cancel_work_sync() is called before unregister_netdev():

lan865x_remove() {
    ...
    cancel_work_sync(&priv->multicast_work);
    unregister_netdev(priv->netdev);
    ...
}

Because the netdev is still registered and visible when cancel_work_sync()
returns, could the network stack or userspace still invoke ndo_set_rx_mode
and successfully re-queue the work onto the global workqueue?

If lan865x_remove() then proceeds to unregister the device and free its
private data, would the workqueue subsequently dereference the freed priv
pointer when executing lan865x_multicast_work_handler()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-s2500-mac-phy-support-v4-0-de0fbc13c6d8@onsemi.com?part=9