[PATCH 1/1] khubd -- switch USB product/manufacturer/serial handling to RCU

[PATCH 1/1] khubd -- switch USB product/manufacturer/serial handling to RCU

[Andy Whitcroft]

With the introduction of wireless USB hubs the product, manufacturer,
and serial number are now mutable.  This necessitates new locking in the
consumers of these values including the sysfs read routines in order to
prevent use-after-free acces to these values.  These extra locks create
significant lock contention leading to increased boot times (0.3s for an
example Atom based system).  Move update of these values to RCU based
locking.

BugLink: http://bugs.launchpad.net/bugs/510937
Signed-off-by: Andy Whitcroft <apw@canonical.com>
---
 drivers/usb/core/hub.c   |   34 ++++++++++++++++++++++++++++------
 drivers/usb/core/sysfs.c |    6 +++---
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 83e7bbb..6967421 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -24,6 +24,7 @@
 #include <linux/mutex.h>
 #include <linux/freezer.h>
 #include <linux/pm_runtime.h>
+#include <linux/rcupdate.h>
 
 #include <asm/uaccess.h>
 #include <asm/byteorder.h>
@@ -1849,6 +1850,10 @@ fail:
  */
 int usb_deauthorize_device(struct usb_device *usb_dev)
 {
+	char *product = NULL;
+	char *manufacturer = NULL;
+	char *serial = NULL;
+
 	usb_lock_device(usb_dev);
 	if (usb_dev->authorized == 0)
 		goto out_unauthorized;
@@ -1856,11 +1861,12 @@ int usb_deauthorize_device(struct usb_device *usb_dev)
 	usb_dev->authorized = 0;
 	usb_set_configuration(usb_dev, -1);
 
-	kfree(usb_dev->product);
+	product = usb_dev->product;
+	manufacturer = usb_dev->manufacturer;
+	serial = usb_dev->serial;
+
 	usb_dev->product = kstrdup("n/a (unauthorized)", GFP_KERNEL);
-	kfree(usb_dev->manufacturer);
 	usb_dev->manufacturer = kstrdup("n/a (unauthorized)", GFP_KERNEL);
-	kfree(usb_dev->serial);
 	usb_dev->serial = kstrdup("n/a (unauthorized)", GFP_KERNEL);
 
 	usb_destroy_configuration(usb_dev);
@@ -1868,6 +1874,12 @@ int usb_deauthorize_device(struct usb_device *usb_dev)
 
 out_unauthorized:
 	usb_unlock_device(usb_dev);
+	if (product || manufacturer || serial) {
+		synchronize_rcu();
+		kfree(product);
+		kfree(manufacturer);
+		kfree(serial);
+	}
 	return 0;
 }
 
@@ -1875,6 +1887,9 @@ out_unauthorized:
 int usb_authorize_device(struct usb_device *usb_dev)
 {
 	int result = 0, c;
+	char *product = NULL;
+	char *manufacturer = NULL;
+	char *serial = NULL;
 
 	usb_lock_device(usb_dev);
 	if (usb_dev->authorized == 1)
@@ -1893,11 +1908,12 @@ int usb_authorize_device(struct usb_device *usb_dev)
 		goto error_device_descriptor;
 	}
 
-	kfree(usb_dev->product);
+	product = usb_dev->product;
+	manufacturer = usb_dev->manufacturer;
+	serial = usb_dev->serial;
+
 	usb_dev->product = NULL;
-	kfree(usb_dev->manufacturer);
 	usb_dev->manufacturer = NULL;
-	kfree(usb_dev->serial);
 	usb_dev->serial = NULL;
 
 	usb_dev->authorized = 1;
@@ -1925,6 +1941,12 @@ error_device_descriptor:
 error_autoresume:
 out_authorized:
 	usb_unlock_device(usb_dev);	// complements locktree
+	if (product || manufacturer || serial) {
+		synchronize_rcu();
+		kfree(product);
+		kfree(manufacturer);
+		kfree(serial);
+	}
 	return result;
 }
 
diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index 448f5b4..98856d8 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -85,9 +85,9 @@ static ssize_t  show_##name(struct device *dev,				\
 	int retval;							\
 									\
 	udev = to_usb_device(dev);					\
