Re: [PATCH net-next v20 03/13] ax88179_178a: Split driver into library and device specific code

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] net: usb: ax88179: premature MTU state update in
  ax88179_change_mtu()
- [Medium] net: usb: ax88179: incomplete medium_status_mutex locking in
  ax88179_link_reset()
--

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

ax88179_178a: Split driver into library and device specific code

This commit splits common helpers and definitions out of ax88179_178a.c
into ax88179_lib.c and ax88179_lib.h, renames the module to ax88179, and
adds medium_status_mutex and error handling to ax88179_change_mtu().

> diff --git a/drivers/net/usb/ax88179_lib.c b/drivers/net/usb/ax88179_lib.c
> new file mode 100644
> index 0000000000000..063427ff55e42
> --- /dev/null
> +++ b/drivers/net/usb/ax88179_lib.c

[ ... ]

> +int ax88179_change_mtu(struct net_device *net, int new_mtu)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	struct ax88179_data *data;
> +	u16 tmp16;
> +	int err;
> +
> +	data = dev->driver_priv;
> +	mutex_lock(&data->medium_status_mutex);
> +	WRITE_ONCE(net->mtu, new_mtu);
> +	dev->hard_mtu = net->mtu + net->hard_header_len;
> +	if (net->mtu > 1500) {
> +		err = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> +				       2, 2, &tmp16);
> +		if (err < 0)
> +			goto err_out;
> +		tmp16 |= AX_MEDIUM_JUMBO_EN;
> +		err = ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> +					2, 2, &tmp16);
> +		if (err < 0)
> +			goto err_out;
> +	} else {
> +		err = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> +				       2, 2, &tmp16);
> +		if (err < 0)
> +			goto err_out;
> +		tmp16 &= ~AX_MEDIUM_JUMBO_EN;
> +		err = ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> +					2, 2, &tmp16);
> +		if (err < 0)
> +			goto err_out;
> +	}
> +	mutex_unlock(&data->medium_status_mutex);
> +
> +	/* max qlen depend on hard_mtu and rx_urb_size */
> +	usbnet_update_max_qlen(dev);
> +
> +	return 0;
> +
> +err_out:
> +	mutex_unlock(&data->medium_status_mutex);
> +	return err;
> +}

[Severity: Medium]
Does jumping to err_out leave net->mtu and dev->hard_mtu updated when a
register read or write fails?

netif_set_mtu_ext() assumes dev->mtu is unchanged when ndo_change_mtu()
returns an error, so it neither restores the old MTU nor fires
NETDEV_CHANGEMTU notifiers, and err_out also skips
usbnet_update_max_qlen().  This leaves net->mtu and dev->hard_mtu out of
sync with the hardware AX_MEDIUM_JUMBO_EN setting and dev->tx_qlen, and
causes later attempts to set the same new_mtu to return early in
netif_set_mtu_ext() because dev->mtu already equals new_mtu.  Updating
net->mtu and dev->hard_mtu only after the register access succeeds would
avoid this regression.

> diff --git a/drivers/net/usb/ax88179_lib.h b/drivers/net/usb/ax88179_lib.h
> new file mode 100644
> index 0000000000000..9111a1395eb2f
> --- /dev/null
> +++ b/drivers/net/usb/ax88179_lib.h

[ ... ]

> +struct ax88179_data {
> +	struct mutex medium_status_mutex;  /* protect AX_MEDIUM_STATUS_MODE */
> +	u8  eee_enabled;
> +	u8  eee_active;
> +	u16 rxctl;
> +	u8 in_pm;
> +	u32 wol_supported;
> +	u32 wolopts;
> +	u8 disconnecting;
> +};

[Severity: Medium]
Should ax88179_link_reset() also take medium_status_mutex when updating
AX_MEDIUM_STATUS_MODE?

ax88179_link_reset() runs from usbnet_deferred_kevent() without rtnl_lock
or medium_status_mutex, reads dev->net->mtu, sleeps in a USB control
transfer for AX_RX_BULKIN_QCTRL, and then writes AX_MEDIUM_STATUS_MODE:

drivers/net/usb/ax88179_178a.c:ax88179_link_reset() {
	...
	} else if (GMII_PHY_PHYSR_GIGA == (tmp16 & GMII_PHY_PHYSR_SMASK)) {
		mode |= AX_MEDIUM_GIGAMODE | AX_MEDIUM_EN_125MHZ;
		if (dev->net->mtu > 1500)
			mode |= AX_MEDIUM_JUMBO_EN;
	...
	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_RX_BULKIN_QCTRL, 5, 5, tmp);
	...
	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
			  2, 2, &mode);
}

This allows a race with ax88179_change_mtu():

CPU0                              kworker
ax88179_change_mtu()              ax88179_link_reset()
                                    read dev->net->mtu (1500)
                                    ax88179_write_cmd(... BULKIN_QCTRL ...)
  mutex_lock(&medium_status_mutex)
  WRITE_ONCE(net->mtu, 4000)
  read AX_MEDIUM_STATUS_MODE
  write AX_MEDIUM_STATUS_MODE
    (sets AX_MEDIUM_JUMBO_EN)
  mutex_unlock(&medium_status_mutex)
                                    write AX_MEDIUM_STATUS_MODE
                                      <-- overwrites AX_MEDIUM_JUMBO_EN

If the writes interleave in the opposite order, ax88179_change_mtu() can
read AX_MEDIUM_STATUS_MODE before ax88179_link_reset() updates it and
write back stale speed and duplex bits.

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