Re: [PATCH net-next v21 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: unsynchronized AX_RX_CTL updates in
  ax88179_set_multicast()
--

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

ax88179_178a: Add AX179A/AX279 multicast configuration

This commit updates ax88179_set_multicast() to set AX_RX_CTL_DROPCRCERR
and conditionally set AX_RX_CTL_IPE based on data->ip_align, 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 fa6423f51849e..f27b073e1bccb 100644
> --- a/drivers/net/usb/ax88179_lib.c
> +++ b/drivers/net/usb/ax88179_lib.c
> @@ -319,16 +319,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 {
> @@ -349,9 +354,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 ax88179_set_multicast() race with ax88179a_mac_link_up() when writing
AX_RX_CTL?

ax88179_set_multicast() runs in BH context under netif_addr_lock_bh(),
while phylink runs ax88179a_mac_link_up() in process context without any
shared lock:

drivers/net/usb/ax88179a_devices.c:ax88179a_mac_link_up() {
	/* Stop RX/TX for link configuration */
	ax88179_write_u16(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, AX_RX_CTL_STOP);
	...
	mutex_lock(&ax179_data->medium_status_mutex);
	if (dev->net->mtu > 1500)
		mode |= AX_MEDIUM_JUMBO_EN;
	ax88179_write_u16(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, 2, mode);

	ax88179_write_u16(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, READ_ONCE(ax179_data->rxctl));
	mutex_unlock(&ax179_data->medium_status_mutex);
}

WRITE_ONCE() and READ_ONCE() only protect the in-memory ax179_data->rxctl
load and store, not the USB control transfers to AX_RX_CTL:

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

This leaves the hardware AX_RX_CTL filter out of sync with data->rxctl
when RX mode updates (such as IPv6 MLD joins during interface bring-up)
overlap with link resolution.  In addition, if ax88179_set_multicast()
runs while ax88179a_mac_link_up() is between writing AX_RX_CTL_STOP and
restoring AX_RX_CTL at the end of link setup, its asynchronous write sets
AX_RX_CTL_START and re-enables RX in the middle of MAC and bulk-in queue
reconfiguration.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-ax88179a-v21-0-c2d34410bcbe@birger-koblitz.de?part=8