Commit e11ffbd5 authored by Dennis Dalessandro's avatar Dennis Dalessandro Committed by Doug Ledford

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: default avatarMitko Haralanov <mitko.haralanov@intel.com>
Signed-off-by: default avatarIra Weiny <ira.weiny@intel.com>
Signed-off-by: default avatarDennis Dalessandro <dennis.dalessandro@intel.com>
Signed-off-by: default avatarDoug Ledford <dledford@redhat.com>
parent 8a1882eb
...@@ -60,7 +60,8 @@ static dev_t hfi1_dev; ...@@ -60,7 +60,8 @@ static dev_t hfi1_dev;
int hfi1_cdev_init(int minor, const char *name, int hfi1_cdev_init(int minor, const char *name,
const struct file_operations *fops, const struct file_operations *fops,
struct cdev *cdev, struct device **devp, struct cdev *cdev, struct device **devp,
bool user_accessible) bool user_accessible,
struct kobject *parent)
{ {
const dev_t dev = MKDEV(MAJOR(hfi1_dev), minor); const dev_t dev = MKDEV(MAJOR(hfi1_dev), minor);
struct device *device = NULL; struct device *device = NULL;
...@@ -68,6 +69,7 @@ int hfi1_cdev_init(int minor, const char *name, ...@@ -68,6 +69,7 @@ int hfi1_cdev_init(int minor, const char *name,
cdev_init(cdev, fops); cdev_init(cdev, fops);
cdev->owner = THIS_MODULE; cdev->owner = THIS_MODULE;
cdev->kobj.parent = parent;
kobject_set_name(&cdev->kobj, name); kobject_set_name(&cdev->kobj, name);
ret = cdev_add(cdev, dev, 1); ret = cdev_add(cdev, dev, 1);
......
...@@ -50,7 +50,8 @@ ...@@ -50,7 +50,8 @@
int hfi1_cdev_init(int minor, const char *name, int hfi1_cdev_init(int minor, const char *name,
const struct file_operations *fops, const struct file_operations *fops,
struct cdev *cdev, struct device **devp, 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); void hfi1_cdev_cleanup(struct cdev *cdev, struct device **devp);
const char *class_name(void); const char *class_name(void);
int __init dev_init(void); int __init dev_init(void);
......
...@@ -168,6 +168,13 @@ static inline int is_valid_mmap(u64 token) ...@@ -168,6 +168,13 @@ static inline int is_valid_mmap(u64 token)
static int hfi1_file_open(struct inode *inode, struct file *fp) 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() */ /* The real work is performed later in assign_ctxt() */
fp->private_data = kzalloc(sizeof(struct hfi1_filedata), GFP_KERNEL); fp->private_data = kzalloc(sizeof(struct hfi1_filedata), GFP_KERNEL);
if (fp->private_data) /* no cpu affinity by default */ if (fp->private_data) /* no cpu affinity by default */
...@@ -690,7 +697,9 @@ static int hfi1_file_close(struct inode *inode, struct file *fp) ...@@ -690,7 +697,9 @@ static int hfi1_file_close(struct inode *inode, struct file *fp)
{ {
struct hfi1_filedata *fdata = fp->private_data; struct hfi1_filedata *fdata = fp->private_data;
struct hfi1_ctxtdata *uctxt = fdata->uctxt; 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; unsigned long flags, *ev;
fp->private_data = NULL; fp->private_data = NULL;
...@@ -699,7 +708,6 @@ static int hfi1_file_close(struct inode *inode, struct file *fp) ...@@ -699,7 +708,6 @@ static int hfi1_file_close(struct inode *inode, struct file *fp)
goto done; goto done;
hfi1_cdbg(PROC, "freeing ctxt %u:%u", uctxt->ctxt, fdata->subctxt); hfi1_cdbg(PROC, "freeing ctxt %u:%u", uctxt->ctxt, fdata->subctxt);
dd = uctxt->dd;
mutex_lock(&hfi1_mutex); mutex_lock(&hfi1_mutex);
flush_wc(); flush_wc();
...@@ -765,6 +773,7 @@ static int hfi1_file_close(struct inode *inode, struct file *fp) ...@@ -765,6 +773,7 @@ static int hfi1_file_close(struct inode *inode, struct file *fp)
mutex_unlock(&hfi1_mutex); mutex_unlock(&hfi1_mutex);
hfi1_free_ctxtdata(dd, uctxt); hfi1_free_ctxtdata(dd, uctxt);
done: done:
kobject_put(&dd->kobj);
kfree(fdata); kfree(fdata);
return 0; return 0;
} }
...@@ -1464,7 +1473,7 @@ static int user_add(struct hfi1_devdata *dd) ...@@ -1464,7 +1473,7 @@ static int user_add(struct hfi1_devdata *dd)
snprintf(name, sizeof(name), "%s_%d", class_name(), dd->unit); snprintf(name, sizeof(name), "%s_%d", class_name(), dd->unit);
ret = hfi1_cdev_init(dd->unit, name, &hfi1_file_ops, ret = hfi1_cdev_init(dd->unit, name, &hfi1_file_ops,
&dd->user_cdev, &dd->user_device, &dd->user_cdev, &dd->user_device,
true); true, &dd->kobj);
if (ret) if (ret)
user_remove(dd); user_remove(dd);
......
...@@ -1169,6 +1169,7 @@ struct hfi1_devdata { ...@@ -1169,6 +1169,7 @@ struct hfi1_devdata {
atomic_t aspm_disabled_cnt; atomic_t aspm_disabled_cnt;
struct hfi1_affinity *affinity; struct hfi1_affinity *affinity;
struct kobject kobj;
}; };
/* 8051 firmware version helper */ /* 8051 firmware version helper */
......
...@@ -989,8 +989,10 @@ static void release_asic_data(struct hfi1_devdata *dd) ...@@ -989,8 +989,10 @@ static void release_asic_data(struct hfi1_devdata *dd)
dd->asic_data = NULL; 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; unsigned long flags;
spin_lock_irqsave(&hfi1_devs_lock, flags); spin_lock_irqsave(&hfi1_devs_lock, flags);
...@@ -1007,6 +1009,15 @@ void hfi1_free_devdata(struct hfi1_devdata *dd) ...@@ -1007,6 +1009,15 @@ void hfi1_free_devdata(struct hfi1_devdata *dd)
rvt_dealloc_device(&dd->verbs_dev.rdi); 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 * Allocate our primary per-unit data structure. Must be done via verbs
* allocator, because the verbs cleanup process both does cleanup and * 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) ...@@ -1102,6 +1113,7 @@ struct hfi1_devdata *hfi1_alloc_devdata(struct pci_dev *pdev, size_t extra)
&pdev->dev, &pdev->dev,
"Could not alloc cpulist info, cpu affinity might be wrong\n"); "Could not alloc cpulist info, cpu affinity might be wrong\n");
} }
kobject_init(&dd->kobj, &hfi1_devdata_type);
return dd; return dd;
bail: bail:
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment