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

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

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 4832 bytes --]

> 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?

This issue is not introduced in this patch.

> 
> >  
> > -	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?

ack, I will fix it in v7.

> 
> > +		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()?

According to my understanding, airoha_dev_change_mtu() is running with RTNL
lock held so we will always have the max configured mtu in cur_len.

Regards,
Lorenzo

> 
> > +
> >  	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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]