CHROMIUM: qcusbnet: fix devqmi_close() races.

drivers/net/usb/gobi/qmidevice.c

 /* qmidevice.c - gobi QMI device
  * Copyright (c) 2010, Code Aurora Forum. All rights reserved.
 
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 and
  * only version 2 as published by the Free Software Foundation.
 
  * This program is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
@@ skipping 52 matching lines @@
 static bool client_delread(struct qcusbnet *dev, u16 cid, u16 tid, void **data, u16 *size);
 static bool client_addnotify(struct qcusbnet *dev, u16 cid, u16 tid,
                              void (*hook)(struct qcusbnet *, u16 cid, void *),
                              void *data);
 static bool client_notify(struct qcusbnet *dev, u16 cid, u16 tid);
 static bool client_addurb(struct qcusbnet *dev, u16 cid, struct urb *urb);
 static struct urb *client_delurb(struct qcusbnet *dev, u16 cid);
 
 static int devqmi_open(struct inode *inode, struct file *file);
 static int devqmi_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg);
-static int devqmi_close(struct file *file, fl_owner_t ftable);
+static int devqmi_release(struct inode *inode, struct file *file);
 static ssize_t devqmi_read(struct file *file, char __user *buf, size_t size,
                            loff_t *pos);
 static ssize_t devqmi_write(struct file *file, const char __user *buf,
                             size_t size, loff_t *pos);
 
 static bool qmi_ready(struct qcusbnet *dev, u16 timeout);
 static void wds_callback(struct qcusbnet *dev, u16 cid, void *data);
 static int setup_wds_callback(struct qcusbnet *dev);
 static int qmidms_getmeid(struct qcusbnet *dev);
 
-#define IOCTL_QMI_GET_SERVICE_FILE      (0x8BE0 + 1)
-#define IOCTL_QMI_GET_DEVICE_VIDPID     (0x8BE0 + 2)
-#define IOCTL_QMI_GET_DEVICE_MEID       (0x8BE0 + 3)
+#define IOCTL_QMI_GET_SERVICE_FILE      (0x8BE0 + 1)
+#define IOCTL_QMI_GET_DEVICE_VIDPID     (0x8BE0 + 2)
+#define IOCTL_QMI_GET_DEVICE_MEID       (0x8BE0 + 3)
+#define IOCTL_QMI_CLOSE                 (0x8BE0 + 4)
 #define CDC_GET_ENCAPSULATED_RESPONSE   0x01A1ll
 #define CDC_CONNECTION_SPEED_CHANGE     0x08000000002AA1ll
 
 static const struct file_operations devqmi_fops = {
-        .owner = THIS_MODULE,
-        .read  = devqmi_read,
-        .write = devqmi_write,
-        .ioctl = devqmi_ioctl,
-        .open  = devqmi_open,
-        .flush = devqmi_close,
+        .owner   = THIS_MODULE,
+        .read    = devqmi_read,
+        .write   = devqmi_write,
+        .ioctl   = devqmi_ioctl,
+        .open    = devqmi_open,
+        .release = devqmi_release,
 };
 
 #ifdef CONFIG_SMP
 static inline void assert_locked(struct qcusbnet *dev)
 {
         BUG_ON(!spin_is_locked(&dev->qmi.clients_lock));
 }
 #else
 static inline void assert_locked(struct qcusbnet *dev)
 {
@@ skipping 565 matching lines @@
         struct urb *urb;
         void *data;
         u16 size;
         void *wbuf;
         size_t wbufsize;
         void *rbuf;
         u16 rbufsize;
         unsigned long flags;
         u8 tid;
 
-        if (!device_valid(dev)) {
-                DBG("invalid device\n");
-                return;
-        }
-
         DBG("releasing 0x%04X\n", cid);
 
         if (cid != QMICTL) {
                 tid = atomic_add_return(1, &dev->qmi.qmitid);
                 if (!tid)
                         tid = atomic_add_return(1, &dev->qmi.qmitid);
                 wbuf = qmictl_new_releasecid(tid, cid, &wbufsize);
                 if (!wbuf) {
                         DBG("memory error\n");
                 } else {
@@ skipping 281 matching lines @@
         struct qmihandle *handle = (struct qmihandle *)file->private_data;
 
         DBG("%p %04x %08x", handle, handle->cid, cmd);
 
         if (!handle) {
                 DBG("Bad file data\n");
                 return -EBADF;
         }
 
         if (!device_valid(handle->dev)) {
-                DBG("Invalid device! Updating f_ops\n");
-                file->f_op = file->f_dentry->d_inode->i_fop;
+                DBG("Invalid device!\n");
                 return -ENXIO;
         }
 
         switch (cmd) {
         case IOCTL_QMI_GET_SERVICE_FILE:
 
                 DBG("Setting up QMI for service %lu\n", arg);
                 if (!(u8)arg) {
                         DBG("Cannot use QMICTL from userspace\n");
                         return -EINVAL;
                 }
 
                 if (handle->cid != (u16)-1) {
                         DBG("Close the current connection before opening a new one\n");
                         return -EBADR;
                 }
 
                 result = client_alloc(handle->dev, (u8)arg);
                 if (result < 0)
                         return result;
                 handle->cid = result;
 
                 return 0;
                 break;
 
+        /* Okay, all aboard the nasty hack express. If we don't have this
+         * ioctl() (and we just rely on userspace to close() the file
+         * descriptors), if userspace has any refs left to this fd (like, say, a
+         * pending read()), then the read might hang around forever. Userspace
+         * needs a way to cause us to kick people off those waitqueues before
+         * closing the fd for good.
+         *
+         * If this driver used workqueues, the correct approach here would
+         * instead be to make the file descriptor select()able, and then just
+         * use select() instead of aio in userspace (thus allowing us to get
+         * away with one thread total and avoiding the recounting mess
+         * altogether).
+         */

