Re: [PATCH net-next v6 09/12] net: airoha: Do not stop GDM port if it is shared

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Stopping a net_device leaves its packets in the shared QDMA hardware TX ring, causing a BUG_ON kernel panic or Use-After-Free when the hardware later transmits them.
- [Medium] When decreasing the MTU on a shared GDM port with multiple active users, the hardware MTU is not reduced to match the new global maximum.
- [Low] The function parameter `netdev` is overwritten in a loop within `airoha_dev_stop()`.
--

commit 30071ffd90f181adfa0fb1ab6717d5d5cfa7e0e8
Author: Lorenzo Bianconi <lorenzo@kernel.org>

net: airoha: Do not stop GDM port if it is shared

Theoretically, in the current codebase, two independent net_devices can
be connected to the same GDM port so we need to check the GDM port is not
used by any other running net_device before setting the forward
configuration to FE_PSE_PORT_DROP.

> diff --git a/drivers/net/ethernet/airoha/airoha_eth.c b/drivers/net/ethernet/airoha/airoha_eth.c
> index 4efd36779453e..aed182c7a5e17 100644
> --- a/drivers/net/ethernet/airoha/airoha_eth.c
> +++ b/drivers/net/ethernet/airoha/airoha_eth.c
[ ... ]
> @@ -1759,6 +1769,7 @@ static int airoha_dev_stop(struct net_device *netdev)
>  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
>  	struct airoha_gdm_port *port = dev->port;
>  	struct airoha_qdma *qdma = dev->qdma;
> +	u32 len = 0;
>  	int i;
>  
>  	netif_tx_disable(netdev);
> @@ -1766,8 +1777,23 @@ static int airoha_dev_stop(struct net_device *netdev)
>  	for (i = 0; i < netdev->num_tx_queues; i++)
>  		netdev_tx_reset_subqueue(netdev, i);

If another device is still using the QDMA (qdma->users > 1), the hardware TX
ring is not cleaned up. NAPI polling is bound to a dummy device and continues
running.

When NAPI later processes the stopped device's pending TX completions, it
unconditionally calls netdev_tx_completed_queue() which invokes
dql_completed().

Since BQL was reset here, will this trigger a BUG_ON in
lib/dynamic_queue_limits.c? Furthermore, if the device was unregistered,
would the skb->dev access in the completion handler result in a
use-after-free?

>  
> -	airoha_set_gdm_port_fwd_cfg(qdma->eth, REG_GDM_FWD_CFG(port->id),
> -				    FE_PSE_PORT_DROP);
> +	for (i = 0; i < ARRAY_SIZE(port->devs); i++) {
> +		if (!port->devs[i])
> +			continue;
> +
> +		netdev = port->devs[i]->dev;

This isn't a bug, but this loop reassigns the local parameter netdev. While
it might be functionally harmless right now because netdev isn't dereferenced
later in this function, could this create issues for future modifications
expecting netdev to still point to the device being stopped?

> +		if (netif_running(netdev))
> +			len = max_t(u32, len, netdev->mtu);
> +	}
> +	len += ETH_HLEN + ETH_FCS_LEN;
> +	airoha_fe_rmw(qdma->eth, REG_GDM_LEN_CFG(port->id),
> +		      GDM_LONG_LEN_MASK,
> +		      FIELD_PREP(GDM_LONG_LEN_MASK, len));
> +
[ ... ]
> @@ -1919,13 +1945,18 @@ static void airoha_dev_get_stats64(struct net_device *netdev,
>  static int airoha_dev_change_mtu(struct net_device *netdev, int mtu)
>  {
>  	struct airoha_gdm_dev *dev = netdev_priv(netdev);
> +	u32 cur_len, len = ETH_HLEN + mtu + ETH_FCS_LEN;
>  	struct airoha_gdm_port *port = dev->port;
> -	u32 len = ETH_HLEN + mtu + ETH_FCS_LEN;
>  	struct airoha_eth *eth = dev->eth;
>  
> -	airoha_fe_rmw(eth, REG_GDM_LEN_CFG(port->id),
> -		      GDM_LONG_LEN_MASK,
> -		      FIELD_PREP(GDM_LONG_LEN_MASK, len));
> +	cur_len = airoha_fe_get(eth, REG_GDM_LEN_CFG(port->id),
> +				GDM_LONG_LEN_MASK);
> +	if (len > cur_len || !port->users ||
> +	    (port->users == 1 && netif_running(netdev)))
> +		airoha_fe_rmw(eth, REG_GDM_LEN_CFG(port->id),
> +			      GDM_LONG_LEN_MASK,
> +			      FIELD_PREP(GDM_LONG_LEN_MASK, len));

If there are multiple running devices on the shared port and they all lower
their MTU, this condition prevents the hardware MTU from being updated to
match the new global maximum. Should this iterate over port->devs to compute
the actual maximum MTU of all running devices, similar to how it is handled
in airoha_dev_stop()?

> +
>  	WRITE_ONCE(netdev->mtu, mtu);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-airoha-eth-multi-serdes-v6-0-c899462c4f75@kernel.org?part=9