usbfs: fix deadlock on 'usbfs_mutex', clean up poll

The caller of usbfs_conn_disc_event() in some cases (but not always)
already holds usbfs_mutex, so trying to protect the event counter with
that lock causes nasty deadlocks.

The problem was introduced by commit 554f76962d ("USB: Remove BKL from
poll()") when the BLK protection was turned into using the mutex instead.

So fix this by using an atomic variable instead.  And while we're at it,
get rid of the atrocious naming of said variable and the waitqueue it is
associated with.

This also cleans up the unnecessary locking in the poll routine, since
the whole point of how the pollwait table works is that you can just add
yourself to the waiting list, and then check the condition you're
waiting for afterwards - avoiding all races.

It also gets rid of the unnecessary dynamic allocation of the device
status that just contained a single word.  We should use f_version for
this, as Dmitry Torokhov points out.  That simplifies everything
further.

Reported-and-tested-by: Jeff Chua <jeff.chua.linux@gmail.com>
Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
Acked-by: Alan Stern <stern@rowland.harvard.edu>
Cc: Oliver Neukum <oliver@neukum.org>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>

diff --git a/drivers/usb/core/devices.c b/drivers/usb/core/devices.c
index c83c975..6053a8b 100644 (file)
--- a/drivers/usb/core/devices.c
+++ b/drivers/usb/core/devices.c
@@ -117,9 +117,21 @@ static const char *format_endpt =
  * However, these will come from functions that return ptrs to each of them.
  */
 
-static DECLARE_WAIT_QUEUE_HEAD(deviceconndiscwq);
-/* guarded by usbfs_mutex */
-static unsigned int conndiscevcnt;
+/*
+ * Wait for an connect/disconnect event to happen. We initialize
+ * the event counter with an odd number, and each event will increment
+ * the event counter by two, so it will always _stay_ odd. That means
+ * that it will never be zero, so "event 0" will never match a current
+ * event, and thus 'poll' will always trigger as readable for the first
+ * time it gets called.
+ */
+static struct device_connect_event {
+       atomic_t count;
+       wait_queue_head_t wait;
+} device_event = {
+       .count = ATOMIC_INIT(1),
+       .wait = __WAIT_QUEUE_HEAD_INITIALIZER(device_event.wait)
+};
 
 /* this struct stores the poll state for <mountpoint>/devices pollers */
 struct usb_device_status {
@@ -157,10 +169,8 @@ static const struct class_info clas_info[] =
 
 void usbfs_conn_disc_event(void)
 {
-       mutex_lock(&usbfs_mutex);
-       conndiscevcnt++;
-       mutex_unlock(&usbfs_mutex);
-       wake_up(&deviceconndiscwq);
+       atomic_add(2, &device_event.count);
+       wake_up(&device_event.wait);
 }
 
 static const char *class_decode(const int class)
@@ -632,42 +642,16 @@ static ssize_t usb_device_read(struct file *file, char __user *buf,
 static unsigned int usb_device_poll(struct file *file,
                                    struct poll_table_struct *wait)
 {
-       struct usb_device_status *st;
-       unsigned int mask = 0;
-
-       mutex_lock(&usbfs_mutex);
-       st = file->private_data;
-       if (!st) {
-               st = kmalloc(sizeof(struct usb_device_status), GFP_KERNEL);
-               if (!st) {
-                       mutex_unlock(&usbfs_mutex);
-                       return POLLIN;
-               }
-
-               st->lastev = conndiscevcnt;
-               file->private_data = st;
-               mask = POLLIN;
-       }
+       unsigned int event_count;
 
-       if (file->f_mode & FMODE_READ)
-               poll_wait(file, &deviceconndiscwq, wait);
-       if (st->lastev != conndiscevcnt)
-               mask |= POLLIN;
-       st->lastev = conndiscevcnt;
-       mutex_unlock(&usbfs_mutex);
-       return mask;
-}
+       poll_wait(file, &device_event.wait, wait);
 
-static int usb_device_open(struct inode *inode, struct file *file)
-{
-       file->private_data = NULL;
-       return 0;
-}
+       event_count = atomic_read(&device_event.count);
+       if (file->f_version != event_count) {
+               file->f_version = event_count;
+               return POLLIN | POLLRDNORM;
+       }
 
-static int usb_device_release(struct inode *inode, struct file *file)
-{
-       kfree(file->private_data);
-       file->private_data = NULL;
        return 0;
 }
 
@@ -699,6 +683,4 @@ const struct file_operations usbfs_devices_fops = {
        .llseek =       usb_device_lseek,
        .read =         usb_device_read,
        .poll =         usb_device_poll,
-       .open =         usb_device_open,
-       .release =      usb_device_release,
 };