[PATCH v3] USB: Fix device driver race

[PATCH v3] USB: Fix device driver race

[Bastien Nocera]

When a new device with a specialised device driver is plugged in, the
new driver will be modprobe()'d but the driver core will attach the
"generic" driver to the device.

After that, nothing will trigger a reprobe when the modprobe()'d device
driver has finished initialising, as the device has the "generic"
driver attached to it.

Trigger a reprobe ourselves when new specialised drivers get registered.

Fixes: 88b7381a939d
Signed-off-by: Bastien Nocera <hadess@hadess.net>
---

Changes since v2:
- Fix formatting

Changes since v1:
- Simplified after Alan Stern's comments and some clarifications from
Benjamin Tissoires.

 drivers/usb/core/driver.c | 29 +++++++++++++++++++++++++++--
 1 file changed, 27 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index f81606c6a35b..44531910637c 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -905,6 +905,25 @@ static int usb_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return 0;
 }
 
+static int __usb_bus_reprobe_drivers(struct device *dev, void *data)
+{
+	struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
+	struct usb_device *udev = to_usb_device(dev);
+
+	if (dev->driver) {
+		struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
+
+		if (udriver == NULL || udriver == &usb_generic_driver) {
+			udev->use_generic_driver = false;
+			device_reprobe(dev);
+		}
+	} else {
+		device_reprobe(dev);
+	}
+
+	return 0;
+}
+
 /**
  * usb_register_device_driver - register a USB device (not interface) driver
  * @new_udriver: USB operations for the device driver
@@ -934,13 +953,19 @@ int usb_register_device_driver(struct usb_device_driver *new_udriver,
 
 	retval = driver_register(&new_udriver->drvwrap.driver);
 
-	if (!retval)
+	if (!retval) {
+		/* Check whether any device could be better served with
+		 * this new driver
+		 */
+		bus_for_each_dev(&usb_bus_type, NULL, NULL,
+				 __usb_bus_reprobe_drivers);
 		pr_info("%s: registered new device driver %s\n",
 			usbcore_name, new_udriver->name);
-	else
+	} else {
 		printk(KERN_ERR "%s: error %d registering device "
 			"	driver %s\n",
 			usbcore_name, retval, new_udriver->name);
+	}
 
 	return retval;
 }

Re: [PATCH v3] USB: Fix device driver race

[Alan Stern]

On Thu, Jul 23, 2020 at 11:30:39PM +0200, Bastien Nocera wrote:
> When a new device with a specialised device driver is plugged in, the
> new driver will be modprobe()'d but the driver core will attach the
> "generic" driver to the device.
> 
> After that, nothing will trigger a reprobe when the modprobe()'d device
> driver has finished initialising, as the device has the "generic"
> driver attached to it.
> 
> Trigger a reprobe ourselves when new specialised drivers get registered.
> 
> Fixes: 88b7381a939d
> Signed-off-by: Bastien Nocera <hadess@hadess.net>

Better than before, but there are still some things to improve.

> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index f81606c6a35b..44531910637c 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -905,6 +905,25 @@ static int usb_uevent(struct device *dev, struct kobj_uevent_env *env)
>  	return 0;
>  }
>  
> +static int __usb_bus_reprobe_drivers(struct device *dev, void *data)
> +{
> +	struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);

This variable isn't used (it gets shadowed below).

> +	struct usb_device *udev = to_usb_device(dev);

And this one doesn't get used unless both tests below succeed, so the 
declaration should be moved down to the inner block.

> +
> +	if (dev->driver) {
> +		struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
> +
> +		if (udriver == NULL || udriver == &usb_generic_driver) {

Since dev->driver is not NULL, udriver cannot possibly be NULL.  No need 
to test for it.

> +			udev->use_generic_driver = false;

Are you sure you want to do that?  If udev->use_generic_driver was set, 
it means that a specialized driver has already been probed and failed.  
We assume only one specialized driver will match a particular device, so 
there's no point in reprobing -- the same driver will match and it will 
just fail the probe again.

> +			device_reprobe(dev);

We shouldn't do this unless we have a good reason for believing the new 
driver will bind to the device.  Otherwise you're messing up a perfectly 
good existing binding for no reason.

You should first test whether the device matches the new driver (pass 
the new driver as the iterator parameter).

> +		}
> +	} else {
> +		device_reprobe(dev);
> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * usb_register_device_driver - register a USB device (not interface) driver
>   * @new_udriver: USB operations for the device driver
> @@ -934,13 +953,19 @@ int usb_register_device_driver(struct usb_device_driver *new_udriver,
>  
>  	retval = driver_register(&new_udriver->drvwrap.driver);
>  
> -	if (!retval)
> +	if (!retval) {
> +		/* Check whether any device could be better served with
> +		 * this new driver
> +		 */
> +		bus_for_each_dev(&usb_bus_type, NULL, NULL,
> +				 __usb_bus_reprobe_drivers);
>  		pr_info("%s: registered new device driver %s\n",
>  			usbcore_name, new_udriver->name);

The new iterator should be added after the pr_info(), not before, 
because we want the new driver's registration to show up in the kernel 
log before any notices about it being bound to devices.

Alan Stern

> -	else
> +	} else {
>  		printk(KERN_ERR "%s: error %d registering device "
>  			"	driver %s\n",
>  			usbcore_name, retval, new_udriver->name);
> +	}
>  
>  	return retval;
>  }
> 

Re: [PATCH v3] USB: Fix device driver race

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2454 bytes --]

