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

drivers/net/usb/gobi/qcusbnet.c

Index: drivers/net/usb/gobi/qcusbnet.c
diff --git a/drivers/net/usb/gobi/qcusbnet.c b/drivers/net/usb/gobi/qcusbnet.c
index 7e218a81fe3217e7a7595ceabd2aaabc251b3da6..47ef8808e5022cb2c6134f2aa49435e01bfcdafe 100644
--- a/drivers/net/usb/gobi/qcusbnet.c
+++ b/drivers/net/usb/gobi/qcusbnet.c
@@ -28,6 +28,32 @@
 int qcusbnet_debug;
 static struct class *devclass;
 
+static void free_dev(struct kref *ref)
+{
+	struct qcusbnet *dev = container_of(ref, struct qcusbnet, refcount);
+	kfree(dev);
+}
+
+void qcusbnet_put(struct qcusbnet *dev)
+{
+	kref_put(&dev->refcount, free_dev);
+}
+
+struct qcusbnet *qcusbnet_get(struct qcusbnet *dev)
+{
+	/* If we don't take dev->lock here, we can interleave with qc_deregister
+	 * such that they set dying and drop their ref between our test and our
+	 * kref_get(). Boo. :( */
+	mutex_lock(&dev->lock);

[Jason Glasgow, 2011/03/09 23:47:55]

What if the other thread "wins" the race and sets dev->dying = 1, but than does
the qcusbnet_put and frees the device before we get a chance to run mutex_lock? 
Then we try to mutex_lock a freed object.

Am I missing something?  This idiom does not seem correct.

[Elly Fong-Jones, 2011/03/10 18:29:13]

There is something to what you say. I will meditate upon this.

+	if (dev->dying || !dev->valid) {
+		mutex_unlock(&dev->lock);
+		return NULL;
+	}
+	kref_get(&dev->refcount);
+	mutex_unlock(&dev->lock);
+	return dev;
+}
+
 int qc_suspend(struct usb_interface *iface, pm_message_t event)
 {
 	struct usbnet *usbnet;
@@ -180,12 +206,15 @@ static void qcnet_unbind(struct usbnet *usbnet, struct usb_interface *iface)
 	struct qcusbnet *dev = (struct qcusbnet *)usbnet->data[0];
 
 	netif_carrier_off(usbnet->net);
+	/* See the comments in qcusbnet_get() for an explanation of why we need
+	 * this lock. */
+	mutex_lock(&dev->lock);
 	qc_deregister(dev);
+	mutex_unlock(&dev->lock);
 
-	kfree(usbnet->net->netdev_ops);
 	usbnet->net->netdev_ops = NULL;
-
-	kfree(dev);
+	kfree(usbnet->net->netdev_ops);

[Jason Glasgow, 2011/03/09 23:47:55]

Like Olof said, kfree(NULL)?

[Elly Fong-Jones, 2011/03/10 18:29:13]

Argh.

+	qcusbnet_put(dev);
 }
 
 static void qcnet_urbhook(struct urb *urb)
@@ -590,10 +619,12 @@ int qcnet_probe(struct usb_interface *iface, const struct usb_device_id *vidpids
 	DBG("Mac Address: %pM\n", dev->usbnet->net->dev_addr);
 
 	dev->valid = false;
-	memset(&dev->qmi, 0, sizeof(struct qmidev));
+	memset(&dev->qmi, 0, sizeof(dev->qmi));
 
 	dev->qmi.devclass = devclass;
 
+	kref_init(&dev->refcount);
+	mutex_init(&dev->lock);
 	INIT_LIST_HEAD(&dev->qmi.clients);
 	init_completion(&dev->worker.work);
 	spin_lock_init(&dev->qmi.clients_lock);