[PATCH 3/3 v5] USB: Fix device driver race

[PATCH 3/3 v5] 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 ("USB: Select better matching USB drivers when available")
Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
Changes since v4:
- Add commit subject to "fixes" section
- Clarify conditional that checks for generic driver
- Remove check duplicated inside the loop

Changes since v3:
- Only reprobe devices that could use the new driver
- Many code fixes

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 | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
index f81606c6a35b..7d3878aa8090 100644
--- a/drivers/usb/core/driver.c
+++ b/drivers/usb/core/driver.c
@@ -905,6 +905,32 @@ static int usb_uevent(struct device *dev, struct kobj_uevent_env *env)
 	return 0;
 }
 
+static bool is_dev_usb_generic_driver(struct device *dev)
+{
+	struct usb_device_driver *udd = dev->driver ?
+		to_usb_device_driver(dev->driver) : NULL;
+
+	return udd == &usb_generic_driver;
+}
+
+static int __usb_bus_reprobe_drivers(struct device *dev, void *data)
+{
+	struct usb_device_driver *new_udriver = data;
+	struct usb_device *udev;
+
+	if (!is_dev_usb_generic_driver(dev))
+		return 0;
+
+	udev = to_usb_device(dev);
+	if (usb_device_match_id(udev, new_udriver->id_table) == NULL &&
+	    (!new_udriver->match || new_udriver->match(udev) != 0))
+		return 0;
+
+	(void)!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 +960,20 @@ int usb_register_device_driver(struct usb_device_driver *new_udriver,
 
 	retval = driver_register(&new_udriver->drvwrap.driver);
 
-	if (!retval)
+	if (!retval) {
 		pr_info("%s: registered new device driver %s\n",
 			usbcore_name, new_udriver->name);
-	else
+		/*
+		 * Check whether any device could be better served with
+		 * this new driver
+		 */
+		bus_for_each_dev(&usb_bus_type, NULL, new_udriver,
+				 __usb_bus_reprobe_drivers);
+	} else {
 		printk(KERN_ERR "%s: error %d registering device "
 			"	driver %s\n",
 			usbcore_name, retval, new_udriver->name);
+	}
 
 	return retval;
 }

Re: [PATCH 3/3 v5] USB: Fix device driver race

[Alan Stern]

On Sat, Jul 25, 2020 at 11:16:47AM +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 ("USB: Select better matching USB drivers when available")
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
> Changes since v4:
> - Add commit subject to "fixes" section
> - Clarify conditional that checks for generic driver
> - Remove check duplicated inside the loop
> 
> Changes since v3:
> - Only reprobe devices that could use the new driver
> - Many code fixes
> 
> 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 | 37 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/core/driver.c b/drivers/usb/core/driver.c
> index f81606c6a35b..7d3878aa8090 100644
> --- a/drivers/usb/core/driver.c
> +++ b/drivers/usb/core/driver.c
> @@ -905,6 +905,32 @@ static int usb_uevent(struct device *dev, struct kobj_uevent_env *env)
>  	return 0;
>  }
>  
> +static bool is_dev_usb_generic_driver(struct device *dev)
> +{
> +	struct usb_device_driver *udd = dev->driver ?
> +		to_usb_device_driver(dev->driver) : NULL;
> +
> +	return udd == &usb_generic_driver;
> +}

Heh...  I don't recommend this optimization because it's a little 
unclear, but the function can be shortened to:

	return dev->driver == &usb_generic_driver.drvwrap.driver;

