linux-kernel - Re: [PATCH v3] net: usb: ax88179_178a: avoid failed operations when device is disconnected

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4ce32363-378c-4ea3-9a4e-d7274d4f7787@suse.com>
Date:   Fri, 1 Dec 2023 21:14:46 +0100
From:   Oliver Neukum <oneukum@...e.com>
To:     Jose Ignacio Tornos Martinez <jtornosm@...hat.com>, greg@...ah.com
Cc:     davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org,
        netdev@...r.kernel.org, oneukum@...e.com, pabeni@...hat.com,
        stern@...land.harvard.edu, stable@...r.kernel.org
Subject: Re: [PATCH v3] net: usb: ax88179_178a: avoid failed operations when
 device is disconnected

On 01.12.23 14:26, Jose Ignacio Tornos Martinez wrote:
Hi,

this is much better.

> @@ -1661,14 +1668,19 @@ static int ax88179_reset(struct usbnet *dev)
>   
>   static int ax88179_stop(struct usbnet *dev)
>   {
> +	struct ax88179_data *ax179_data = dev->driver_priv;
>   	u16 tmp16;
>   
> +	ax179_data->stopping_unbinding = 1;

This is problematic. ndo_stop() is not limited to disconnection.
It is also used whenever an interface transitions from up to down.

> +
>   	ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
>   			 2, 2, &tmp16);
>   	tmp16 &= ~AX_MEDIUM_RECEIVE_EN;
>   	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE,
>   			  2, 2, &tmp16);
>   
> +	ax179_data->stopping_unbinding = 0;
> +
>   	return 0;
>   }
>   

On a general note, you are going for a belt and suspenders approach.
It seems to me that you have two options.

1. Do as Alan suggested and ignore ENODEV. You'd be acknowledging that
these devices are hotpluggable and therefore -ENODEV is not an error
2. Use only a flag. But if you do that, you are setting it in the wrong
place. It should be set in usbnet_disconnect()

O and, well, this is a very mior issue, but you've introduced a memory
ordering issue. You ought to use smp_wmb() after setting the flag and
smp_rmb() before reading it.

	Regards
		Oliver