Re: [PATCH net-next v19 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 issue(s) to consider:
- [Medium] net: incomplete locking of AX_MEDIUM_STATUS_MODE via medium_status_mutex
- [Medium] net: state inconsistency in ax88179_change_mtu() on hardware error
--

commit 8bce779356427c710a868a77c78e34ccdbc5fe82
Author: Birger Koblitz <mail@birger-koblitz.de>
ax88179_178a: Split driver into library and device specific code

This commit splits the ax88179_178a module code into common library
code and device-specific code. It also introduces medium_status_mutex
to protect concurrent access to hardware registers in 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);

[Severity: Medium]
While this mutex protects AX_MEDIUM_STATUS_MODE here, are there other
functions that modify this register concurrently without acquiring the mutex?

For example, it appears ax88179_link_reset() in ax88179_178a.c modifies
AX_MEDIUM_STATUS_MODE asynchronously without acquiring medium_status_mutex or
rtnl_lock:

	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
			  2, 2, &mode);

Can this lead to data races and hardware state corruption when a PHY link
event triggers asynchronously?

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

[Severity: Medium]
If the hardware command fails, we jump to err_out, but the software state
(net->mtu and dev->hard_mtu) has already been updated. Will this leave the
networking stack out of sync with the hardware MTU?

Would it be safer to store the old MTU values and restore them on the error
path, or defer updating the software state until the hardware commands
succeed?

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010-ax88179a-v19-0-7475173cfb56@birger-koblitz.de?part=3