> +
> +static int __usb_bus_reprobe_drivers(struct device *dev, void *data)
> +{
> +	struct usb_device_driver *new_udriver = data;
> +	struct usb_device *udev;
> +
> +	if (!is_dev_usb_generic_driver(dev))
> +		return 0;
> +
> +	udev = to_usb_device(dev);
> +	if (usb_device_match_id(udev, new_udriver->id_table) == NULL &&
> +	    (!new_udriver->match || new_udriver->match(udev) != 0))
> +		return 0;
> +
> +	(void)!device_reprobe(dev);

What's that '!' doing hiding in there?  It doesn't affect the final 
outcome, but it sure looks weird -- if people notice it at all.

Aside from that,

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

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

Re: [PATCH 3/3 v5] USB: Fix device driver race

[Bastien Nocera]

On Sat, 2020-07-25 at 10:59 -0400, Alan Stern wrote:
<snip>
> > +	udev = to_usb_device(dev);
> > +	if (usb_device_match_id(udev, new_udriver->id_table) == NULL &&
> > +	    (!new_udriver->match || new_udriver->match(udev) != 0))
> > +		return 0;
> > +
> > +	(void)!device_reprobe(dev);
> 
> What's that '!' doing hiding in there?  It doesn't affect the final 
> outcome, but it sure looks weird -- if people notice it at all.

It's how we stop gcc from complaining about the warn_unused_result
attribute on device_reprobe()... (void) is enough with clang, but not
with gcc.

> Aside from that,
> 
> Acked-by: Alan Stern <stern@rowland.harvard.edu>

Thanks!

Re: [PATCH 3/3 v5] USB: Fix device driver race

[Alan Stern]

On Sat, Jul 25, 2020 at 05:24:20PM +0200, Bastien Nocera wrote:
> On Sat, 2020-07-25 at 10:59 -0400, Alan Stern wrote:
> <snip>
> > > +	udev = to_usb_device(dev);
> > > +	if (usb_device_match_id(udev, new_udriver->id_table) == NULL &&
> > > +	    (!new_udriver->match || new_udriver->match(udev) != 0))
> > > +		return 0;
> > > +
> > > +	(void)!device_reprobe(dev);
> > 
> > What's that '!' doing hiding in there?  It doesn't affect the final 
> > outcome, but it sure looks weird -- if people notice it at all.
> 
> It's how we stop gcc from complaining about the warn_unused_result
> attribute on device_reprobe()... (void) is enough with clang, but not
> with gcc.

Hmmm.  Maybe this is an indication that device_reprobe() doesn't really 
need to be __must_check.

Greg, do you know why it's annotated this way?

Alan Stern

Re: [PATCH 3/3 v5] USB: Fix device driver race

[Greg Kroah-Hartman]

On Sat, Jul 25, 2020 at 03:57:07PM -0400, Alan Stern wrote:
> On Sat, Jul 25, 2020 at 05:24:20PM +0200, Bastien Nocera wrote:
> > On Sat, 2020-07-25 at 10:59 -0400, Alan Stern wrote:
> > <snip>
> > > > +	udev = to_usb_device(dev);
> > > > +	if (usb_device_match_id(udev, new_udriver->id_table) == NULL &&
> > > > +	    (!new_udriver->match || new_udriver->match(udev) != 0))
> > > > +		return 0;
> > > > +
> > > > +	(void)!device_reprobe(dev);
> > > 
> > > What's that '!' doing hiding in there?  It doesn't affect the final 
> > > outcome, but it sure looks weird -- if people notice it at all.
> > 
> > It's how we stop gcc from complaining about the warn_unused_result
> > attribute on device_reprobe()... (void) is enough with clang, but not
> > with gcc.
> 
> Hmmm.  Maybe this is an indication that device_reprobe() doesn't really 
> need to be __must_check.
> 
> Greg, do you know why it's annotated this way?

Because you really should pass up the return value if an error happens
here.  Why do we think it is safe to ignore?

And that "(void)!" is not ok, if the annotation is safe to ignore, then
we need to remove the annotation, don't work around stuff like this
without at the very least, a comment saying why it is ok.

thanks,

greg k-h

Re: [PATCH 3/3 v5] USB: Fix device driver race

[Alan Stern]

On Sun, Jul 26, 2020 at 10:36:55AM +0200, Greg Kroah-Hartman wrote:
> On Sat, Jul 25, 2020 at 03:57:07PM -0400, Alan Stern wrote:
> > On Sat, Jul 25, 2020 at 05:24:20PM +0200, Bastien Nocera wrote:
> > > On Sat, 2020-07-25 at 10:59 -0400, Alan Stern wrote:
> > > <snip>
> > > > > +	udev = to_usb_device(dev);
> > > > > +	if (usb_device_match_id(udev, new_udriver->id_table) == NULL &&
> > > > > +	    (!new_udriver->match || new_udriver->match(udev) != 0))
> > > > > +		return 0;
> > > > > +
> > > > > +	(void)!device_reprobe(dev);
> > > > 
> > > > What's that '!' doing hiding in there?  It doesn't affect the final 
> > > > outcome, but it sure looks weird -- if people notice it at all.
> > > 
> > > It's how we stop gcc from complaining about the warn_unused_result
> > > attribute on device_reprobe()... (void) is enough with clang, but not
> > > with gcc.
> > 
> > Hmmm.  Maybe this is an indication that device_reprobe() doesn't really 
> > need to be __must_check.
> > 
> > Greg, do you know why it's annotated this way?
> 
> Because you really should pass up the return value if an error happens
> here.  Why do we think it is safe to ignore?
> 
> And that "(void)!" is not ok, if the annotation is safe to ignore, then
> we need to remove the annotation, don't work around stuff like this
> without at the very least, a comment saying why it is ok.

I suppose Bastien could log an error message at that point.  There isn't 
much else to do.

Alan Stern

Re: [PATCH 3/3 v5] USB: Fix device driver race

[Greg Kroah-Hartman]

On Sun, Jul 26, 2020 at 10:17:08AM -0400, Alan Stern wrote:
> On Sun, Jul 26, 2020 at 10:36:55AM +0200, Greg Kroah-Hartman wrote:
> > On Sat, Jul 25, 2020 at 03:57:07PM -0400, Alan Stern wrote:
> > > On Sat, Jul 25, 2020 at 05:24:20PM +0200, Bastien Nocera wrote:
> > > > On Sat, 2020-07-25 at 10:59 -0400, Alan Stern wrote:
> > > > <snip>
> > > > > > +	udev = to_usb_device(dev);
> > > > > > +	if (usb_device_match_id(udev, new_udriver->id_table) == NULL &&
> > > > > > +	    (!new_udriver->match || new_udriver->match(udev) != 0))
> > > > > > +		return 0;
> > > > > > +
> > > > > > +	(void)!device_reprobe(dev);
> > > > > 
> > > > > What's that '!' doing hiding in there?  It doesn't affect the final 
> > > > > outcome, but it sure looks weird -- if people notice it at all.
> > > > 
> > > > It's how we stop gcc from complaining about the warn_unused_result
> > > > attribute on device_reprobe()... (void) is enough with clang, but not
> > > > with gcc.
> > > 
> > > Hmmm.  Maybe this is an indication that device_reprobe() doesn't really 
> > > need to be __must_check.
> > > 
> > > Greg, do you know why it's annotated this way?
> > 
> > Because you really should pass up the return value if an error happens
> > here.  Why do we think it is safe to ignore?
> > 
> > And that "(void)!" is not ok, if the annotation is safe to ignore, then
> > we need to remove the annotation, don't work around stuff like this
> > without at the very least, a comment saying why it is ok.
> 
> I suppose Bastien could log an error message at that point.  There isn't 
> much else to do.

It looks like no one does anything with the return value of that
function, so we should just change it to void and stop checking it
entirely :(

greg k-h