[Mandeep Singh Baines, 2011/03/15 20:56:31]

I've read the comment but I'm still not getting the Use Case for this ioctl. Why
do you need kernel support to remove all refs? Can't you just send a signal to
any processes blocked on read. What is the requirement leading to this ioctl.

+        case IOCTL_QMI_CLOSE:
+                DBG("Tearing down QMI for service %lu", arg);
+                if (handle->cid == (u16)-1) {
+                        DBG("no qmi cid");
+                        return -EBADR;
+                }
+
+                file->private_data = NULL;
+                client_free(handle->dev, handle->cid);
+                kfree(handle);
+                return 0;
+                break;
 
         case IOCTL_QMI_GET_DEVICE_VIDPID:
                 if (!arg) {
                         DBG("Bad VIDPID buffer\n");
                         return -EINVAL;
                 }
 
                 if (!handle->dev->usbnet) {
                         DBG("Bad usbnet\n");
                         return -ENOMEM;
@@ skipping 24 matching lines @@
                 if (result)
                         DBG("copy to userspace failure\n");
 
                 return result;
                 break;
         default:
                 return -EBADRQC;
         }
 }
 
-static int devqmi_close(struct file *file, fl_owner_t ftable)
+static int devqmi_release(struct inode *inode, struct file *file)
 {
         struct qmihandle *handle = (struct qmihandle *)file->private_data;
-        struct task_struct *task;
-        struct fdtable *fdtable;
-        int count = 0;
-        int used = 0;
-        unsigned long flags;
-
-        if (!handle) {
-                DBG("bad file data\n");
-                return -EBADF;
-        }
-
-        if (file_count(file) != 1) {
-                rcu_read_lock();
-                for_each_process(task) {
-                        if (!task || !task->files)
-                                continue;
-                        spin_lock_irqsave(&task->files->file_lock, flags);
-                        fdtable = files_fdtable(task->files);
-                        for (count = 0; count < fdtable->max_fds; count++) {
-                                /* Before this function was called, this file was removed
-                                 * from our task's file table so if we find it in a file
-                                 * table then it is being used by another task
-                                 */
-                                if (fdtable->fd[count] == file) {
-                                        used++;
-                                        break;
-                                }
-                        }
-                        spin_unlock_irqrestore(&task->files->file_lock, flags);
-                }
-                rcu_read_unlock();
-
-                if (used > 0) {
-                        DBG("not closing, as this FD is open by %d other process\n", used);
-                        return 0;
-                }
-        }
-
-        if (!device_valid(handle->dev)) {
-                DBG("Invalid device! Updating f_ops\n");
-                file->f_op = file->f_dentry->d_inode->i_fop;
-                return -ENXIO;
-        }
-
-        DBG("0x%04X\n", handle->cid);
-
+        if (!handle)
+                return 0;
         file->private_data = NULL;
-
         if (handle->cid != (u16)-1)
                 client_free(handle->dev, handle->cid);
-
         kfree(handle);
         return 0;
 }
 
 static ssize_t devqmi_read(struct file *file, char __user *buf, size_t size,
                            loff_t *pos)
 {
         int result;
         void *data = NULL;
         void *smalldata;
         struct qmihandle *handle = (struct qmihandle *)file->private_data;
 
         if (!handle) {
                 DBG("Bad file data\n");
                 return -EBADF;
         }
 
         if (!device_valid(handle->dev)) {
-                DBG("Invalid device! Updating f_ops\n");
-                file->f_op = file->f_dentry->d_inode->i_fop;
+                DBG("Invalid device!\n");
                 return -ENXIO;
         }
 
         if (handle->cid == (u16)-1) {
                 DBG("Client ID must be set before reading 0x%04X\n",
                     handle->cid);
                 return -EBADR;
         }
 
         result = read_sync(handle->dev, &data, handle->cid, 0);
@@ skipping 137 matching lines @@
         struct list_head *node, *tmp;
         struct client *client;
         struct inode *inode;
         struct list_head *inodes;
         struct task_struct *task;
         struct fdtable *fdtable;
         struct file *file;
         unsigned long flags;
         int count = 0;
 
-        if (!device_valid(dev)) {
-                DBG("wrong device\n");
-                return;
-        }
-
         list_for_each_safe(node, tmp, &dev->qmi.clients) {
                 client = list_entry(node, struct client, node);
                 DBG("release 0x%04X\n", client->cid);
                 client_free(dev, client->cid);
         }
 
         qc_stopread(dev);
         dev->valid = false;
         list_for_each(inodes, &dev->qmi.cdev.list) {
                 inode = container_of(inodes, struct inode, i_devices);
@@ skipping 278 matching lines @@
                 DBG("bad get MEID resp\n");
                 memset(&dev->meid[0], '0', 14);
         }
 
         client_free(dev, cid);
         return 0;
 }
 
 module_param(qcusbnet2k_fwdelay, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(qcusbnet2k_fwdelay, "Delay for old firmware");