CHROMIUM: gobi: Fix races in qc_deregister() once and for all.

drivers/net/usb/gobi/qmidevice.c

Index: drivers/net/usb/gobi/qmidevice.c
diff --git a/drivers/net/usb/gobi/qmidevice.c b/drivers/net/usb/gobi/qmidevice.c
index 33df8261612cb3d32a8846516fe13cae4d9ad49c..6abf7c4fcefa6126c53bf65ca0c031497876a7e4 100644
--- a/drivers/net/usb/gobi/qmidevice.c
+++ b/drivers/net/usb/gobi/qmidevice.c
@@ -732,7 +732,6 @@ static void client_free(struct qcusbnet *dev, u16 cid)
 			kfree(client);
 		}
 	}
-
 	spin_unlock_irqrestore(&dev->qmi.clients_lock, flags);
 }
 
@@ -946,10 +945,13 @@ static int devqmi_open(struct inode *inode, struct file *file)
 	struct qmidev *qmidev = container_of(inode->i_cdev, struct qmidev, cdev);
 	struct qcusbnet *dev = container_of(qmidev, struct qcusbnet, qmi);
 
-	if (!device_valid(dev)) {
-		DBG("Invalid device\n");
+	/* We need an extra ref on the device per fd, since we stash a ref
+	 * inside the handle. If qcusbnet_get() returns NULL, that means the
+	 * device has been removed from the list - no new refs for us. */
+	struct qcusbnet *ref = qcusbnet_get(dev);
+
+	if (!ref)
 		return -ENXIO;
-	}
 
 	file->private_data = kmalloc(sizeof(struct qmihandle), GFP_KERNEL);
 	if (!file->private_data) {
@@ -959,7 +961,7 @@ static int devqmi_open(struct inode *inode, struct file *file)
 
 	handle = (struct qmihandle *)file->private_data;
 	handle->cid = (u16)-1;
-	handle->dev = dev;
+	handle->dev = ref;
 
 	DBG("%p %04x", handle, handle->cid);
 
@@ -980,6 +982,11 @@ static int devqmi_ioctl(struct inode *inode, struct file *file, unsigned int cmd
 		return -EBADF;
 	}
 
+	if (handle->dev->dying) {

[Mandeep Singh Baines, 2011/03/16 20:49:41]

Why isn't device_valid sufficient?

[Elly Fong-Jones, 2011/03/16 21:23:09]

To avoid racing against the loop in qc_deregister() that frees all the clients.
Basically, if someone can set up a new client and then have the ->valid flag set
to false with that client still live, it can never be freed, because all our
internal functions bail if ->valid is false, including the ones we'd need to
tear down the client context. Fixing this is on my TODO list for the future, but
is not a small task.

+		DBG("Dying device");
+		return -ENXIO;
+	}
+
 	if (!device_valid(handle->dev)) {
 		DBG("Invalid device!\n");
 		return -ENXIO;
@@ -1084,6 +1091,7 @@ static int devqmi_release(struct inode *inode, struct file *file)
 	file->private_data = NULL;
 	if (handle->cid != (u16)-1)
 		client_free(handle->dev, handle->cid);
+	qcusbnet_put(handle->dev);
 	kfree(handle);
 	return 0;
 }
@@ -1101,6 +1109,11 @@ static ssize_t devqmi_read(struct file *file, char __user *buf, size_t size,
 		return -EBADF;
 	}
 
+	if (handle->dev->dying) {
+		DBG("Dying device");
+		return -ENXIO;
+	}
+
 	if (!device_valid(handle->dev)) {
 		DBG("Invalid device!\n");
 		return -ENXIO;
@@ -1185,6 +1198,7 @@ int qc_register(struct qcusbnet *dev)
 	char *name;
 
 	dev->valid = true;
+	dev->dying = false;
 	result = client_alloc(dev, QMICTL);
 	if (result) {
 		dev->valid = false;
@@ -1252,14 +1266,8 @@ void qc_deregister(struct qcusbnet *dev)
 {
 	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;
 
+	dev->dying = true;
 	list_for_each_safe(node, tmp, &dev->qmi.clients) {
 		client = list_entry(node, struct client, node);
 		DBG("release 0x%04X\n", client->cid);
@@ -1268,33 +1276,6 @@ void qc_deregister(struct qcusbnet *dev)
 
 	qc_stopread(dev);
 	dev->valid = false;
-	list_for_each(inodes, &dev->qmi.cdev.list) {
-		inode = container_of(inodes, struct inode, i_devices);
-		if (inode != NULL && !IS_ERR(inode)) {
-			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++) {
-					file = fdtable->fd[count];
-					if (file != NULL &&  file->f_dentry != NULL) {
-						if (file->f_dentry->d_inode == inode) {
-							rcu_assign_pointer(fdtable->fd[count], NULL);
-							spin_unlock_irqrestore(&task->files->file_lock, flags);
-							DBG("forcing close of open file handle\n");
-							filp_close(file, task->files);
-							spin_lock_irqsave(&task->files->file_lock, flags);
-						}
-					}
-				}
-				spin_unlock_irqrestore(&task->files->file_lock, flags);
-			}
-			rcu_read_unlock();
-		}
-	}
-
 	if (!IS_ERR(dev->qmi.devclass))
 		device_destroy(dev->qmi.devclass, dev->qmi.devnum);
 	cdev_del(&dev->qmi.cdev);