Hi Bastien,

I love your patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on peter.chen-usb/ci-for-usb-next balbi-usb/testing/next v5.8-rc6 next-20200723]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Bastien-Nocera/USB-Fix-device-driver-race/20200724-053320
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-rhel-7.6-kselftests (attached as .config)
compiler: gcc-9 (Debian 9.3.0-14) 9.3.0
reproduce (this is a W=1 build):
        # save the attached .config to linux build tree
        make W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/usb/core/driver.c: In function '__usb_bus_reprobe_drivers':
   drivers/usb/core/driver.c:910:28: warning: unused variable 'udriver' [-Wunused-variable]
     910 |  struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
         |                            ^~~~~~~
>> drivers/usb/core/driver.c:918:4: warning: ignoring return value of 'device_reprobe', declared with attribute warn_unused_result [-Wunused-result]
     918 |    device_reprobe(dev);
         |    ^~~~~~~~~~~~~~~~~~~
   drivers/usb/core/driver.c:921:3: warning: ignoring return value of 'device_reprobe', declared with attribute warn_unused_result [-Wunused-result]
     921 |   device_reprobe(dev);
         |   ^~~~~~~~~~~~~~~~~~~

vim +/device_reprobe +918 drivers/usb/core/driver.c

   907	
   908	static int __usb_bus_reprobe_drivers(struct device *dev, void *data)
   909	{
   910		struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
   911		struct usb_device *udev = to_usb_device(dev);
   912	
   913		if (dev->driver) {
   914			struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
   915	
   916			if (udriver == NULL || udriver == &usb_generic_driver) {
   917				udev->use_generic_driver = false;
 > 918				device_reprobe(dev);
   919			}
   920		} else {
   921			device_reprobe(dev);
   922		}
   923	
   924		return 0;
   925	}
   926	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50020 bytes --]

Re: [PATCH v3] USB: Fix device driver race

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2806 bytes --]

Hi Bastien,

I love your patch! Perhaps something to improve:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on peter.chen-usb/ci-for-usb-next balbi-usb/testing/next v5.8-rc6 next-20200723]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Bastien-Nocera/USB-Fix-device-driver-race/20200724-053320
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
config: x86_64-randconfig-a004-20200724 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project 1d09ecf36175f7910ffedd6d497c07b5c74c22fb)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/usb/core/driver.c:918:4: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]
                           device_reprobe(dev);
                           ^~~~~~~~~~~~~~ ~~~
   drivers/usb/core/driver.c:921:3: warning: ignoring return value of function declared with 'warn_unused_result' attribute [-Wunused-result]
                   device_reprobe(dev);
                   ^~~~~~~~~~~~~~ ~~~
   drivers/usb/core/driver.c:910:28: warning: unused variable 'udriver' [-Wunused-variable]
           struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
                                     ^
   3 warnings generated.

vim +/warn_unused_result +918 drivers/usb/core/driver.c

   907	
   908	static int __usb_bus_reprobe_drivers(struct device *dev, void *data)
   909	{
   910		struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
   911		struct usb_device *udev = to_usb_device(dev);
   912	
   913		if (dev->driver) {
   914			struct usb_device_driver *udriver = to_usb_device_driver(dev->driver);
   915	
   916			if (udriver == NULL || udriver == &usb_generic_driver) {
   917				udev->use_generic_driver = false;
 > 918				device_reprobe(dev);
   919			}
   920		} else {
   921			device_reprobe(dev);
   922		}
   923	
   924		return 0;
   925	}
   926	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35140 bytes --]

Re: [PATCH v3] USB: Fix device driver race

[Bastien Nocera]

On Thu, 2020-07-23 at 17:58 -0400, Alan Stern wrote:
> On Thu, Jul 23, 2020 at 11:30:39PM +0200, Bastien Nocera wrote:
> > When a new device with a specialised device driver is plugged in,
> > the
> > new driver will be modprobe()'d but the driver core will attach the
> > "generic" driver to the device.
> > 
> > After that, nothing will trigger a reprobe when the modprobe()'d
> > device
> > driver has finished initialising, as the device has the "generic"
> > driver attached to it.
> > 
> > Trigger a reprobe ourselves when new specialised drivers get
> > registered.
> > 
> > Fixes: 88b7381a939d
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> 
> Better than before, but there are still some things to improve.

That's kind of you to say, but it was really incredibly sloppy. I've
sent a v4 that only runs the reprobe on device that could be attached
to the new driver.

It was closer to an early version I made locally and didn't work
because I forgot that ->id_table could also be used for matching, and
because I was only running it when ->match was present, the reprobe was
never actually run...

This should all be fixed now.

Cheers