ChangeSet 1.1371.759.40, 2004/04/28 11:52:21-07:00, stern@rowland.harvard.edu

[PATCH] USB: Lock devices during tree traversal

On Tue, 27 Apr 2004, Greg KH wrote:

> So, what's next in this patch series?  :)

Funny you should ask...

While writing those patches I noted a problem, that the USB device tree
can change while a process reading /proc/bus/usb/devices is traversing it,
leading to an oops when a pointer to a no-longer-existing child device is
dereferenced.  The ensuing discussion led to the conclusion that the
devices' ->serialize locks should be acquired, top-down, while going
through the tree.

That means changing the code that populates the devices file and changing
the code that adds and removes USB device structures.  This patch takes
care of the first part.  I'm delaying the second part because that section
of usbcore is still under change -- David Brownell's revisions have not
yet been fully integrated.

A similar change should be made to usb_find_device() and match_device() in
usb.c.  You may want to add that yourself.


 drivers/usb/core/devices.c |   12 ++++++++++--
 1 files changed, 10 insertions(+), 2 deletions(-)


diff -Nru a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
--- a/drivers/usb/core/devices.c	Fri May 14 15:31:37 2004
+++ b/drivers/usb/core/devices.c	Fri May 14 15:31:37 2004
@@ -452,6 +452,7 @@
  * nbytes - the maximum number of bytes to write
  * skip_bytes - the number of bytes to skip before writing anything
  * file_offset - the offset into the devices file on completion
+ * The caller must own the usbdev->serialize semaphore.
  */
 static ssize_t usb_device_dump(char __user **buffer, size_t *nbytes, loff_t *skip_bytes, loff_t *file_offset,
 				struct usb_device *usbdev, struct usb_bus *bus, int level, int index, int count)
@@ -552,9 +553,13 @@
 	
 	/* Now look at all of this device's children. */
 	for (chix = 0; chix < usbdev->maxchild; chix++) {
-		if (usbdev->children[chix]) {
-			ret = usb_device_dump(buffer, nbytes, skip_bytes, file_offset, usbdev->children[chix],
+		struct usb_device *childdev = usbdev->children[chix];
+
+		if (childdev) {
+			down(&childdev->serialize);
+			ret = usb_device_dump(buffer, nbytes, skip_bytes, file_offset, childdev,
 					bus, level + 1, chix, ++cnt);
+			up(&childdev->serialize);
 			if (ret == -EFAULT)
 				return total_written;
 			total_written += ret;
@@ -582,8 +587,11 @@
 	for (buslist = usb_bus_list.next; buslist != &usb_bus_list; buslist = buslist->next) {
 		/* print devices for this bus */
 		bus = list_entry(buslist, struct usb_bus, bus_list);
+
 		/* recurse through all children of the root hub */
+		down(&bus->root_hub->serialize);
 		ret = usb_device_dump(&buf, &nbytes, &skip_bytes, ppos, bus->root_hub, bus, 0, 0, 0);
+		up(&bus->root_hub->serialize);
 		if (ret < 0) {
 			up(&usb_bus_list_lock);
 			return ret;