netdev - Re: [PATCH] smsc95xx: Ignore -ENODEV errors when device is unplugged

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: <YiO5Ayetj9aCRkmR@lunn.ch>
Date:   Sat, 5 Mar 2022 20:24:51 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Fabio Estevam <festevam@...il.com>
Cc:     kuba@...nel.org, steve.glendinning@...well.net,
        UNGLinuxDriver@...rochip.com, fntoth@...il.com,
        martyn.welch@...labora.com, netdev@...r.kernel.org,
        linux-usb@...r.kernel.org, Fabio Estevam <festevam@...x.de>
Subject: Re: [PATCH] smsc95xx: Ignore -ENODEV errors when device is unplugged

On Sat, Mar 05, 2022 at 03:45:03PM -0300, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@...x.de>
> 
> According to Documentation/driver-api/usb/URB.rst when a device
> is unplugged usb_submit_urb() returns -ENODEV.
> 
> This error code propagates all the way up to usbnet_read_cmd() and
> usbnet_write_cmd() calls inside the smsc95xx.c driver during
> Ethernet cable unplug, unbind or reboot.
> 
> This causes the following errors to be shown on reboot, for example:
> 
> ci_hdrc ci_hdrc.1: remove, state 1
> usb usb2: USB disconnect, device number 1
> usb 2-1: USB disconnect, device number 2
> usb 2-1.1: USB disconnect, device number 3
> smsc95xx 2-1.1:1.0 eth1: unregister 'smsc95xx' usb-ci_hdrc.1-1.1, smsc95xx USB 2.0 Ethernet
> smsc95xx 2-1.1:1.0 eth1: Failed to read reg index 0x00000114: -19
> smsc95xx 2-1.1:1.0 eth1: Error reading MII_ACCESS
> smsc95xx 2-1.1:1.0 eth1: __smsc95xx_mdio_read: MII is busy
> smsc95xx 2-1.1:1.0 eth1: Failed to read reg index 0x00000114: -19
> smsc95xx 2-1.1:1.0 eth1: Error reading MII_ACCESS
> smsc95xx 2-1.1:1.0 eth1: __smsc95xx_mdio_read: MII is busy
> smsc95xx 2-1.1:1.0 eth1: hardware isn't capable of remote wakeup
> usb 2-1.4: USB disconnect, device number 4
> ci_hdrc ci_hdrc.1: USB bus 2 deregistered
> ci_hdrc ci_hdrc.0: remove, state 4
> usb usb1: USB disconnect, device number 1
> ci_hdrc ci_hdrc.0: USB bus 1 deregistered
> imx2-wdt 30280000.watchdog: Device shutdown: Expect reboot!
> reboot: Restarting system
> 
> Ignore the -ENODEV errors inside __smsc95xx_mdio_read() and
> __smsc95xx_phy_wait_not_busy() and do not print error messages
> when -ENODEV is returned.
> 
> Signed-off-by: Fabio Estevam <festevam@...x.de>
> ---
> Hi,
> 
> Tested on 5.10.102 and 5.17-rc6.

Please indicate what tree this patch is actually for. It should be
against net, since you want it backporting. Please see the netdev FAQ.
Please also include a Fixes: tag.

> @@ -84,7 +84,7 @@ static int __must_check __smsc95xx_read_reg(struct usbnet *dev, u32 index,
>  	ret = fn(dev, USB_VENDOR_REQUEST_READ_REGISTER, USB_DIR_IN
>  		 | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
>  		 0, index, &buf, 4);
> -	if (unlikely(ret < 0)) {
> +	if (ret < 0 && ret != -ENODEV) {
>  		netdev_warn(dev->net, "Failed to read reg index 0x%08x: %d\n",
>  			    index, ret);
>  		return ret;

I suspect this will result in kasan warnings. The contents of buf is
probably undefined because of the error, yet you continue to set *data
to it. You probably need to explicitly handle the ENODEV case setting
*data to some value.

      Andrew