Re: [PATCH net-next v5 03/13] net: ethernet: oa_tc6: add OA_TC6_BROKEN_PHY quirk flag

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Workqueue canceled before netdev unregistration, leading to potential Use-After-Free of private data.
- [Medium] Lockless modification of waiting_tx_skb in oa_tc6_free_pending_skbs causes an skb memory leak.
--

--- Patch [3]: [PATCH net-next v5 03/13] net: ethernet: oa_tc6: add OA_TC6_BROKEN_PHY quirk flag ---
commit bf2392954a9b18d08ae43c154ebae75b683c36c6
Author: Ciprian Regus <ciprian.regus@analog.com>

net: ethernet: oa_tc6: add OA_TC6_BROKEN_PHY quirk flag
    
Some MAC-PHY devices need custom MDIO bus access functions to work
around hardware issues. Add the OA_TC6_BROKEN_PHY quirk flag so drivers
can opt in to skip oa_tc6's internal PHY init and manage the PHY
themselves. When the flag is set, oa_tc6 skips MDIO bus registration,
PHY discovery and PHY connection, leaving these to the driver.
    
Drivers that do not set the flag retain the existing behavior. Update
lan865x and the framework documentation accordingly.

> diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> index 0277d9737369a..26a2761332a5a 100644
> --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
> +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> @@ -346,7 +346,7 @@ static int lan865x_probe(struct spi_device *spi)
>  	spi_set_drvdata(spi, priv);
>  	INIT_WORK(&priv->multicast_work, lan865x_multicast_work_handler);
>  
> -	priv->tc6 = oa_tc6_init(spi, netdev);
> +	priv->tc6 = oa_tc6_init(spi, netdev, NULL);
>  	if (!priv->tc6) {
>  		ret = -ENODEV;
>  		goto free_netdev;

[Severity: High]
This isn't a bug introduced by this patch, but looking at the driver's
teardown order in lan865x_remove(), is there a risk of a use-after-free of
the priv structure?

drivers/net/ethernet/microchip/lan865x/lan865x.c:lan865x_remove() {
	...
	cancel_work_sync(&priv->multicast_work);
	unregister_netdev(priv->netdev);
	...
}

If user-space changes the interface flags (e.g., promiscuous mode) while the
driver is being unbound, .ndo_set_rx_mode (lan865x_set_multicast_list) could
be invoked, unconditionally rescheduling priv->multicast_work.

If this happens after cancel_work_sync() but before unregister_netdev()
completes, the rescheduled work could execute after free_netdev() has freed
the priv structure. 

Could unregister_netdev() be called before cancel_work_sync() to guarantee
no further callbacks will be invoked?

[ ... ]

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 8b96558834960..fa1359224535c 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c

[ ... ]

> @@ -1327,11 +1334,13 @@ static int oa_tc6_check_ctrl_protection(struct oa_tc6 *tc6)
>   * oa_tc6_init - allocates and initializes oa_tc6 structure.
>   * @spi: device with which data will be exchanged.
>   * @netdev: network device interface structure.
> + * @quirks: device specific modifiers for the OA TC6 protocol.
>   *
>   * Return: pointer reference to the oa_tc6 structure if the MAC-PHY
>   * initialization is successful otherwise NULL.
>   */
> -struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)
> +struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev,
> +			   struct oa_tc6_quirks *quirks)
>  {

[Severity: Medium]
This is a pre-existing issue, but could this sequence result in a memory leak
of a single SKB in the error handling path?

Looking at oa_tc6_disable_traffic(), tc6->disable_traffic is set to true
locklessly. Immediately after, oa_tc6_free_pending_skbs() calls
oa_tc6_cleanup_waiting_tx_skb(), which checks and clears tc6->waiting_tx_skb
without holding tc6->tx_skb_lock:

drivers/net/ethernet/oa_tc6.c:oa_tc6_cleanup_waiting_tx_skb() {
	if (tc6->waiting_tx_skb) {
		tc6->netdev->stats.tx_dropped++;
		kfree_skb(tc6->waiting_tx_skb);
		tc6->waiting_tx_skb = NULL;
	}
}

Concurrently, oa_tc6_start_xmit() evaluates tc6->disable_traffic and
tc6->waiting_tx_skb locklessly. If oa_tc6_start_xmit() reads false/NULL just
before oa_tc6_disable_traffic() modifies them, it will acquire the
tx_skb_lock and overwrite tc6->waiting_tx_skb with a new skb:

drivers/net/ethernet/oa_tc6.c:oa_tc6_start_xmit() {
	...
	spin_lock_bh(&tc6->tx_skb_lock);
	tc6->waiting_tx_skb = skb;
	spin_unlock_bh(&tc6->tx_skb_lock);
	...
}

Since traffic is disabled and oa_tc6_cleanup_waiting_tx_skb() has already
executed, will this new skb be left in the queue and leaked until the device
is explicitly unbound?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-adin1140-driver-v5-0-4aca7b51a58b@analog.com?part=3