IB/hfi1: Do not free hfi1 cdev parent structure early

The deletion of a cdev is not a fence for holding off references to the
structure. The driver attempts to delete the cdev and then proceeds to
free the parent structure, the hfi1_devdata, or dd. This can potentially
lead to a kernel panic in situations where a user has an FD for the cdev
open, and the pci device gets removed. If the user then closes the FD
there will be a NULL dereference when trying to do put on the cdev's
kobject.

Fix this by pointing the cdev's kobject.parent at a new kobject embedded
in its parent structure. Also take a reference when the device is opened
and put it back when it is closed.

Reviewed-by: Mitko Haralanov <mitko.haralanov@intel.com>
Signed-off-by: Ira Weiny <ira.weiny@intel.com>
Signed-off-by: Dennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: Doug Ledford <dledford@redhat.com>

diff --git a/drivers/staging/rdma/hfi1/device.c b/drivers/staging/rdma/hfi1/device.c
index 6ee800f0359f768418b835e517473471a428fc96..bf64b5a7bfd79fd01d0e28268e017cb4e66429a6 100644 (file)
--- a/drivers/staging/rdma/hfi1/device.c
+++ b/drivers/staging/rdma/hfi1/device.c
@@ -60,7 +60,8 @@ static dev_t hfi1_dev;
 int hfi1_cdev_init(int minor, const char *name,
                   const struct file_operations *fops,
                   struct cdev *cdev, struct device **devp,
-                  bool user_accessible)
+                  bool user_accessible,
+                  struct kobject *parent)
 {
        const dev_t dev = MKDEV(MAJOR(hfi1_dev), minor);
        struct device *device = NULL;
@@ -68,6 +69,7 @@ int hfi1_cdev_init(int minor, const char *name,
 
        cdev_init(cdev, fops);
        cdev->owner = THIS_MODULE;
+       cdev->kobj.parent = parent;
        kobject_set_name(&cdev->kobj, name);
 
        ret = cdev_add(cdev, dev, 1);

diff --git a/drivers/staging/rdma/hfi1/device.h b/drivers/staging/rdma/hfi1/device.h
index 5bb3e83cf2da25bf0dfa3c394e56b6359184a1d4..c3ec19cb0ac94da37b21c4505083d32748f1159c 100644 (file)
--- a/drivers/staging/rdma/hfi1/device.h
+++ b/drivers/staging/rdma/hfi1/device.h
@@ -50,7 +50,8 @@
 int hfi1_cdev_init(int minor, const char *name,
                   const struct file_operations *fops,
                   struct cdev *cdev, struct device **devp,
-                  bool user_accessible);
+                  bool user_accessible,
+                  struct kobject *parent);
 void hfi1_cdev_cleanup(struct cdev *cdev, struct device **devp);
 const char *class_name(void);
 int __init dev_init(void);

diff --git a/drivers/staging/rdma/hfi1/file_ops.c b/drivers/staging/rdma/hfi1/file_ops.c
index 113917021af8f029227c52373a4ed248028f902b..7a5b0e676cc7a4b1c1d9a65c76d66ba2f23a1f32 100644 (file)
--- a/drivers/staging/rdma/hfi1/file_ops.c
+++ b/drivers/staging/rdma/hfi1/file_ops.c
@@ -168,6 +168,13 @@ static inline int is_valid_mmap(u64 token)
 
 static int hfi1_file_open(struct inode *inode, struct file *fp)
 {
+       struct hfi1_devdata *dd = container_of(inode->i_cdev,
+                                              struct hfi1_devdata,
+                                              user_cdev);
+
+       /* Just take a ref now. Not all opens result in a context assign */
+       kobject_get(&dd->kobj);
+
        /* The real work is performed later in assign_ctxt() */
        fp->private_data = kzalloc(sizeof(struct hfi1_filedata), GFP_KERNEL);
        if (fp->private_data) /* no cpu affinity by default */
@@ -690,7 +697,9 @@ static int hfi1_file_close(struct inode *inode, struct file *fp)
 {
        struct hfi1_filedata *fdata = fp->private_data;
        struct hfi1_ctxtdata *uctxt = fdata->uctxt;
-       struct hfi1_devdata *dd;
+       struct hfi1_devdata *dd = container_of(inode->i_cdev,
+                                              struct hfi1_devdata,
+                                              user_cdev);
        unsigned long flags, *ev;
 
        fp->private_data = NULL;
@@ -699,7 +708,6 @@ static int hfi1_file_close(struct inode *inode, struct file *fp)
                goto done;
 
        hfi1_cdbg(PROC, "freeing ctxt %u:%u", uctxt->ctxt, fdata->subctxt);
-       dd = uctxt->dd;
        mutex_lock(&hfi1_mutex);
 
        flush_wc();
@@ -765,6 +773,7 @@ static int hfi1_file_close(struct inode *inode, struct file *fp)
        mutex_unlock(&hfi1_mutex);
        hfi1_free_ctxtdata(dd, uctxt);
 done:
+       kobject_put(&dd->kobj);
        kfree(fdata);
        return 0;
 }
@@ -1464,7 +1473,7 @@ static int user_add(struct hfi1_devdata *dd)
        snprintf(name, sizeof(name), "%s_%d", class_name(), dd->unit);
        ret = hfi1_cdev_init(dd->unit, name, &hfi1_file_ops,
                             &dd->user_cdev, &dd->user_device,
-                            true);
+                            true, &dd->kobj);
        if (ret)
                user_remove(dd);
 

diff --git a/drivers/staging/rdma/hfi1/hfi.h b/drivers/staging/rdma/hfi1/hfi.h
index 337bd2dc2dbfdaa6a044620ed6aa4d4a51520bbb..4417a0fd3ef9032bb6336f4afbb30b1241b3727c 100644 (file)
--- a/drivers/staging/rdma/hfi1/hfi.h
+++ b/drivers/staging/rdma/hfi1/hfi.h
@@ -1169,6 +1169,7 @@ struct hfi1_devdata {
        atomic_t aspm_disabled_cnt;
 
        struct hfi1_affinity *affinity;
+       struct kobject kobj;
 };
 
 /* 8051 firmware version helper */

diff --git a/drivers/staging/rdma/hfi1/init.c b/drivers/staging/rdma/hfi1/init.c
index b9beaae5953de2bbdba534a73473d6633e41250e..5cc492e5776d8d87f3833136ed6d33b05f700da0 100644 (file)
--- a/drivers/staging/rdma/hfi1/init.c
+++ b/drivers/staging/rdma/hfi1/init.c
@@ -989,8 +989,10 @@ static void release_asic_data(struct hfi1_devdata *dd)
        dd->asic_data = NULL;
 }
 