-	usb_lock_device(udev);						\
-	retval = sprintf(buf, "%s\n", udev->name);			\
-	usb_unlock_device(udev);					\
+	rcu_read_lock();						\
+	retval = sprintf(buf, "%s\n", rcu_dereference(udev->name));	\
+	rcu_read_unlock();						\
 	return retval;							\
 }									\
 static DEVICE_ATTR(name, S_IRUGO, show_##name, NULL);
-- 
1.7.0.4

Re: [PATCH 1/1] khubd -- switch USB product/manufacturer/serial handling to RCU

[Greg KH]

On Mon, Jun 14, 2010 at 12:15:41PM +0100, Andy Whitcroft wrote:
> With the introduction of wireless USB hubs the product, manufacturer,
> and serial number are now mutable.

Changable by whom?  The device itself?  This has always been the case,
although it is usually very rare for a device to do this.

> This necessitates new locking in the gconsumers of these values
> including the sysfs read routines in order to prevent use-after-free
> acces to these values.

Who would access them after they go away?

> These extra locks create significant lock contention leading to
> increased boot times (0.3s for an example Atom based system).

Who is doing all of the deauthorization at boot time that this would
ever matter at all?

> Move update of these values to RCU based locking.

Ick.  What has changed that necessitates this?  You discuss a variety of
different things in this bug report:

> BugLink: http://bugs.launchpad.net/bugs/510937

Which would have been nice to dicuss with the people on these lists.  I
really don't think this is the correct fix, as a normal boot path
shouldn't be having non-authorized usb devices, right?  And from that
bug, once the machine is up and running, there is no contention here,
right?

not convinced,

greg k-h

Re: [PATCH 1/1] khubd -- switch USB product/manufacturer/serial handling to RCU

[Andy Whitcroft]

On Mon, Jun 14, 2010 at 09:29:19AM -0700, Greg KH wrote:
> On Mon, Jun 14, 2010 at 12:15:41PM +0100, Andy Whitcroft wrote:
> > With the introduction of wireless USB hubs the product, manufacturer,
> > and serial number are now mutable.
> 
> Changable by whom?  The device itself?  This has always been the case,
> although it is usually very rare for a device to do this.
> 
> > This necessitates new locking in the gconsumers of these values
> > including the sysfs read routines in order to prevent use-after-free
> > acces to these values.
> 
> Who would access them after they go away?

I think you have miss-understood what this is trying to say.  This is
trying to state the current state of play.  I am trying to say that
previous commits which introduced wireless hubs made these string
fungible and fungible while the device is present and registered in sysfs.
Those commits necessarily added the locking to the sysfs files to ensure
safe reading of these values.

> > These extra locks create significant lock contention leading to
> > increased boot times (0.3s for an example Atom based system).
> 
> Who is doing all of the deauthorization at boot time that this would
> ever matter at all?
> > Move update of these values to RCU based locking.
> 
> Ick.  What has changed that necessitates this?  You discuss a variety of
> different things in this bug report:
> 
> > BugLink: http://bugs.launchpad.net/bugs/510937
> 
> Which would have been nice to dicuss with the people on these lists.  I
> really don't think this is the correct fix, as a normal boot path
> shouldn't be having non-authorized usb devices, right?  And from that
> bug, once the machine is up and running, there is no contention here,
> right?

It is not that these values are changing at all, indeed they are not.
It is that because they could, should the device be of a type which can
authorise/deauthorise at any time, that these strings may come and go
at any time and thus sysfs read accesses now already are locked against
these changes.

The contention that is triggered on boot is triggered by udev probing
in parallel across the usb hubs.  The lock used to protect these string
values is the device lock and is held for some time during hub probes and
prevents udev from reading these strings during that time.  This lead to
significant spin times for udev and lead to a .3s to .5s increase in device
initialisation, approximatly 10% of the overal device initialisation time
of 4s.  This was a significant boot performance regression for us.

In the first instance we confirmed that it was the lock on those string
by simply removing the locking, which was not required at all in the
static hub case.  But we need some protection here for the wireless case.
It seemed appropriate as these are read-mostly values and changed very
rarely (if at all) to switch these over to RCU locking rather than
introduce a new lock for them.

They key concern here is to split the locking between the device lock
and the string locking to prevent these needless boot stalls.  I am not
wedded to any particular solution.

-apw