Re: [PATCH net-next v18 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 3 potential issue(s) to consider:
- [High] net: incomplete locking of AX_MEDIUM_STATUS_MODE using medium_status_mutex
- [Medium] net: logic error evaluating old MTU value for jumbo frame config
- [Medium] net: ax88179: data race on rxctl between set_multicast and link_reset
--

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

This commit splits the ax88179_178a module code into a common library for the
AX88179/178a/179a and 279 controllers, and device-specific code. The module
is renamed to ax88179 to reflect the broader scope.

> diff --git a/drivers/net/usb/ax88179_lib.c b/drivers/net/usb/ax88179_lib.c
> --- /dev/null
> +++ b/drivers/net/usb/ax88179_lib.c

[ ... ]

> +void ax88179_set_multicast(struct net_device *net)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	struct ax88179_data *data = dev->driver_priv;

[ ... ]

> +	if (net->flags & IFF_PROMISC) {
> +		data->rxctl |= AX_RX_CTL_PRO;
> +	} else if (net->flags & IFF_ALLMULTI ||

[ ... ]

> +		data->rxctl |= AX_RX_CTL_AM;
> +	}
> +
> +	ax88179_write_cmd_async(dev, AX_ACCESS_MAC, AX_RX_CTL,
> +				2, 2, &data->rxctl);
> +}

[Severity: Medium]
Can a data race occur on rxctl between this asynchronous write and the
synchronous write in ax88179_link_reset()?

The ax88179_link_reset() function runs in a workqueue and reads
ax179_data->rxctl to write it synchronously to the hardware. Since
ax88179_set_multicast() runs in softirq context, it can preempt
ax88179_link_reset().

If ax88179_set_multicast() interrupts ax88179_link_reset() after the
latter has read ax179_data->rxctl but before its synchronous USB
control message is submitted:

1. ax88179_set_multicast() updates ax179_data->rxctl and enqueues an
asynchronous write URB.
2. ax88179_link_reset() resumes and submits a synchronous write URB
with the old rxctl value.

The host controller will execute the async URB first and the sync URB
second. Could this revert the hardware to the old multicast configuration
while the driver state retains the new configuration, potentially causing
dropped multicast packets until the configuration is changed again?

[ ... ]

> +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: High]
Is this mutex fully protecting the AX_MEDIUM_STATUS_MODE register?

This newly added medium_status_mutex correctly serializes accesses to
the AX_MEDIUM_STATUS_MODE register in ax88179_change_mtu(). However,
other functions like ax88179_link_reset(), ax88179_net_reset(), and
ax88179_stop() perform read-modify-write operations on the exact same
register without acquiring this mutex.

Since ax88179_link_reset() is executed asynchronously from the
usbnet_deferred_kevent workqueue, could it overlap with
ax88179_change_mtu() and cause concurrent lockless modifications
to clobber the MTU or link duplex settings in the hardware?

> +	if (net->mtu > 1500) {

[Severity: Medium]
Does this check the old MTU value instead of the new one?

Because WRITE_ONCE(net->mtu, new_mtu) was moved to the end of
ax88179_change_mtu(), this condition evaluates the old net->mtu value.

If the MTU is being increased from 1500 to 9000, jumbo frames would
be incorrectly left disabled in hardware. Conversely, if it is being
decreased from 9000 to 1500, they would be incorrectly left enabled.

> +		err = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
> +				       2, 2, &tmp16);

[ ... ]

> +		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;
> +	}
> +	WRITE_ONCE(net->mtu, new_mtu);
> +	dev->hard_mtu = net->mtu + net->hard_header_len;
> +	mutex_unlock(&data->medium_status_mutex);
> +
> +	/* max qlen depend on hard_mtu and rx_urb_size */
> +	usbnet_update_max_qlen(dev);
> +
> +	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-ax88179a-v18-0-6fa1d4c40422@birger-koblitz.de?part=3