-void hfi1_free_devdata(struct hfi1_devdata *dd)
+static void __hfi1_free_devdata(struct kobject *kobj)
 {
+       struct hfi1_devdata *dd =
+               container_of(kobj, struct hfi1_devdata, kobj);
        unsigned long flags;
 
        spin_lock_irqsave(&hfi1_devs_lock, flags);
@@ -1007,6 +1009,15 @@ void hfi1_free_devdata(struct hfi1_devdata *dd)
        rvt_dealloc_device(&dd->verbs_dev.rdi);
 }
 
+static struct kobj_type hfi1_devdata_type = {
+       .release = __hfi1_free_devdata,
+};
+
+void hfi1_free_devdata(struct hfi1_devdata *dd)
+{
+       kobject_put(&dd->kobj);
+}
+
 /*
  * Allocate our primary per-unit data structure.  Must be done via verbs
  * allocator, because the verbs cleanup process both does cleanup and
@@ -1102,6 +1113,7 @@ struct hfi1_devdata *hfi1_alloc_devdata(struct pci_dev *pdev, size_t extra)
                        &pdev->dev,
                        "Could not alloc cpulist info, cpu affinity might be wrong\n");
        }
+       kobject_init(&dd->kobj, &hfi1_devdata_type);
        return dd;
 
 bail: