Attachment 272295 Details for Bug 431192

[patch] Patch fixing similarly looking problem in SLES10

usbfs-private-mutex-for-open-release-and-remove.patch (text/plain), 4.55 KB, created by Jan Kara on 2009-02-12 14:09:05 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jan Kara

Created: 2009-02-12 14:09:05 UTC

Size: 4.55 KB

patch

obsolete

>From 4a2a8a2cce86b9d001378cc25acb5c61e6ca7d63 Mon Sep 17 00:00:00 2001
>From: Alan Stern <stern@rowland.harvard.edu>
>Date: Sat, 1 Jul 2006 22:05:01 -0400
>Subject: usbfs: private mutex for open, release, and remove
>References: bnc#420059
>Patch-mainline: 2.6.19
>
>
>From: Alan Stern <stern@rowland.harvard.edu>
>
>commit 4a2a8a2cce86b9d001378cc25acb5c61e6ca7d63 upstream.
>
>The usbfs code doesn't provide sufficient mutual exclusion among open,
>release, and remove.  Release vs. remove is okay because they both
>acquire the device lock, but open is not exclusive with either one.  All
>three routines modify the udev->filelist linked list, so they must not
>run concurrently.
>
>Apparently someone gave this a minimum amount of thought in the past by
>explicitly acquiring the BKL at the start of the usbdev_open routine.
>Oddly enough, there's a comment pointing out that locking is unnecessary
>because chrdev_open already has acquired the BKL.
>
>But this ignores the point that the files in /proc/bus/usb/* are not
>char device files; they are regular files and so they don't get any
>special locking.  Furthermore it's necessary to acquire the same lock in
>the release and remove routines, which the code does not do.
>
>Yet another problem arises because the same file_operations structure is
>accessible through both the /proc/bus/usb/* and /dev/usb/usbdev* file
>nodes.  Even when one of them has been removed, it's still possible for
>userspace to open the other.  So simple locking around the individual
>remove routines is insufficient; we need to lock the entire
>usb_notify_remove_device notifier chain.
>
>Rather than rely on the BKL, this patch (as723) introduces a new private
>mutex for the purpose.  Holding the BKL while invoking a notifier chain
>doesn't seem like a good idea.
>
>
>Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
>Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de>
>
>---
> drivers/usb/core/devio.c  |   26 +++++++++++++++-----------
> drivers/usb/core/notify.c |    3 +++
> drivers/usb/core/usb.h    |    1 +
> 3 files changed, 19 insertions(+), 11 deletions(-)
>
>--- a/drivers/usb/core/devio.c
>+++ b/drivers/usb/core/devio.c
>@@ -58,6 +58,9 @@
> #define USB_DEVICE_MAX			USB_MAXBUS * 128
> static struct class *usb_device_class;
> 
>+/* Mutual exclusion for removal, open, and release */
>+DEFINE_MUTEX(usbfs_mutex);
>+
> struct async {
> 	struct list_head asynclist;
> 	struct dev_state *ps;
>@@ -543,15 +546,13 @@ static int usbdev_open(struct inode *ino
> 	struct dev_state *ps;
> 	int ret;
> 
>-	/* 
>-	 * no locking necessary here, as chrdev_open has the kernel lock
>-	 * (still acquire the kernel lock for safety)
>-	 */
>+	/* Protect against simultaneous removal or release */
>+	mutex_lock(&usbfs_mutex);
>+
> 	ret = -ENOMEM;
> 	if (!(ps = kmalloc(sizeof(struct dev_state), GFP_KERNEL)))
>-		goto out_nolock;
>+		goto out;
> 
>-	lock_kernel();
> 	ret = -ENOENT;
> 	/* check if we are called from a real node or usbfs */
> 	if (imajor(inode) == USB_DEVICE_MAJOR)
>@@ -580,9 +581,8 @@ static int usbdev_open(struct inode *ino
> 	list_add_tail(&ps->list, &dev->filelist);
> 	file->private_data = ps;
>  out:
>-	unlock_kernel();
>- out_nolock:
>-        return ret;
>+	mutex_unlock(&usbfs_mutex);
>+	return ret;
> }
> 
> static int usbdev_release(struct inode *inode, struct file *file)
>@@ -592,7 +592,12 @@ static int usbdev_release(struct inode *
> 	unsigned int ifnum;
> 
> 	usb_lock_device(dev);
>+
>+	/* Protect against simultaneous open */
>+	mutex_lock(&usbfs_mutex);
> 	list_del_init(&ps->list);
>+	mutex_unlock(&usbfs_mutex);
>+
> 	for (ifnum = 0; ps->ifclaimed && ifnum < 8*sizeof(ps->ifclaimed);
> 			ifnum++) {
> 		if (test_bit(ifnum, &ps->ifclaimed))
>@@ -601,9 +606,8 @@ static int usbdev_release(struct inode *
> 	destroy_all_async(ps);
> 	usb_unlock_device(dev);
> 	usb_put_dev(dev);
>-	ps->dev = NULL;
> 	kfree(ps);
>-        return 0;
>+	return 0;
> }
> 
> static int proc_control(struct dev_state *ps, void __user *arg)
>--- a/drivers/usb/core/notify.c
>+++ b/drivers/usb/core/notify.c
>@@ -100,7 +100,10 @@ void usb_notify_add_device(struct usb_de
> 
> void usb_notify_remove_device(struct usb_device *udev)
> {
>+	/* Protect against simultaneous usbfs open */
>+	mutex_lock(&usbfs_mutex);
> 	usb_notifier_call_chain(&usb_notifier_list, USB_DEVICE_REMOVE, udev);
>+	mutex_unlock(&usbfs_mutex);
> }
> 
> void usb_notify_add_bus(struct usb_bus *ubus)
>--- a/drivers/usb/core/usb.h
>+++ b/drivers/usb/core/usb.h
>@@ -56,6 +56,7 @@ static inline int is_active(struct usb_i
> extern const char *usbcore_name;
> 
> /* usbfs stuff */
>+extern struct mutex usbfs_mutex;
> extern struct usb_driver usbfs_driver;
> extern struct file_operations usbfs_devices_fops;
> extern struct file_operations usbfs_device_file_operations;

Actions: View | Diff

Attachments on bug 431192: 242744 | 242756 | 244539 | 245617 | 272295