From aadfc3b2042d69a6b4b8d719d4221b988d7f31a5 Mon Sep 17 00:00:00 2001 From: Ira Weiny Date: Wed, 9 Sep 2015 01:28:21 -0400 Subject: [PATCH 1/9] IB/hfi1: fix pstateinfo from returning improperly byteswapped value Byteswap link_width_downgrade_*_active values before sending on the wire. In addition properly define the Port State Info structure. Reviewed-by: Dennis Dalessandro Reviewed-by: Christian Gomez Signed-off-by: Rimmer, Todd Signed-off-by: Ira Weiny Acked-by: Mike Marciniszyn Signed-off-by: Doug Ledford --- drivers/staging/rdma/hfi1/mad.c | 4 ++-- include/rdma/opa_port_info.h | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/staging/rdma/hfi1/mad.c b/drivers/staging/rdma/hfi1/mad.c index 37269eb90c34..b2c1b72d38ce 100644 --- a/drivers/staging/rdma/hfi1/mad.c +++ b/drivers/staging/rdma/hfi1/mad.c @@ -1717,9 +1717,9 @@ static int __subn_get_opa_psi(struct opa_smp *smp, u32 am, u8 *data, psi->port_states.portphysstate_portstate = (hfi1_ibphys_portstate(ppd) << 4) | (lstate & 0xf); psi->link_width_downgrade_tx_active = - ppd->link_width_downgrade_tx_active; + cpu_to_be16(ppd->link_width_downgrade_tx_active); psi->link_width_downgrade_rx_active = - ppd->link_width_downgrade_rx_active; + cpu_to_be16(ppd->link_width_downgrade_rx_active); if (resp_len) *resp_len += sizeof(struct opa_port_state_info); diff --git a/include/rdma/opa_port_info.h b/include/rdma/opa_port_info.h index 391dae1931c0..a0fa975cd1c1 100644 --- a/include/rdma/opa_port_info.h +++ b/include/rdma/opa_port_info.h @@ -294,8 +294,8 @@ struct opa_port_states { struct opa_port_state_info { struct opa_port_states port_states; - u16 link_width_downgrade_tx_active; - u16 link_width_downgrade_rx_active; + __be16 link_width_downgrade_tx_active; + __be16 link_width_downgrade_rx_active; }; struct opa_port_info { From e1df0068a24ba56673183cc3bd392d8bc301d423 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Tue, 15 Sep 2015 13:35:25 +0300 Subject: [PATCH 2/9] IB/hfi1: fix copy_to/from_user() error handling copy_to/from_user() returns the number of bytes which we were not able to copy. It doesn't return an error code. Also a couple places had a printk() on error and I removed that because people can take advantage of it to fill /var/log/messages with spam. Signed-off-by: Dan Carpenter Acked-by: Mike Marciniszyn Signed-off-by: Doug Ledford --- drivers/staging/rdma/hfi1/diag.c | 31 +++++++++++++++---------------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/drivers/staging/rdma/hfi1/diag.c b/drivers/staging/rdma/hfi1/diag.c index 6777d6b659cf..ce01deea834c 100644 --- a/drivers/staging/rdma/hfi1/diag.c +++ b/drivers/staging/rdma/hfi1/diag.c @@ -1012,11 +1012,10 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) case HFI1_SNOOP_IOCSETLINKSTATE_EXTRA: memset(&link_info, 0, sizeof(link_info)); - ret = copy_from_user(&link_info, + if (copy_from_user(&link_info, (struct hfi1_link_info __user *)arg, - sizeof(link_info)); - if (ret) - break; + sizeof(link_info))) + ret = -EFAULT; value = link_info.port_state; index = link_info.port_number; @@ -1080,9 +1079,10 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) case HFI1_SNOOP_IOCGETLINKSTATE_EXTRA: if (cmd == HFI1_SNOOP_IOCGETLINKSTATE_EXTRA) { memset(&link_info, 0, sizeof(link_info)); - ret = copy_from_user(&link_info, + if (copy_from_user(&link_info, (struct hfi1_link_info __user *)arg, - sizeof(link_info)); + sizeof(link_info))) + ret = -EFAULT; index = link_info.port_number; } else { ret = __get_user(index, (int __user *) arg); @@ -1114,9 +1114,10 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) ppd->link_speed_active; link_info.link_width_active = ppd->link_width_active; - ret = copy_to_user( + if (copy_to_user( (struct hfi1_link_info __user *)arg, - &link_info, sizeof(link_info)); + &link_info, sizeof(link_info))) + ret = -EFAULT; } else { ret = __put_user(value, (int __user *)arg); } @@ -1142,10 +1143,9 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) snoop_dbg("Setting filter"); /* just copy command structure */ argp = (unsigned long *)arg; - ret = copy_from_user(&filter_cmd, (void __user *)argp, - sizeof(filter_cmd)); - if (ret < 0) { - pr_alert("Error copying filter command\n"); + if (copy_from_user(&filter_cmd, (void __user *)argp, + sizeof(filter_cmd))) { + ret = -EFAULT; break; } if (filter_cmd.opcode >= HFI1_MAX_FILTERS) { @@ -1167,12 +1167,11 @@ static long hfi1_ioctl(struct file *fp, unsigned int cmd, unsigned long arg) break; } /* copy remaining data from userspace */ - ret = copy_from_user((u8 *)filter_value, + if (copy_from_user((u8 *)filter_value, (void __user *)filter_cmd.value_ptr, - filter_cmd.length); - if (ret < 0) { + filter_cmd.length)) { kfree(filter_value); - pr_alert("Error copying filter data\n"); + ret = -EFAULT; break; } /* Drain packets first */ From aeef010a0f63ad0a6f993d3da30753e9a8a39ec5 Mon Sep 17 00:00:00 2001 From: Mike Marciniszyn Date: Tue, 15 Sep 2015 10:19:27 -0400 Subject: [PATCH 3/9] IB/hfi1: fix sdma_descq_cnt parameter parsing The boolean tests should have been or-ed. Reported-by: David Binderman Reviewed-by: Jubin John Signed-off-by: Mike Marciniszyn Signed-off-by: Doug Ledford --- drivers/staging/rdma/hfi1/sdma.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/staging/rdma/hfi1/sdma.c b/drivers/staging/rdma/hfi1/sdma.c index a8c903caecce..7e01f30f67b9 100644 --- a/drivers/staging/rdma/hfi1/sdma.c +++ b/drivers/staging/rdma/hfi1/sdma.c @@ -737,7 +737,7 @@ u16 sdma_get_descq_cnt(void) */ if (!is_power_of_2(count)) return SDMA_DESCQ_CNT; - if (count < 64 && count > 32768) + if (count < 64 || count > 32768) return SDMA_DESCQ_CNT; return count; } From 50b19729ced72cfa8bb1c44fed9203f395f13991 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Wed, 16 Sep 2015 09:22:20 +0300 Subject: [PATCH 4/9] IB/hfi1: checking for NULL instead of IS_ERR __get_txreq() returns an ERR_PTR() but this checks for NULL so it would oops on failure. Signed-off-by: Dan Carpenter Signed-off-by: Doug Ledford --- drivers/staging/rdma/hfi1/verbs.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/staging/rdma/hfi1/verbs.c b/drivers/staging/rdma/hfi1/verbs.c index 53ac21431542..41bb59eb001c 100644 --- a/drivers/staging/rdma/hfi1/verbs.c +++ b/drivers/staging/rdma/hfi1/verbs.c @@ -749,11 +749,13 @@ static inline struct verbs_txreq *get_txreq(struct hfi1_ibdev *dev, struct verbs_txreq *tx; tx = kmem_cache_alloc(dev->verbs_txreq_cache, GFP_ATOMIC); - if (!tx) + if (!tx) { /* call slow path to get the lock */ tx = __get_txreq(dev, qp); - if (tx) - tx->qp = qp; + if (IS_ERR(tx)) + return tx; + } + tx->qp = qp; return tx; } From 951842b0540d2ed49ae29ba968adc496baf46556 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Wed, 16 Sep 2015 09:22:51 +0300 Subject: [PATCH 5/9] IB/hfi1: fix a locking bug mutex_trylock() returns zero on failure, not EBUSY. Signed-off-by: Dan Carpenter Signed-off-by: Doug Ledford --- drivers/staging/rdma/hfi1/chip.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/rdma/hfi1/chip.c b/drivers/staging/rdma/hfi1/chip.c index 654eafef1d30..aa58e597df06 100644 --- a/drivers/staging/rdma/hfi1/chip.c +++ b/drivers/staging/rdma/hfi1/chip.c @@ -2710,7 +2710,7 @@ int acquire_lcb_access(struct hfi1_devdata *dd, int sleep_ok) if (sleep_ok) { mutex_lock(&ppd->hls_lock); } else { - while (mutex_trylock(&ppd->hls_lock) == EBUSY) + while (!mutex_trylock(&ppd->hls_lock)) udelay(1); } @@ -2758,7 +2758,7 @@ int release_lcb_access(struct hfi1_devdata *dd, int sleep_ok) if (sleep_ok) { mutex_lock(&dd->pport->hls_lock); } else { - while (mutex_trylock(&dd->pport->hls_lock) == EBUSY) + while (!mutex_trylock(&dd->pport->hls_lock)) udelay(1); } From ebe6b2e8bc2cd06a330b3f9be8a4fa3ff44ab026 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Wed, 16 Sep 2015 09:42:25 +0300 Subject: [PATCH 6/9] IB/hfi1: info leak in get_ctxt_info() The cinfo struct has a hole after the last struct member so we need to zero it out. Otherwise we disclose some uninitialized stack data. Signed-off-by: Dan Carpenter Signed-off-by: Doug Ledford --- drivers/staging/rdma/hfi1/file_ops.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/staging/rdma/hfi1/file_ops.c b/drivers/staging/rdma/hfi1/file_ops.c index 469861750b76..2c43ca5a379b 100644 --- a/drivers/staging/rdma/hfi1/file_ops.c +++ b/drivers/staging/rdma/hfi1/file_ops.c @@ -1181,6 +1181,7 @@ static int get_ctxt_info(struct file *fp, void __user *ubase, __u32 len) struct hfi1_filedata *fd = fp->private_data; int ret = 0; + memset(&cinfo, 0, sizeof(cinfo)); ret = hfi1_get_base_kinfo(uctxt, &cinfo); if (ret < 0) goto done; From 3f2686a2665b4d06753b602fe394b5d87bc7f279 Mon Sep 17 00:00:00 2001 From: Dan Carpenter Date: Wed, 16 Sep 2015 19:02:54 +0300 Subject: [PATCH 7/9] IB/hfi1: clean up some defines I added spaces around operators so it matches kernel style because normally "-1ULL" is a number and " - 1" is a subtract operation. Also removed some superflous "ULL" types so "1ULL" becomes "1". Signed-off-by: Dan Carpenter Signed-off-by: Doug Ledford --- drivers/staging/rdma/hfi1/sdma.h | 36 ++++++++++++++++---------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/drivers/staging/rdma/hfi1/sdma.h b/drivers/staging/rdma/hfi1/sdma.h index 1e613fcd8f4c..496086903891 100644 --- a/drivers/staging/rdma/hfi1/sdma.h +++ b/drivers/staging/rdma/hfi1/sdma.h @@ -109,53 +109,53 @@ /* * Bits defined in the send DMA descriptor. */ -#define SDMA_DESC0_FIRST_DESC_FLAG (1ULL<<63) -#define SDMA_DESC0_LAST_DESC_FLAG (1ULL<<62) +#define SDMA_DESC0_FIRST_DESC_FLAG (1ULL << 63) +#define SDMA_DESC0_LAST_DESC_FLAG (1ULL << 62) #define SDMA_DESC0_BYTE_COUNT_SHIFT 48 #define SDMA_DESC0_BYTE_COUNT_WIDTH 14 #define SDMA_DESC0_BYTE_COUNT_MASK \ - ((1ULL< Date: Wed, 16 Sep 2015 19:03:45 +0300 Subject: [PATCH 8/9] IB/hfi1: mask vs shift confusion We are shifting by the _MASK macros instead of the _SHIFT ones. Signed-off-by: Dan Carpenter Signed-off-by: Doug Ledford --- drivers/staging/rdma/hfi1/sdma.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/staging/rdma/hfi1/sdma.c b/drivers/staging/rdma/hfi1/sdma.c index 7e01f30f67b9..aecd1a74741c 100644 --- a/drivers/staging/rdma/hfi1/sdma.c +++ b/drivers/staging/rdma/hfi1/sdma.c @@ -1848,7 +1848,7 @@ static void dump_sdma_state(struct sdma_engine *sde) dd_dev_err(sde->dd, "\taidx: %u amode: %u alen: %u\n", (u8)((desc[1] & SDMA_DESC1_HEADER_INDEX_SMASK) - >> SDMA_DESC1_HEADER_INDEX_MASK), + >> SDMA_DESC1_HEADER_INDEX_SHIFT), (u8)((desc[1] & SDMA_DESC1_HEADER_MODE_SMASK) >> SDMA_DESC1_HEADER_MODE_SHIFT), (u8)((desc[1] & SDMA_DESC1_HEADER_DWS_SMASK) @@ -1926,7 +1926,7 @@ void sdma_seqfile_dump_sde(struct seq_file *s, struct sdma_engine *sde) if (desc[0] & SDMA_DESC0_FIRST_DESC_FLAG) seq_printf(s, "\t\tahgidx: %u ahgmode: %u\n", (u8)((desc[1] & SDMA_DESC1_HEADER_INDEX_SMASK) - >> SDMA_DESC1_HEADER_INDEX_MASK), + >> SDMA_DESC1_HEADER_INDEX_SHIFT), (u8)((desc[1] & SDMA_DESC1_HEADER_MODE_SMASK) >> SDMA_DESC1_HEADER_MODE_SHIFT)); head = (head + 1) & sde->sdma_mask; From e116a64fab650aed3d7b9b4db0b59c07f361bc9f Mon Sep 17 00:00:00 2001 From: Ira Weiny Date: Thu, 17 Sep 2015 13:47:49 -0400 Subject: [PATCH 9/9] IB/hfi: Properly set permissions for user device files Some of the device files are required to be user accessible for PSM while most should remain accessible only by root. Add a parameter to hfi1_cdev_init which controls if the user should have access to this device which places it in a different class with the appropriate devnode callback. In addition set the devnode call back for the existing class to be a bit more explicit for those permissions. Finally remove the unnecessary null check before class_destroy Tested-by: Donald Dutile Signed-off-by: Haralanov, Mitko (mitko.haralanov@intel.com) Signed-off-by: Ira Weiny Signed-off-by: Doug Ledford --- drivers/staging/rdma/hfi1/device.c | 54 ++++++++++++++++++++++++---- drivers/staging/rdma/hfi1/device.h | 3 +- drivers/staging/rdma/hfi1/diag.c | 5 +-- drivers/staging/rdma/hfi1/file_ops.c | 9 +++-- 4 files changed, 59 insertions(+), 12 deletions(-) diff --git a/drivers/staging/rdma/hfi1/device.c b/drivers/staging/rdma/hfi1/device.c index 07c87a87775f..bc26a5392712 100644 --- a/drivers/staging/rdma/hfi1/device.c +++ b/drivers/staging/rdma/hfi1/device.c @@ -57,11 +57,13 @@ #include "device.h" static struct class *class; +static struct class *user_class; 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) + struct cdev *cdev, struct device **devp, + bool user_accessible) { const dev_t dev = MKDEV(MAJOR(hfi1_dev), minor); struct device *device = NULL; @@ -78,7 +80,11 @@ int hfi1_cdev_init(int minor, const char *name, goto done; } - device = device_create(class, NULL, dev, NULL, "%s", name); + if (user_accessible) + device = device_create(user_class, NULL, dev, NULL, "%s", name); + else + device = device_create(class, NULL, dev, NULL, "%s", name); + if (!IS_ERR(device)) goto done; ret = PTR_ERR(device); @@ -110,6 +116,26 @@ const char *class_name(void) return hfi1_class_name; } +static char *hfi1_devnode(struct device *dev, umode_t *mode) +{ + if (mode) + *mode = 0600; + return kasprintf(GFP_KERNEL, "%s", dev_name(dev)); +} + +static const char *hfi1_class_name_user = "hfi1_user"; +const char *class_name_user(void) +{ + return hfi1_class_name_user; +} + +static char *hfi1_user_devnode(struct device *dev, umode_t *mode) +{ + if (mode) + *mode = 0666; + return kasprintf(GFP_KERNEL, "%s", dev_name(dev)); +} + int __init dev_init(void) { int ret; @@ -125,7 +151,22 @@ int __init dev_init(void) ret = PTR_ERR(class); pr_err("Could not create device class (err %d)\n", -ret); unregister_chrdev_region(hfi1_dev, HFI1_NMINORS); + goto done; } + class->devnode = hfi1_devnode; + + user_class = class_create(THIS_MODULE, class_name_user()); + if (IS_ERR(user_class)) { + ret = PTR_ERR(user_class); + pr_err("Could not create device class for user accessible files (err %d)\n", + -ret); + class_destroy(class); + class = NULL; + user_class = NULL; + unregister_chrdev_region(hfi1_dev, HFI1_NMINORS); + goto done; + } + user_class->devnode = hfi1_user_devnode; done: return ret; @@ -133,10 +174,11 @@ int __init dev_init(void) void dev_cleanup(void) { - if (class) { - class_destroy(class); - class = NULL; - } + class_destroy(class); + class = NULL; + + class_destroy(user_class); + user_class = NULL; unregister_chrdev_region(hfi1_dev, HFI1_NMINORS); } diff --git a/drivers/staging/rdma/hfi1/device.h b/drivers/staging/rdma/hfi1/device.h index 98caecd3d807..2850ff739d81 100644 --- a/drivers/staging/rdma/hfi1/device.h +++ b/drivers/staging/rdma/hfi1/device.h @@ -52,7 +52,8 @@ int hfi1_cdev_init(int minor, const char *name, const struct file_operations *fops, - struct cdev *cdev, struct device **devp); + struct cdev *cdev, struct device **devp, + bool user_accessible); 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/diag.c b/drivers/staging/rdma/hfi1/diag.c index ce01deea834c..3e8d5ac4c626 100644 --- a/drivers/staging/rdma/hfi1/diag.c +++ b/drivers/staging/rdma/hfi1/diag.c @@ -292,7 +292,7 @@ int hfi1_diag_add(struct hfi1_devdata *dd) if (atomic_inc_return(&diagpkt_count) == 1) { ret = hfi1_cdev_init(HFI1_DIAGPKT_MINOR, name, &diagpkt_file_ops, &diagpkt_cdev, - &diagpkt_device); + &diagpkt_device, false); } return ret; @@ -592,7 +592,8 @@ static int hfi1_snoop_add(struct hfi1_devdata *dd, const char *name) ret = hfi1_cdev_init(HFI1_SNOOP_CAPTURE_BASE + dd->unit, name, &snoop_file_ops, - &dd->hfi1_snoop.cdev, &dd->hfi1_snoop.class_dev); + &dd->hfi1_snoop.cdev, &dd->hfi1_snoop.class_dev, + false); if (ret) { dd_dev_err(dd, "Couldn't create %s device: %d", name, ret); diff --git a/drivers/staging/rdma/hfi1/file_ops.c b/drivers/staging/rdma/hfi1/file_ops.c index 2c43ca5a379b..72d38500d8ce 100644 --- a/drivers/staging/rdma/hfi1/file_ops.c +++ b/drivers/staging/rdma/hfi1/file_ops.c @@ -2090,14 +2090,16 @@ static int user_add(struct hfi1_devdata *dd) if (atomic_inc_return(&user_count) == 1) { ret = hfi1_cdev_init(0, class_name(), &hfi1_file_ops, - &wildcard_cdev, &wildcard_device); + &wildcard_cdev, &wildcard_device, + true); if (ret) goto done; } snprintf(name, sizeof(name), "%s_%d", class_name(), dd->unit); ret = hfi1_cdev_init(dd->unit + 1, name, &hfi1_file_ops, - &dd->user_cdev, &dd->user_device); + &dd->user_cdev, &dd->user_device, + true); if (ret) goto done; @@ -2105,7 +2107,8 @@ static int user_add(struct hfi1_devdata *dd) snprintf(name, sizeof(name), "%s_ui%d", class_name(), dd->unit); ret = hfi1_cdev_init(dd->unit + UI_OFFSET, name, &ui_file_ops, - &dd->ui_cdev, &dd->ui_device); + &dd->ui_cdev, &dd->ui_device, + false); if (ret) goto done; }