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

drivers/net/usb/gobi/qcusbnet.c

 /* qcusbnet.c - gobi network 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 10 matching lines @@
 #include "qmi.h"
 #include "qcusbnet.h"
 
 #define DRIVER_VERSION "1.0.110"
 #define DRIVER_AUTHOR "Qualcomm Innovation Center"
 #define DRIVER_DESC "QCUSBNet2k"
 
 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;
         struct qcusbnet *dev;
 
         if (!iface)
                 return -ENOMEM;
 
         usbnet = usb_get_intfdata(iface);
 
@@ skipping 132 matching lines @@
             out->desc.bEndpointAddress);
 
         return 0;
 }
 
 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);
 
+        usbnet->net->netdev_ops = NULL;
         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.

-        usbnet->net->netdev_ops = NULL;
-
-        kfree(dev);
+        qcusbnet_put(dev);
 }
 
 static void qcnet_urbhook(struct urb *urb)
 {
         unsigned long flags;
         struct worker *worker = urb->context;
         if (!worker) {
                 DBG("bad context\n");
                 return;
         }
@@ skipping 384 matching lines @@
         usbnet->net->netdev_ops = netdevops;
 
         memset(&(dev->usbnet->net->stats), 0, sizeof(struct net_device_stats));
 
         dev->iface = iface;
         memset(&(dev->meid), '0', 14);
 
         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);
 
         dev->down = 0;
         qc_setdown(dev, DOWN_NO_NDIS_CONNECTION);
         qc_setdown(dev, DOWN_NET_IFACE_STOPPED);
 
         status = qc_register(dev);
         if (status) {
@@ skipping 33 matching lines @@
 }
 module_exit(modexit);
 
 MODULE_VERSION(DRIVER_VERSION);
 MODULE_AUTHOR(DRIVER_AUTHOR);
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE("Dual BSD/GPL");
 
 module_param(qcusbnet_debug, bool, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(qcusbnet_debug, "Debugging enabled or not");