Re: [PATCH net-next v20 08/13] ax88179_178a: Add AX179A/AX279 multicast configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] net: usb: ax88179: TOCTOU race updating AX_RX_CTL in
  ax88179_set_multicast()
--

commit f3a2f1850ef2ba90e3243642ec0642d91a64aa9a
Author: Birger Koblitz <mail@birger-koblitz.de>

ax88179_178a: Add AX179A/AX279 multicast configuration

This commit updates ax88179_set_multicast() to configure AX_RX_CTL_DROPCRCERR
and conditionally set AX_RX_CTL_IPE, and wires it up as .ndo_set_rx_mode in
ax88179a_netdev_ops.

> diff --git a/drivers/net/usb/ax88179_lib.c b/drivers/net/usb/ax88179_lib.c
> index a314db4c97666..b4b00c8b4b623 100644
> --- a/drivers/net/usb/ax88179_lib.c
> +++ b/drivers/net/usb/ax88179_lib.c
> @@ -321,16 +321,21 @@ int ax88179_set_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom, u8
>  void ax88179_set_multicast(struct net_device *net)
>  {
>  	struct usbnet *dev = netdev_priv(net);
> -	struct ax88179_data *data = dev->driver_priv;
>  	u8 *m_filter = ((u8 *)dev->data);
> +	struct ax88179_data *data;
> +	u16 rxctl = 0;
> +
> +	data = dev->driver_priv;
>  
> -	data->rxctl = (AX_RX_CTL_START | AX_RX_CTL_AB | AX_RX_CTL_IPE);
> +	rxctl = (AX_RX_CTL_START | AX_RX_CTL_AB | AX_RX_CTL_DROPCRCERR);
> +	if (data->ip_align)
> +		rxctl |= AX_RX_CTL_IPE;
>  
>  	if (net->flags & IFF_PROMISC) {
> -		data->rxctl |= AX_RX_CTL_PRO;
> +		rxctl |= AX_RX_CTL_PRO;
>  	} else if (net->flags & IFF_ALLMULTI ||
>  		   netdev_mc_count(net) > AX_MAX_MCAST) {
> -		data->rxctl |= AX_RX_CTL_AMALL;
> +		rxctl |= AX_RX_CTL_AMALL;
>  	} else if (netdev_mc_empty(net)) {
>  		/* just broadcast and directed */
>  	} else {
> @@ -351,9 +356,10 @@ void ax88179_set_multicast(struct net_device *net)
>  					AX_MCAST_FLTSIZE, AX_MCAST_FLTSIZE,
>  					m_filter);
>  
> -		data->rxctl |= AX_RX_CTL_AM;
> +		rxctl |= AX_RX_CTL_AM;
>  	}
>  
> +	WRITE_ONCE(data->rxctl, rxctl);
>  	ax88179_write_cmd_async(dev, AX_ACCESS_MAC, AX_RX_CTL,
>  				2, 2, &data->rxctl);
>  }
> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> index 4207d2cc2b865..a6b0a3f47118c 100644
> --- a/drivers/net/usb/ax88179a_devices.c
> +++ b/drivers/net/usb/ax88179a_devices.c
> @@ -678,6 +678,7 @@ static const struct net_device_ops ax88179a_netdev_ops = {
>  	.ndo_set_mac_address	= ax88179_set_mac_addr,
>  	.ndo_validate_addr	= eth_validate_addr,
>  	.ndo_eth_ioctl		= ax88179a_mii_ioctl,
> +	.ndo_set_rx_mode	= ax88179_set_multicast,
>  	.ndo_set_features	= ax88179a_set_features,
>  	.ndo_vlan_rx_add_vid	= ax88179a_vlan_rx_add_vid,
>  	.ndo_vlan_rx_kill_vid	= ax88179a_vlan_rx_kill_vid,

[Severity: Medium]
Can a concurrent ax88179a_mac_link_up() overwrite AX_RX_CTL with a stale
rxctl value after ax88179_set_multicast() updates it?

ax88179_set_multicast() runs in BH context under netif_addr_lock_bh(net),
while ax88179a_mac_link_up() runs in process context from the phylink resolve
workqueue under medium_status_mutex:

drivers/net/usb/ax88179a_devices.c:ax88179a_mac_link_up() {
    ...
	ax88179_write_u16(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, READ_ONCE(ax179_data->rxctl));
	mutex_unlock(&ax179_data->medium_status_mutex);
    ...
}

If an RX mode update (such as an IPv6 MLD multicast join on link up) runs
while ax88179a_mac_link_up() is entering usbnet_write_cmd(), the control URBs
can be queued in the reverse order of the data->rxctl accesses:

CPU0 (kworker)                        CPU1 (<softirq>)
ax88179a_mac_link_up()
  val = READ_ONCE(ax179_data->rxctl)
  ax88179_write_u16(..., val)
    ...
      usb_autopm_get_interface()
                                      ax88179_set_multicast()
                                        WRITE_ONCE(data->rxctl, rxctl)
                                        ax88179_write_cmd_async(...)
                                          usb_submit_urb()  [new rxctl]
      usb_control_msg()  [old val]
        <-- overwrites AX_RX_CTL with stale rxctl

This leaves the hardware AX_RX_CTL register out of sync with data->rxctl and
silently drops multicast or promiscuous traffic until the next RX mode or
link state change.  Deferring the work via usbnet_defer_kevent(dev,
EVENT_SET_RX_MODE) or re-checking rxctl in ax88179a_mac_link_up() would avoid
the race.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-ax88179a-v20-0-37b5a1189983@birger-koblitz.de?part=8