RDMA fixes for v4.16

PATCH Applied Leon Romanovsky 4 participants 35 messages Feb 13, 2018 – Feb 15, 2018 18 messages not in the archive InfiniBand

lore

Share & tools

lore

Leon Romanovsky

author

2018-02-13 10:18 UTC #0 lore raw

cover RDMA fixes for v4.16

Hi,

This is mostly fixes for uverbs layers with two extra patches: one from Alaa, who removed annoying warning prints in IPoIB and another from Leon who removed unimplented part of restrack code.

Thanks

Alaa Hleihel (1):
IB/ipoib: Do not warn if IPoIB debugfs doesn't exist

Jason Gunthorpe (7):
IB/uverbs: Use inline data transfer for UHW_IN IB/uverbs: Use u64_to_user_ptr() not a union IB/uverbs: Use __aligned_u64 for uapi headers IB/uverbs: Hold the uobj write lock after allocate IB/uverbs: Tidy lockdep_check
IB/uverbs: Tidy uverbs_uobject_add IB/uverbs: Fix unbalanced unlock on error path for
rdma_explicit_destroy

Leon Romanovsky (3):
RDMA/restrack: Remove unimplemented XRCD object RDMA/uverbs: Protect from races between lookup and destroy of uobjects RDMA/uverbs: Protect from command mask overflow

Matan Barak (4):
IB/uverbs: Always the attribute size provided by the user IB/uverbs: Fix method merging in uverbs_ioctl_merge IB/uverbs: Add ioctl support for 32bit processes IB/uverbs: Fix possible oops with duplicate ioctl attributes

 drivers/infiniband/core/rdma_core.c          | 51 +++++++++++++++++-----------
 drivers/infiniband/core/restrack.c           |  5 ---
 drivers/infiniband/core/uverbs_ioctl.c       |  3 ++
 drivers/infiniband/core/uverbs_ioctl_merge.c | 18 +++++-----
 drivers/infiniband/core/uverbs_main.c        | 29 ++++++++++++----
 drivers/infiniband/core/uverbs_std_types.c   | 12 ++++---
 drivers/infiniband/ulp/ipoib/ipoib_fs.c      |  2 --
 include/rdma/restrack.h                      |  4 ---
 include/rdma/uverbs_ioctl.h                  | 43 ++++++++++++++++-------
 include/uapi/rdma/rdma_user_ioctl.h          |  4 +--
 10 files changed, 107 insertions(+), 64 deletions(-)

--
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #2 lore raw

PATCH 1/15 IB/ipoib: Do not warn if IPoIB debugfs doesn't exist

In reply to a message not in the archive

From: Alaa Hleihel <alaa-VPRAkNaXOzVWk0Htik3J/[email protected]>

netdev_wait_allrefs() could rebroadcast NETDEV_UNREGISTER event multiple times until all refs are gone, which will result in calling ipoib_delete_debug_files multiple times and printing a warning.

Remove the WARN_ONCE since checks of NULL pointers before calling debugfs_remove are not needed.

Fixes 771a52584096 ("IB/IPoIB: ibX: failed to create mcg debug file")

Signed-off-by Alaa Hleihel <alaa-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/ulp/ipoib/ipoib_fs.c | 2 --
 1 file changed, 2 deletions(-)

drivers/infiniband/ulp/ipoib/ipoib_fs.c +0 -2

index 11f74cbe6660..ea302b054601 100644
@@ -281,8 +281,6 @@ void ipoib_delete_debug_files(struct net_device *dev)
 {
 	struct ipoib_dev_priv *priv = ipoib_priv(dev);
 
-	WARN_ONCE(!priv->mcg_dentry, "null mcg debug file\n");
-	WARN_ONCE(!priv->path_dentry, "null path debug file\n");
 	debugfs_remove(priv->mcg_dentry);
 	debugfs_remove(priv->path_dentry);
 	priv->mcg_dentry = priv->path_dentry = NULL;

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #9 lore raw

PATCH 2/15 RDMA/restrack: Remove unimplemented XRCD object

In reply to a message not in the archive

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

Resource tracking of XRCD objects is not implemented in current version of restrack and hence can be removed.

Fixes 02d8883f520e ("RDMA/restrack: Add general infrastructure to track RDMA resources")

Signed-off-by Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/core/restrack.c | 5 -----
 include/rdma/restrack.h            | 4 ----
 2 files changed, 9 deletions(-)

drivers/infiniband/core/restrack.c +0 -5

index 857637bf46da..d8dc709a3715 100644
@@ -63,7 +63,6 @@ static struct ib_device *res_to_dev(struct rdma_restrack_entry *res)
 {
 	enum rdma_restrack_type type = res->type;
 	struct ib_device *dev;
-	struct ib_xrcd *xrcd;
 	struct ib_pd *pd;
 	struct ib_cq *cq;
 	struct ib_qp *qp;

@@ -81,10 +80,6 @@ static struct ib_device *res_to_dev(struct rdma_restrack_entry *res)
 		qp = container_of(res, struct ib_qp, res);
 		dev = qp->device;
 		break;
-	case RDMA_RESTRACK_XRCD:
-		xrcd = container_of(res, struct ib_xrcd, res);
-		dev = xrcd->device;
-		break;
 	default:
 		WARN_ONCE(true, "Wrong resource tracking type %u\n", type);
 		return NULL;

include/rdma/restrack.h +0 -4

index c2d81167c858..2cdf8dcf4bdc 100644
@@ -28,10 +28,6 @@ enum rdma_restrack_type {
 	 * @RDMA_RESTRACK_QP: Queue pair (QP)
 	 */
 	RDMA_RESTRACK_QP,
-	/**
-	 * @RDMA_RESTRACK_XRCD: XRC domain (XRCD)
-	 */
-	RDMA_RESTRACK_XRCD,
 	/**
 	 * @RDMA_RESTRACK_MAX: Last entry, used for array dclarations
 	 */

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #16 lore raw

PATCH 3/15 IB/uverbs: Always the attribute size provided by the user

In reply to a message not in the archive

From: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>

This fixes several bugs around the copy_to/from user path:
- copy_to used the user provided size of the attribute and could copy data beyond the end of the kernel buffer into userspace.
- copy_from didn't know the size of the kernel buffer and could have left kernel memory unexpectedly un-initialized.
- copy_from did not use the user length to determine if the attribute data is inlined or not.

Signed-off-by Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/uverbs_std_types.c |  5 +++--
 include/rdma/uverbs_ioctl.h                | 35 ++++++++++++++++++++++++------
 2 files changed, 31 insertions(+), 9 deletions(-)

drivers/infiniband/core/uverbs_std_types.c +3 -2

index cab0ac3556eb..c6502c7b7c46 100644
@@ -323,7 +323,8 @@ static int uverbs_create_cq_handler(struct ib_device *ib_dev,
 	cq->res.type = RDMA_RESTRACK_CQ;
 	rdma_restrack_add(&cq->res);
 
-	ret = uverbs_copy_to(attrs, CREATE_CQ_RESP_CQE, &cq->cqe);
+	ret = uverbs_copy_to(attrs, CREATE_CQ_RESP_CQE, &cq->cqe,
+			     sizeof(cq->cqe));
 	if (ret)
 		goto err_cq;

@@ -375,7 +376,7 @@ static int uverbs_destroy_cq_handler(struct ib_device *ib_dev,
 	resp.comp_events_reported  = obj->comp_events_reported;
 	resp.async_events_reported = obj->async_events_reported;
 
-	return uverbs_copy_to(attrs, DESTROY_CQ_RESP, &resp);
+	return uverbs_copy_to(attrs, DESTROY_CQ_RESP, &resp, sizeof(resp));
 }
 
 static DECLARE_UVERBS_METHOD(

include/rdma/uverbs_ioctl.h +28 -7

index 6da44079aa58..32cb14703914 100644
@@ -351,29 +351,50 @@ static inline const struct uverbs_attr *uverbs_attr_get(const struct uverbs_attr
 }
 
 static inline int uverbs_copy_to(const struct uverbs_attr_bundle *attrs_bundle,
-				 size_t idx, const void *from)
+				 size_t idx, const void *from, size_t size)
 {
 	const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx);
 	u16 flags;
+	size_t min_size;
 
 	if (IS_ERR(attr))
 		return PTR_ERR(attr);
 
+	min_size = min_t(size_t, attr->ptr_attr.len, size);
+	if (copy_to_user(attr->ptr_attr.ptr, from, min_size))
+		return -EFAULT;
+
 	flags = attr->ptr_attr.flags | UVERBS_ATTR_F_VALID_OUTPUT;
-	return (!copy_to_user(attr->ptr_attr.ptr, from, attr->ptr_attr.len) &&
-		!put_user(flags, &attr->uattr->flags)) ? 0 : -EFAULT;
+	if (put_user(flags, &attr->uattr->flags))
+		return -EFAULT;
+
+	return 0;
 }
 
-static inline int _uverbs_copy_from(void *to, size_t to_size,
+static inline bool uverbs_attr_ptr_is_inline(const struct uverbs_attr *attr)
+{
+	return attr->ptr_attr.len <= sizeof(attr->ptr_attr.data);
+}
+
+static inline int _uverbs_copy_from(void *to,
 				    const struct uverbs_attr_bundle *attrs_bundle,
-				    size_t idx)
+				    size_t idx,
+				    size_t size)
 {
 	const struct uverbs_attr *attr = uverbs_attr_get(attrs_bundle, idx);
 
 	if (IS_ERR(attr))
 		return PTR_ERR(attr);
 
-	if (to_size <= sizeof(((struct ib_uverbs_attr *)0)->data))
+	/*
+	 * Validation ensures attr->ptr_attr.len >= size. If the caller is
+	 * using UVERBS_ATTR_SPEC_F_MIN_SZ then it must call copy_from with
+	 * the right size.
+	 */
+	if (unlikely(size < attr->ptr_attr.len))
+		return -EINVAL;
+
+	if (uverbs_attr_ptr_is_inline(attr))
 		memcpy(to, &attr->ptr_attr.data, attr->ptr_attr.len);
 	else if (copy_from_user(to, attr->ptr_attr.ptr, attr->ptr_attr.len))
 		return -EFAULT;

@@ -382,7 +403,7 @@ static inline int _uverbs_copy_from(void *to, size_t to_size,
 }
 
 #define uverbs_copy_from(to, attrs_bundle, idx)				      \
-	_uverbs_copy_from(to, sizeof(*(to)), attrs_bundle, idx)
+	_uverbs_copy_from(to, attrs_bundle, idx, sizeof(*to))
 
 /* =================================================
  *	 Definitions -> Specs infrastructure

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #17 lore raw

PATCH 4/15 IB/uverbs: Use inline data transfer for UHW_IN

In reply to a message not in the archive

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

The rule for the API is pointers less than 8 bytes are inlined into the .data field of the attribute. Fix the creation of the driver udata struct to follow this rule and point to the .data itself when the size is less than 8 bytes.

Otherwise if the UHW struct is less than 8 bytes the driver will get EFAULT during copy_from_user.

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/uverbs_std_types.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/infiniband/core/uverbs_std_types.c +4 -1

index c6502c7b7c46..7b0e4d778d79 100644
@@ -234,8 +234,11 @@ static void create_udata(struct uverbs_attr_bundle *ctx,
 		uverbs_attr_get(ctx, UVERBS_UHW_OUT);
 
 	if (!IS_ERR(uhw_in)) {
-		udata->inbuf = uhw_in->ptr_attr.ptr;
 		udata->inlen = uhw_in->ptr_attr.len;
+		if (uverbs_attr_ptr_is_inline(uhw_in))
+			udata->inbuf = &uhw_in->uattr->data;
+		else
+			udata->inbuf = uhw_in->ptr_attr.ptr;
 	} else {
 		udata->inbuf = NULL;
 		udata->inlen = 0;

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #18 lore raw

PATCH 5/15 IB/uverbs: Use u64_to_user_ptr() not a union

In reply to a message not in the archive

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

The union approach will get the endianness wrong sometimes if the kernel's pointer size is 32 bits resulting in EFAULTs when trying to copy to/from user.

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/uverbs_std_types.c |  4 ++--
 include/rdma/uverbs_ioctl.h                | 10 ++++------
 2 files changed, 6 insertions(+), 8 deletions(-)

drivers/infiniband/core/uverbs_std_types.c +2 -2

index 7b0e4d778d79..df1360e6774f 100644
@@ -238,14 +238,14 @@ static void create_udata(struct uverbs_attr_bundle *ctx,
 		if (uverbs_attr_ptr_is_inline(uhw_in))
 			udata->inbuf = &uhw_in->uattr->data;
 		else
-			udata->inbuf = uhw_in->ptr_attr.ptr;
+			udata->inbuf = u64_to_user_ptr(uhw_in->ptr_attr.data);
 	} else {
 		udata->inbuf = NULL;
 		udata->inlen = 0;
 	}
 
 	if (!IS_ERR(uhw_out)) {
-		udata->outbuf = uhw_out->ptr_attr.ptr;
+		udata->outbuf = u64_to_user_ptr(uhw_out->ptr_attr.data);
 		udata->outlen = uhw_out->ptr_attr.len;
 	} else {
 		udata->outbuf = NULL;

include/rdma/uverbs_ioctl.h +4 -6

index 32cb14703914..38287d9d23a1 100644
@@ -276,10 +276,7 @@ struct uverbs_object_tree_def {
  */
 
 struct uverbs_ptr_attr {
-	union {
-		u64		data;
-		void	__user *ptr;
-	};
+	u64		data;
 	u16		len;
 	/* Combination of bits from enum UVERBS_ATTR_F_XXXX */
 	u16		flags;

@@ -361,7 +358,7 @@ static inline int uverbs_copy_to(const struct uverbs_attr_bundle *attrs_bundle,
 		return PTR_ERR(attr);
 
 	min_size = min_t(size_t, attr->ptr_attr.len, size);
-	if (copy_to_user(attr->ptr_attr.ptr, from, min_size))
+	if (copy_to_user(u64_to_user_ptr(attr->ptr_attr.data), from, min_size))
 		return -EFAULT;
 
 	flags = attr->ptr_attr.flags | UVERBS_ATTR_F_VALID_OUTPUT;

@@ -396,7 +393,8 @@ static inline int _uverbs_copy_from(void *to,
 
 	if (uverbs_attr_ptr_is_inline(attr))
 		memcpy(to, &attr->ptr_attr.data, attr->ptr_attr.len);
-	else if (copy_from_user(to, attr->ptr_attr.ptr, attr->ptr_attr.len))
+	else if (copy_from_user(to, u64_to_user_ptr(attr->ptr_attr.data),
+				attr->ptr_attr.len))
 		return -EFAULT;
 
 	return 0;

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #21 lore raw

PATCH 6/15 IB/uverbs: Fix method merging in uverbs_ioctl_merge

In reply to a message not in the archive

From: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>

Fix a bug in uverbs_ioctl_merge that looked at objects iterators number instead of methods iterators number when merging methods.

While we're at it, make the uverbs_ioctl_merge code a bit more clear and faster.

Fixes 118620d3686b ('IB/core: Add uverbs merge trees functionality')

Signed-off-by Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/uverbs_ioctl_merge.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

drivers/infiniband/core/uverbs_ioctl_merge.c +9 -9

index 062485f9300d..62e1eb1d2a28 100644
@@ -114,6 +114,7 @@ static size_t get_elements_above_id(const void **iters,
 	short min = SHRT_MAX;
 	const void *elem;
 	int i, j, last_stored = -1;
+	unsigned int equal_min = 0;
 
 	for_each_element(elem, i, j, elements, num_elements, num_offset,
 			 data_offset) {

@@ -136,6 +137,10 @@ static size_t get_elements_above_id(const void **iters,
 		 */
 		iters[last_stored == i ? num_iters - 1 : num_iters++] = elem;
 		last_stored = i;
+		if (min == GET_ID(id))
+			equal_min++;
+		else
+			equal_min = 1;
 		min = GET_ID(id);
 	}

@@ -146,15 +151,10 @@ static size_t get_elements_above_id(const void **iters,
 	 * Therefore, we need to clean the beginning of the array to make sure
 	 * all ids of final elements are equal to min.
 	 */
-	for (i = num_iters - 1; i >= 0 &&
-	     GET_ID(*(u16 *)(iters[i] + id_offset)) == min; i--)
-		;
-
-	num_iters -= i + 1;
-	memmove(iters, iters + i + 1, sizeof(*iters) * num_iters);
+	memmove(iters, iters + num_iters - equal_min, sizeof(*iters) * equal_min);
 
 	*min_id = min;
-	return num_iters;
+	return equal_min;
 }
 
 #define find_max_element_entry_id(num_elements, elements, num_objects_fld, \

@@ -322,7 +322,7 @@ static struct uverbs_method_spec *build_method_with_attrs(const struct uverbs_me
 		hash = kzalloc(sizeof(*hash) +
 			       ALIGN(sizeof(*hash->attrs) * (attr_max_bucket + 1),
 				     sizeof(long)) +
-			       BITS_TO_LONGS(attr_max_bucket) * sizeof(long),
+			       BITS_TO_LONGS(attr_max_bucket + 1) * sizeof(long),
 			       GFP_KERNEL);
 		if (!hash) {
 			res = -ENOMEM;

@@ -509,7 +509,7 @@ static struct uverbs_object_spec *build_object_with_methods(const struct uverbs_
 			 * first handler which != NULL. This also defines the
 			 * set of flags used for this handler.
 			 */
-			for (i = num_object_defs - 1;
+			for (i = num_method_defs - 1;
 			     i >= 0 && !method_defs[i]->handler; i--)
 				;
 			hash->methods[min_id++] = method;

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #22 lore raw

PATCH 7/15 IB/uverbs: Use __aligned_u64 for uapi headers

In reply to a message not in the archive

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

This has no impact on the structure layout since these structs already have their u64s already properly aligned, but it does document that we have this requirement for 32 bit compatibility.

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 include/uapi/rdma/rdma_user_ioctl.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

include/uapi/rdma/rdma_user_ioctl.h +2 -2

index 03557b5f9aa6..46de0885e800 100644
@@ -65,7 +65,7 @@ struct ib_uverbs_attr {
 	__u16 len;		/* only for pointers */
 	__u16 flags;		/* combination of UVERBS_ATTR_F_XXXX */
 	__u16 reserved;
-	__u64 data;		/* ptr to command, inline data or idr/fd */
+	__aligned_u64 data;	/* ptr to command, inline data or idr/fd */
 };
 
 struct ib_uverbs_ioctl_hdr {

@@ -73,7 +73,7 @@ struct ib_uverbs_ioctl_hdr {
 	__u16 object_id;
 	__u16 method_id;
 	__u16 num_attrs;
-	__u64 reserved;
+	__aligned_u64 reserved;
 	struct ib_uverbs_attr  attrs[0];
 };

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #25 lore raw

PATCH 8/15 IB/uverbs: Add ioctl support for 32bit processes

In reply to a message not in the archive

From: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>

32 bit processes running on a 64 bit kernel call compat_ioctl so that implementations can revise any structure layout issues. Point compat_ioctl at our normal ioctl because:

- All our structures are designed to be the same on 32 and 64 bit, ie we use __aligned_u64 when required and are careful to manage padding.

- Any pointers are stored in u64's and userspace is expected to prepare them properly.

Signed-off-by Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/uverbs_main.c | 2 ++
 1 file changed, 2 insertions(+)

drivers/infiniband/core/uverbs_main.c +2 -0

index 395a3b091229..cd72555ad457 100644
@@ -942,6 +942,7 @@ static const struct file_operations uverbs_fops = {
 	.llseek	 = no_llseek,
 #if IS_ENABLED(CONFIG_INFINIBAND_EXP_USER_ACCESS)
 	.unlocked_ioctl = ib_uverbs_ioctl,
+	.compat_ioctl = ib_uverbs_ioctl,
 #endif
 };

@@ -954,6 +955,7 @@ static const struct file_operations uverbs_mmap_fops = {
 	.llseek	 = no_llseek,
 #if IS_ENABLED(CONFIG_INFINIBAND_EXP_USER_ACCESS)
 	.unlocked_ioctl = ib_uverbs_ioctl,
+	.compat_ioctl = ib_uverbs_ioctl,
 #endif
 };

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #32 lore raw

PATCH 9/15 IB/uverbs: Fix possible oops with duplicate ioctl attributes

In reply to a message not in the archive

From: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>

If the same attribute is listed twice by the user in the ioctl attribute list then error unwind can cause the kernel to deref garbage.

This happens when an object with WRITE access is sent twice. The second parse properly fails but corrupts the state required for the error unwind it triggers.

Fixing this by making duplicates in the attribute list invalid. This is not something we need to support.

The ioctl interface is currently recommended to be disabled in kConfig.

Signed-off-by Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/uverbs_ioctl.c | 3 +++
 1 file changed, 3 insertions(+)

drivers/infiniband/core/uverbs_ioctl.c +3 -0

index d96dc1d17be1..339b85145044 100644
@@ -59,6 +59,9 @@ static int uverbs_process_attr(struct ib_device *ibdev,
 			return 0;
 	}
 
+	if (test_bit(attr_id, attr_bundle_h->valid_bitmap))
+		return -EINVAL;
+
 	spec = &attr_spec_bucket->attrs[attr_id];
 	e = &elements[attr_id];
 	e->uattr = uattr_ptr;

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #33 lore raw

PATCH 10/15 IB/uverbs: Hold the uobj write lock after allocate

In reply to a message not in the archive

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

This clarifies the design intention that time between allocate and commit has the uobj exclusive to the caller. We already guarantee this by delaying publishing the uobj pointer via idr_insert, fd_install, list_add, etc.

Additionally holding the usecnt lock during this period provides extra clarity and more protection against future mistakes.

Fixes 3832125624b7 ("IB/core: Add support for idr types")

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/rdma_core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

drivers/infiniband/core/rdma_core.c +10 -1

index 85b5ee4defa4..82afb0cdcca3 100644
@@ -141,7 +141,12 @@ static struct ib_uobject *alloc_uobj(struct ib_ucontext *context,
 	 */
 	uobj->context = context;
 	uobj->type = type;
-	atomic_set(&uobj->usecnt, 0);
+	/*
+	 * Allocated objects start out as write locked to deny any other
+	 * syscalls from accessing them until they are commited. See
+	 * rdma_alloc_commit_uobject
+	 */
+	atomic_set(&uobj->usecnt, -1);
 	kref_init(&uobj->ref);
 
 	return uobj;

@@ -527,6 +532,10 @@ int rdma_alloc_commit_uobject(struct ib_uobject *uobj)
 		return ret;
 	}
 
+	/* matches atomic_set(-1) in alloc_uobj */
+	lockdep_check(uobj, true);
+	atomic_set(&uobj->usecnt, 0);
+
 	uobj->type->type_class->alloc_commit(uobj);
 	up_read(&uobj->context->cleanup_rwsem);

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #34 lore raw

PATCH 11/15 RDMA/uverbs: Protect from races between lookup and destroy of uobjects

In reply to a message not in the archive

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

The race is between lookup_get_idr_uobject and
uverbs_idr_remove_uobj -> uverbs_uobject_put.

We deliberately do not call sychronize_rcu after the idr_remove in uverbs_idr_remove_uobj for performance reasons, instead we call kfree_rcu() during uverbs_uobject_put.

However, this means we can obtain pointers to uobj's that have already been released and must protect against krefing them using kref_get_unless_zero.

==================================================================

BUG: KASAN: use-after-free in copy_ah_attr_from_uverbs.isra.2+0x860/0xa00
Read of size 4 at addr ffff88005fda1ac8 by task syz-executor2/441

CPU: 1 PID: 441 Comm: syz-executor2 Not tainted 4.15.0-rc2+ #56
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
Call Trace:
dump_stack+0x8d/0xd4
print_address_description+0x73/0x290
kasan_report+0x25c/0x370
? copy_ah_attr_from_uverbs.isra.2+0x860/0xa00
copy_ah_attr_from_uverbs.isra.2+0x860/0xa00
? uverbs_try_lock_object+0x68/0xc0
? modify_qp.isra.7+0xdc4/0x10e0
modify_qp.isra.7+0xdc4/0x10e0
ib_uverbs_modify_qp+0xfe/0x170
? ib_uverbs_query_qp+0x970/0x970
? __lock_acquire+0xa11/0x1da0
ib_uverbs_write+0x55a/0xad0
? ib_uverbs_query_qp+0x970/0x970
? ib_uverbs_query_qp+0x970/0x970
? ib_uverbs_open+0x760/0x760
? futex_wake+0x147/0x410
? sched_clock_cpu+0x18/0x180
? check_prev_add+0x1680/0x1680
? do_futex+0x3b6/0xa30
? sched_clock_cpu+0x18/0x180
__vfs_write+0xf7/0x5c0
? ib_uverbs_open+0x760/0x760
? kernel_read+0x110/0x110
? lock_acquire+0x370/0x370
? __fget+0x264/0x3b0
vfs_write+0x18a/0x460
SyS_write+0xc7/0x1a0
? SyS_read+0x1a0/0x1a0
? trace_hardirqs_on_thunk+0x1a/0x1c
entry_SYSCALL_64_fastpath+0x18/0x85
RIP: 0033:0x448e29
RSP: 002b:00007f443fee0c58 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00007f443fee16bc RCX: 0000000000448e29
RDX: 0000000000000078 RSI: 00000000209f8000 RDI: 0000000000000012
RBP: 000000000070bea0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
R13: 0000000000008e98 R14: 00000000006ebf38 R15: 0000000000000000

Allocated by task 1:
kmem_cache_alloc_trace+0x16c/0x2f0
mlx5_alloc_cmd_msg+0x12e/0x670
cmd_exec+0x419/0x1810
mlx5_cmd_exec+0x40/0x70
mlx5_core_mad_ifc+0x187/0x220
mlx5_MAD_IFC+0xd7/0x1b0
mlx5_query_mad_ifc_gids+0x1f3/0x650
mlx5_ib_query_gid+0xa4/0xc0
ib_query_gid+0x152/0x1a0
ib_query_port+0x21e/0x290
mlx5_port_immutable+0x30f/0x490
ib_register_device+0x5dd/0x1130
mlx5_ib_add+0x3e7/0x700
mlx5_add_device+0x124/0x510
mlx5_register_interface+0x11f/0x1c0
mlx5_ib_init+0x56/0x61
do_one_initcall+0xa3/0x250
kernel_init_freeable+0x309/0x3b8
kernel_init+0x14/0x180
ret_from_fork+0x24/0x30

Freed by task 1:
kfree+0xeb/0x2f0
mlx5_free_cmd_msg+0xcd/0x140
cmd_exec+0xeba/0x1810
mlx5_cmd_exec+0x40/0x70
mlx5_core_mad_ifc+0x187/0x220
mlx5_MAD_IFC+0xd7/0x1b0
mlx5_query_mad_ifc_gids+0x1f3/0x650
mlx5_ib_query_gid+0xa4/0xc0
ib_query_gid+0x152/0x1a0
ib_query_port+0x21e/0x290
mlx5_port_immutable+0x30f/0x490
ib_register_device+0x5dd/0x1130
mlx5_ib_add+0x3e7/0x700
mlx5_add_device+0x124/0x510
mlx5_register_interface+0x11f/0x1c0
mlx5_ib_init+0x56/0x61
do_one_initcall+0xa3/0x250
kernel_init_freeable+0x309/0x3b8
kernel_init+0x14/0x180
ret_from_fork+0x24/0x30

The buggy address belongs to the object at ffff88005fda1ab0
which belongs to the cache kmalloc-32 of size 32
The buggy address is located 24 bytes inside of
32-byte region [ffff88005fda1ab0, ffff88005fda1ad0)
The buggy address belongs to the page:
page:00000000d5655c19 count:1 mapcount:0 mapping: (null)
index:0xffff88005fda1fc0
flags: 0x4000000000000100(slab)
raw: 4000000000000100 0000000000000000 ffff88005fda1fc0 0000000180550008
raw: ffffea00017f6780 0000000400000004 ffff88006c803980 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffff88005fda1980: fc fc fb fb fb fb fc fc fb fb fb fb fc fc fb fb
ffff88005fda1a00: fb fb fc fc fb fb fb fb fc fc 00 00 00 00 fc fc
ffff88005fda1a80: fb fb fb fb fc fc fb fb fb fb fc fc fb fb fb fb
ffff88005fda1b00: fc fc 00 00 00 00 fc fc fb fb fb fb fc fc fb fb
ffff88005fda1b80: fb fb fc fc fb fb fb fb fc fc fb fb fb fb fc fc

==================================================================@

Cc syzkaller <syzkaller-/[email protected]>

Cc <[email protected]> # 4.11

Fixes 3832125624b7 ("IB/core: Add support for idr types")

Reported-by Noa Osherovich <noaos-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/core/rdma_core.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

drivers/infiniband/core/rdma_core.c +9 -1

index 82afb0cdcca3..3ec5c7926f1c 100644
@@ -201,7 +201,15 @@ static struct ib_uobject *lookup_get_idr_uobject(const struct uverbs_obj_type *t
 		goto free;
 	}
 
-	uverbs_uobject_get(uobj);
+	/*
+	 * The idr_find is guaranteed to return a pointer to something that
+	 * isn't freed yet, or NULL, as the free after idr_remove goes through
+	 * kfree_rcu(). However the object may still have been released and
+	 * kfree() could be called at any time.
+	 */
+	if (!kref_get_unless_zero(&uobj->ref))
+		uobj = ERR_PTR(-ENOENT);
+
 free:
 	rcu_read_unlock();
 	return uobj;

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #35 lore raw

PATCH 12/15 IB/uverbs: Tidy lockdep_check

In reply to a message not in the archive

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

This is really being used as an asssert that the expected usecnt is being held and implicitly that the usecnt is valid. Rename it to assert_uverbs_usecnt and tighten the checks to only accept valid values of usecnt (eg 0 and < -1 are invalid).

Fixes 3832125624b7 ("IB/core: Add support for idr types")

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/rdma_core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/infiniband/core/rdma_core.c +7 -7

index 3ec5c7926f1c..7aee1c0b9884 100644
@@ -412,13 +412,13 @@ static int __must_check remove_commit_fd_uobject(struct ib_uobject *uobj,
 	return ret;
 }
 
-static void lockdep_check(struct ib_uobject *uobj, bool exclusive)
+static void assert_uverbs_usecnt(struct ib_uobject *uobj, bool exclusive)
 {
 #ifdef CONFIG_LOCKDEP
 	if (exclusive)
-		WARN_ON(atomic_read(&uobj->usecnt) > 0);
+		WARN_ON(atomic_read(&uobj->usecnt) != -1);
 	else
-		WARN_ON(atomic_read(&uobj->usecnt) == -1);
+		WARN_ON(atomic_read(&uobj->usecnt) <= 0);
 #endif
 }

@@ -457,7 +457,7 @@ int __must_check rdma_remove_commit_uobject(struct ib_uobject *uobj)
 		WARN(true, "ib_uverbs: Cleanup is running while removing an uobject\n");
 		return 0;
 	}
-	lockdep_check(uobj, true);
+	assert_uverbs_usecnt(uobj, true);
 	ret = _rdma_remove_commit_uobject(uobj, RDMA_REMOVE_DESTROY);
 
 	up_read(&ucontext->cleanup_rwsem);

@@ -487,7 +487,7 @@ int rdma_explicit_destroy(struct ib_uobject *uobject)
 		WARN(true, "ib_uverbs: Cleanup is running while removing an uobject\n");
 		return 0;
 	}
-	lockdep_check(uobject, true);
+	assert_uverbs_usecnt(uobject, true);
 	ret = uobject->type->type_class->remove_commit(uobject,
 						       RDMA_REMOVE_DESTROY);
 	if (ret)

@@ -541,7 +541,7 @@ int rdma_alloc_commit_uobject(struct ib_uobject *uobj)
 	}
 
 	/* matches atomic_set(-1) in alloc_uobj */
-	lockdep_check(uobj, true);
+	assert_uverbs_usecnt(uobj, true);
 	atomic_set(&uobj->usecnt, 0);
 
 	uobj->type->type_class->alloc_commit(uobj);

@@ -578,7 +578,7 @@ static void lookup_put_fd_uobject(struct ib_uobject *uobj, bool exclusive)
 
 void rdma_lookup_put_uobject(struct ib_uobject *uobj, bool exclusive)
 {
-	lockdep_check(uobj, exclusive);
+	assert_uverbs_usecnt(uobj, exclusive);
 	uobj->type->type_class->lookup_put(uobj, exclusive);
 	/*
 	 * In order to unlock an object, either decrease its usecnt for

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #40 lore raw

PATCH 13/15 IB/uverbs: Tidy uverbs_uobject_add

In reply to a message not in the archive

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Maintaining the uobjects list is mandatory, hoist it into the common rdma_alloc_commit_uobject() function and inline it as there is now only one caller.

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/rdma_core.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

drivers/infiniband/core/rdma_core.c +4 -9

index 7aee1c0b9884..1cb6849b2231 100644
@@ -350,13 +350,6 @@ struct ib_uobject *rdma_alloc_begin_uobject(const struct uverbs_obj_type *type,
 	return type->type_class->alloc_begin(type, ucontext);
 }
 
-static void uverbs_uobject_add(struct ib_uobject *uobject)
-{
-	mutex_lock(&uobject->context->uobjects_lock);
-	list_add(&uobject->list, &uobject->context->uobjects);
-	mutex_unlock(&uobject->context->uobjects_lock);
-}
-
 static int __must_check remove_commit_idr_uobject(struct ib_uobject *uobj,
 						  enum rdma_remove_reason why)
 {

@@ -501,7 +494,6 @@ int rdma_explicit_destroy(struct ib_uobject *uobject)
 
 static void alloc_commit_idr_uobject(struct ib_uobject *uobj)
 {
-	uverbs_uobject_add(uobj);
 	spin_lock(&uobj->context->ufile->idr_lock);
 	/*
 	 * We already allocated this IDR with a NULL object, so

@@ -517,7 +509,6 @@ static void alloc_commit_fd_uobject(struct ib_uobject *uobj)
 	struct ib_uobject_file *uobj_file =
 		container_of(uobj, struct ib_uobject_file, uobj);
 
-	uverbs_uobject_add(&uobj_file->uobj);
 	fd_install(uobj_file->uobj.id, uobj->object);
 	/* This shouldn't be used anymore. Use the file object instead */
 	uobj_file->uobj.id = 0;

@@ -544,6 +535,10 @@ int rdma_alloc_commit_uobject(struct ib_uobject *uobj)
 	assert_uverbs_usecnt(uobj, true);
 	atomic_set(&uobj->usecnt, 0);
 
+	mutex_lock(&uobj->context->uobjects_lock);
+	list_add(&uobj->list, &uobj->context->uobjects);
+	mutex_unlock(&uobj->context->uobjects_lock);
+
 	uobj->type->type_class->alloc_commit(uobj);
 	up_read(&uobj->context->cleanup_rwsem);

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #47 lore raw

PATCH 14/15 IB/uverbs: Fix unbalanced unlock on error path for rdma_explicit_destroy

In reply to a message not in the archive

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

If remove_commit fails then the lock is left locked while the uobj still exists. Eventually the kernel will deadlock.

lockdep detects this and says:

test/4221 is leaving the kernel with locks still held!
1 lock held by test/4221:
#0: (&ucontext->cleanup_rwsem){.+.+}, at: [<000000001e5c7523>] rdma_explicit_destroy+0x37/0x120 [ib_uverbs]

Fixes 4da70da23e9b ("IB/core: Explicitly destroy an object while keeping uobject")

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
 drivers/infiniband/core/rdma_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/infiniband/core/rdma_core.c +3 -2

index 1cb6849b2231..e2c2aa424e4d 100644
@@ -484,12 +484,13 @@ int rdma_explicit_destroy(struct ib_uobject *uobject)
 	ret = uobject->type->type_class->remove_commit(uobject,
 						       RDMA_REMOVE_DESTROY);
 	if (ret)
-		return ret;
+		goto out;
 
 	uobject->type = &null_obj_type;
 
+out:
 	up_read(&ucontext->cleanup_rwsem);
-	return 0;
+	return ret;
 }
 
 static void alloc_commit_idr_uobject(struct ib_uobject *uobj)

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 10:18 UTC #50 lore raw

PATCH 15/15 RDMA/uverbs: Protect from command mask overflow

In reply to a message not in the archive

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

The number of commands supported by the uverbs is less than 64, so let's check it in advance that the supplied command is below that limit.

================================================================================

UBSAN: Undefined behaviour in
drivers/infiniband/core/uverbs_main.c:647:21
shift exponent 207 is too large for 64-bit type 'long long unsigned int'
CPU: 0 PID: 446 Comm: syz-executor3 Not tainted 4.15.0-rc2+ #61
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.7.5-0-ge51488c-20140602_164612-nilsson.home.kraxel.org 04/01/2014
Call Trace:
dump_stack+0xde/0x164
? dma_virt_map_sg+0x22c/0x22c
ubsan_epilogue+0xe/0x81
__ubsan_handle_shift_out_of_bounds+0x293/0x2f7
? debug_check_no_locks_freed+0x340/0x340
? __ubsan_handle_load_invalid_value+0x19b/0x19b
? lock_acquire+0x440/0x440
? lock_acquire+0x19d/0x440
? __might_fault+0xf4/0x240
? ib_uverbs_write+0x68d/0xe20
ib_uverbs_write+0x68d/0xe20
? __lock_acquire+0xcf7/0x3940
? uverbs_devnode+0x110/0x110
? cyc2ns_read_end+0x10/0x10
? sched_clock_cpu+0x18/0x200
? sched_clock_cpu+0x18/0x200
__vfs_write+0x10d/0x700
? uverbs_devnode+0x110/0x110
? kernel_read+0x170/0x170
? __fget+0x35b/0x5d0
? security_file_permission+0x93/0x260
vfs_write+0x1b0/0x550
SyS_write+0xc7/0x1a0
? SyS_read+0x1a0/0x1a0
? trace_hardirqs_on_thunk+0x1a/0x1c
entry_SYSCALL_64_fastpath+0x18/0x85
RIP: 0033:0x448e29
RSP: 002b:00007f033f567c58 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00007f033f5686bc RCX: 0000000000448e29
RDX: 0000000000000060 RSI: 0000000020001000 RDI: 0000000000000012
RBP: 000000000070bea0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
R13: 00000000000056a0 R14: 00000000006e8740 R15: 0000000000000000
================================================================================

Cc syzkaller <syzkaller-/[email protected]>

Cc <[email protected]> # 4.5

Fixes 2dbd5186a39c ("IB/core: IB/core: Allow legacy verbs through extended interfaces")

Reported-by Noa Osherovich <noaos-VPRAkNaXOzVWk0Htik3J/[email protected]>

Reviewed-by Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/core/uverbs_main.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

drivers/infiniband/core/uverbs_main.c +20 -7

index cd72555ad457..b36cb12b3f38 100644
@@ -650,12 +650,21 @@ static int verify_command_mask(struct ib_device *ib_dev, __u32 command)
 	return -1;
 }
 
+static bool verify_command_idx(__u32 command, bool extended)
+{
+	if (extended)
+		return command < ARRAY_SIZE(uverbs_ex_cmd_table);
+
+	return command < ARRAY_SIZE(uverbs_cmd_table);
+}
+
 static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 			     size_t count, loff_t *pos)
 {
 	struct ib_uverbs_file *file = filp->private_data;
 	struct ib_device *ib_dev;
 	struct ib_uverbs_cmd_hdr hdr;
+	bool extended_command;
 	__u32 command;
 	__u32 flags;
 	int srcu_key;

@@ -688,6 +697,15 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	}
 
 	command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
+	flags = (hdr.command &
+		 IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
+
+	extended_command = flags & IB_USER_VERBS_CMD_FLAG_EXTENDED;
+	if (!verify_command_idx(command, extended_command)) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	if (verify_command_mask(ib_dev, command)) {
 		ret = -EOPNOTSUPP;
 		goto out;

@@ -699,12 +717,8 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		goto out;
 	}
 
-	flags = (hdr.command &
-		 IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
-
 	if (!flags) {
-		if (command >= ARRAY_SIZE(uverbs_cmd_table) ||
-		    !uverbs_cmd_table[command]) {
+		if (!uverbs_cmd_table[command]) {
 			ret = -EINVAL;
 			goto out;
 		}

@@ -725,8 +739,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		struct ib_udata uhw;
 		size_t written_count = count;
 
-		if (command >= ARRAY_SIZE(uverbs_ex_cmd_table) ||
-		    !uverbs_ex_cmd_table[command]) {
+		if (!uverbs_ex_cmd_table[command]) {
 			ret = -ENOSYS;
 			goto out;
 		}

Signature · 6 lines

2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dennis Dalessandro

2018-02-13 16:06 UTC #4 lore raw

Re: [PATCH rdma-rc 01/15] IB/ipoib: Do not warn if IPoIB debugfs doesn't exist

In reply to a message not in the archive

drivers/infiniband/ulp/ipoib/ipoib_fs.c void ipoib_delete_debug_files(struct net_device *dev) · line 286

Quoted 8 lines · Leon Romanovsky

  	struct ipoib_dev_priv *priv = ipoib_priv(dev);

drivers/infiniband/ulp/ipoib/ipoib_fs.c +0 -2

-	WARN_ONCE(!priv->mcg_dentry, "null mcg debug file\n");
-	WARN_ONCE(!priv->path_dentry, "null path debug file\n");
  	debugfs_remove(priv->mcg_dentry);
  	debugfs_remove(priv->path_dentry);
  	priv->mcg_dentry = priv->path_dentry = NULL;

Do we really want to keep sending stuff like this for RC? I get that it's a trivial patch and all, but it's not actually fixing anything. Just a console spam that only happens once.

-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Dennis Dalessandro

Reviewed-by

2018-02-13 16:10 UTC #20 lore raw

Re: [PATCH rdma-rc 05/15] IB/uverbs: Use u64_to_user_ptr() not a union

In reply to a message not in the archive

Quoted 6 lines · Leon Romanovsky

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>
The union approach will get the endianness wrong sometimes if the kernel's pointer size is 32 bits resulting in EFAULTs when trying to copy to/from user.

Quoted 6 lines

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
  drivers/infiniband/core/uverbs_std_types.c |  4 ++--
  include/rdma/uverbs_ioctl.h                | 10 ++++------
  2 files changed, 6 insertions(+), 8 deletions(-)

Reviewed-by Dennis Dalessandro <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Dennis Dalessandro

Reviewed-by

2018-02-13 16:12 UTC #24 lore raw

Re: [PATCH rdma-rc 07/15] IB/uverbs: Use __aligned_u64 for uapi headers

In reply to a message not in the archive

Quoted 8 lines · Leon Romanovsky

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>
This has no impact on the structure layout since these structs already have their u64s already properly aligned, but it does document that we have this requirement for 32 bit compatibility.
Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>

Reviewed-by Dennis Dalessandro <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Dennis Dalessandro

2018-02-13 16:56 UTC #27 lore raw

Re: [PATCH rdma-rc 08/15] IB/uverbs: Add ioctl support for 32bit processes

In reply to a message not in the archive

Quoted 33 lines · Leon Romanovsky

From: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>
32 bit processes running on a 64 bit kernel call compat_ioctl so that implementations can revise any structure layout issues. Point compat_ioctl at our normal ioctl because:
- All our structures are designed to be the same on 32 and 64 bit, ie we use __aligned_u64 when required and are careful to manage padding.
- Any pointers are stored in u64's and userspace is expected to prepare them properly.
Signed-off-by Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
---
  drivers/infiniband/core/uverbs_main.c | 2 ++
  1 file changed, 2 insertions(+)
drivers/infiniband/core/uverbs_main.c +1 -0
index 395a3b091229..cd72555ad457 100644
@@ -942,6 +942,7 @@ static const struct file_operations uverbs_fops = {
  	.llseek	 = no_llseek,
  #if IS_ENABLED(CONFIG_INFINIBAND_EXP_USER_ACCESS)
  	.unlocked_ioctl = ib_uverbs_ioctl,
+	.compat_ioctl = ib_uverbs_ioctl,
  #endif
  };
 

drivers/infiniband/core/uverbs_main.c static const struct file_operations uverbs_mmap_fops = { · line 961

Quoted 7 lines

drivers/infiniband/core/uverbs_main.c +1 -0

@@ -954,6 +955,7 @@ static const struct file_operations uverbs_mmap_fops = {
  	.llseek	 = no_llseek,
  #if IS_ENABLED(CONFIG_INFINIBAND_EXP_USER_ACCESS)
  	.unlocked_ioctl = ib_uverbs_ioctl,
+	.compat_ioctl = ib_uverbs_ioctl,
  #endif
  };

Is this patch really necessary? Doesn't sys_ioctl() just end up calling unlocked_ioctl() if .compat_ioctl is not set? I guess nothing wrong with being explicit here, but this is just a clean-up then, and not an -rc patch.

-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Dennis Dalessandro

2018-02-13 17:00 UTC #11 lore raw

Re: [PATCH rdma-rc 02/15] RDMA/restrack: Remove unimplemented XRCD object

In reply to a message not in the archive

Quoted 41 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

Resource tracking of XRCD objects is not implemented in current version of restrack and hence can be removed.

Fixes 02d8883f520e ("RDMA/restrack: Add general infrastructure to track RDMA resources")

Signed-off-by Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
  drivers/infiniband/core/restrack.c | 5 -----
  include/rdma/restrack.h            | 4 ----
  2 files changed, 9 deletions(-)

drivers/infiniband/core/restrack.c +0 -5

index 857637bf46da..d8dc709a3715 100644
@@ -63,7 +63,6 @@ static struct ib_device *res_to_dev(struct rdma_restrack_entry *res)
  {
  	enum rdma_restrack_type type = res->type;
  	struct ib_device *dev;
-	struct ib_xrcd *xrcd;
  	struct ib_pd *pd;
  	struct ib_cq *cq;
  	struct ib_qp *qp;

@@ -81,10 +80,6 @@ static struct ib_device *res_to_dev(struct rdma_restrack_entry *res)
  		qp = container_of(res, struct ib_qp, res);
  		dev = qp->device;
  		break;
-	case RDMA_RESTRACK_XRCD:
-		xrcd = container_of(res, struct ib_xrcd, res);
-		dev = xrcd->device;
-		break;
  	default:
  		WARN_ONCE(true, "Wrong resource tracking type %u\n", type);
  		return NULL;

include/rdma/restrack.h +0 -0

index c2d81167c858..2cdf8dcf4bdc 100644

include/rdma/restrack.h enum rdma_restrack_type { · line 33

Quoted 10 lines

include/rdma/restrack.h +0 -4

@@ -28,10 +28,6 @@ enum rdma_restrack_type {
  	 * @RDMA_RESTRACK_QP: Queue pair (QP)
  	 */
  	RDMA_RESTRACK_QP,
-	/**
-	 * @RDMA_RESTRACK_XRCD: XRC domain (XRCD)
-	 */
-	RDMA_RESTRACK_XRCD,
  	/**
  	 * @RDMA_RESTRACK_MAX: Last entry, used for array dclarations
  	 */

Not -rc material in my opinion.

-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Dennis Dalessandro

2018-02-13 17:09 UTC #42 lore raw

Re: [PATCH rdma-rc 13/15] IB/uverbs: Tidy uverbs_uobject_add

In reply to a message not in the archive

Quoted 48 lines · Leon Romanovsky

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Maintaining the uobjects list is mandatory, hoist it into the common rdma_alloc_commit_uobject() function and inline it as there is now only one caller.

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
  drivers/infiniband/core/rdma_core.c | 13 ++++---------
  1 file changed, 4 insertions(+), 9 deletions(-)

drivers/infiniband/core/rdma_core.c +0 -9

index 7aee1c0b9884..1cb6849b2231 100644
@@ -350,13 +350,6 @@ struct ib_uobject *rdma_alloc_begin_uobject(const struct uverbs_obj_type *type,
  	return type->type_class->alloc_begin(type, ucontext);
  }
 
-static void uverbs_uobject_add(struct ib_uobject *uobject)
-{
-	mutex_lock(&uobject->context->uobjects_lock);
-	list_add(&uobject->list, &uobject->context->uobjects);
-	mutex_unlock(&uobject->context->uobjects_lock);
-}
-
  static int __must_check remove_commit_idr_uobject(struct ib_uobject *uobj,
  						  enum rdma_remove_reason why)
  {

@@ -501,7 +494,6 @@ int rdma_explicit_destroy(struct ib_uobject *uobject)
 
  static void alloc_commit_idr_uobject(struct ib_uobject *uobj)
  {
-	uverbs_uobject_add(uobj);
  	spin_lock(&uobj->context->ufile->idr_lock);
  	/*
  	 * We already allocated this IDR with a NULL object, so

@@ -517,7 +509,6 @@ static void alloc_commit_fd_uobject(struct ib_uobject *uobj)
  	struct ib_uobject_file *uobj_file =
  		container_of(uobj, struct ib_uobject_file, uobj);
 
-	uverbs_uobject_add(&uobj_file->uobj);
  	fd_install(uobj_file->uobj.id, uobj->object);
  	/* This shouldn't be used anymore. Use the file object instead */
  	uobj_file->uobj.id = 0;

drivers/infiniband/core/rdma_core.c int rdma_alloc_commit_uobject(struct ib_uobject *uobj) · line 544

Quoted 10 lines

drivers/infiniband/core/rdma_core.c +4 -0

@@ -544,6 +535,10 @@ int rdma_alloc_commit_uobject(struct ib_uobject *uobj)
  	assert_uverbs_usecnt(uobj, true);
  	atomic_set(&uobj->usecnt, 0);
 
+	mutex_lock(&uobj->context->uobjects_lock);
+	list_add(&uobj->list, &uobj->context->uobjects);
+	mutex_unlock(&uobj->context->uobjects_lock);
+
  	uobj->type->type_class->alloc_commit(uobj);
  	up_read(&uobj->context->cleanup_rwsem);

From the subject it sounds like you are doing a cosmetic clean-up, but this is doing a bit more. You are ensuring the list_add is always going to be called by putting it in the common path. What about making that be the subject?

-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Dennis Dalessandro

2018-02-13 17:10 UTC #37 lore raw

Re: [PATCH rdma-rc 12/15] IB/uverbs: Tidy lockdep_check

In reply to a message not in the archive

Quoted 64 lines · Leon Romanovsky

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Fixes 3832125624b7 ("IB/core: Add support for idr types")

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

---
  drivers/infiniband/core/rdma_core.c | 14 +++++++-------
  1 file changed, 7 insertions(+), 7 deletions(-)

drivers/infiniband/core/rdma_core.c +6 -6

index 3ec5c7926f1c..7aee1c0b9884 100644
@@ -412,13 +412,13 @@ static int __must_check remove_commit_fd_uobject(struct ib_uobject *uobj,
  	return ret;
  }
 
-static void lockdep_check(struct ib_uobject *uobj, bool exclusive)
+static void assert_uverbs_usecnt(struct ib_uobject *uobj, bool exclusive)
  {
  #ifdef CONFIG_LOCKDEP
  	if (exclusive)
-		WARN_ON(atomic_read(&uobj->usecnt) > 0);
+		WARN_ON(atomic_read(&uobj->usecnt) != -1);
  	else
-		WARN_ON(atomic_read(&uobj->usecnt) == -1);
+		WARN_ON(atomic_read(&uobj->usecnt) <= 0);
  #endif
  }

@@ -457,7 +457,7 @@ int __must_check rdma_remove_commit_uobject(struct ib_uobject *uobj)
  		WARN(true, "ib_uverbs: Cleanup is running while removing an uobject\n");
  		return 0;
  	}
-	lockdep_check(uobj, true);
+	assert_uverbs_usecnt(uobj, true);
  	ret = _rdma_remove_commit_uobject(uobj, RDMA_REMOVE_DESTROY);
 
  	up_read(&ucontext->cleanup_rwsem);

@@ -487,7 +487,7 @@ int rdma_explicit_destroy(struct ib_uobject *uobject)
  		WARN(true, "ib_uverbs: Cleanup is running while removing an uobject\n");
  		return 0;
  	}
-	lockdep_check(uobject, true);
+	assert_uverbs_usecnt(uobject, true);
  	ret = uobject->type->type_class->remove_commit(uobject,
  						       RDMA_REMOVE_DESTROY);
  	if (ret)

@@ -541,7 +541,7 @@ int rdma_alloc_commit_uobject(struct ib_uobject *uobj)
  	}
 
  	/* matches atomic_set(-1) in alloc_uobj */
-	lockdep_check(uobj, true);
+	assert_uverbs_usecnt(uobj, true);
  	atomic_set(&uobj->usecnt, 0);
 
  	uobj->type->type_class->alloc_commit(uobj);

drivers/infiniband/core/rdma_core.c static void lookup_put_fd_uobject(struct ib_uobject *uobj, bool exclusive) · line 584

Quoted 8 lines

drivers/infiniband/core/rdma_core.c +1 -1

@@ -578,7 +578,7 @@ static void lookup_put_fd_uobject(struct ib_uobject *uobj, bool exclusive)
 
  void rdma_lookup_put_uobject(struct ib_uobject *uobj, bool exclusive)
  {
-	lockdep_check(uobj, exclusive);
+	assert_uverbs_usecnt(uobj, exclusive);
  	uobj->type->type_class->lookup_put(uobj, exclusive);
  	/*
  	 * In order to unlock an object, either decrease its usecnt for

-rc material?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Dennis Dalessandro

Reviewed-by

2018-02-13 17:11 UTC #49 lore raw

Re: [PATCH rdma-rc 14/15] IB/uverbs: Fix unbalanced unlock on error path for rdma_explicit_destroy

In reply to a message not in the archive

Quoted 7 lines · Leon Romanovsky

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>
If remove_commit fails then the lock is left locked while the uobj still exists. Eventually the kernel will deadlock.
lockdep detects this and says:

Quoted 7 lines

test/4221 is leaving the kernel with locks still held! 1 lock held by test/4221:
   #0:  (&ucontext->cleanup_rwsem){.+.+}, at: [<000000001e5c7523>] rdma_explicit_destroy+0x37/0x120 [ib_uverbs]
Fixes 4da70da23e9b ("IB/core: Explicitly destroy an object while keeping uobject")
Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>

Reviewed-by Dennis Dalessandro <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Jason Gunthorpe

2018-02-13 17:16 UTC #29 lore raw

Re: [PATCH rdma-rc 08/15] IB/uverbs: Add ioctl support for 32bit processes

In reply to a message not in the archive

Quoted 31 lines · Dennis Dalessandro

Quoted 30 lines · Leon Romanovsky
From: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>
32 bit processes running on a 64 bit kernel call compat_ioctl so that implementations can revise any structure layout issues. Point compat_ioctl at our normal ioctl because:
- All our structures are designed to be the same on 32 and 64 bit, ie we use __aligned_u64 when required and are careful to manage padding.
- Any pointers are stored in u64's and userspace is expected to prepare them properly.
Signed-off-by Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
 drivers/infiniband/core/uverbs_main.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/infiniband/core/uverbs_main.c +2 -0
index 395a3b091229..cd72555ad457 100644
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -942,6 +942,7 @@ static const struct file_operations uverbs_fops = {
 	.llseek	 = no_llseek,
 #if IS_ENABLED(CONFIG_INFINIBAND_EXP_USER_ACCESS)
 	.unlocked_ioctl = ib_uverbs_ioctl,
+	.compat_ioctl = ib_uverbs_ioctl,
 #endif
 };
@@ -954,6 +955,7 @@ static const struct file_operations uverbs_mmap_fops = {
 	.llseek	 = no_llseek,

Quoted 8 lines

drivers/infiniband/core/uverbs_main.c static const struct file_operations uverbs_fops = { · line 948
Quoted 6 lines
drivers/infiniband/core/uverbs_main.c +1 -0
 #if IS_ENABLED(CONFIG_INFINIBAND_EXP_USER_ACCESS)
 	.unlocked_ioctl = ib_uverbs_ioctl,
+	.compat_ioctl = ib_uverbs_ioctl,
 #endif
 };
 
Is this patch really necessary?

Yes. Without it 32 bit gets:

ioctl(3, _IOC(_IOC_READ|_IOC_WRITE, 0x1b, 0x1, 0x10), 0xffcaf5f8) = -1 ENOTTY (Inappropriate ioctl for device)

Quoted 3 lines

Doesn't sys_ioctl() just end up calling unlocked_ioctl() if .compat_ioctl is not set? I guess nothing wrong with being explicit here, but this is just a clean-up then, and not an -rc patch.

No, the control flow is really tortured in compat_ioctl.c, but it will only call compat_ioctl for our case.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Jason Gunthorpe

2018-02-13 17:20 UTC #44 lore raw

Re: [PATCH rdma-rc 13/15] IB/uverbs: Tidy uverbs_uobject_add

In reply to a message not in the archive

Quoted 47 lines · Dennis Dalessandro

Quoted 46 lines · Leon Romanovsky

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Maintaining the uobjects list is mandatory, hoist it into the common rdma_alloc_commit_uobject() function and inline it as there is now only one caller.

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

 drivers/infiniband/core/rdma_core.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

drivers/infiniband/core/rdma_core.c +3 -9

index 7aee1c0b9884..1cb6849b2231 100644
+++ b/drivers/infiniband/core/rdma_core.c

@@ -350,13 +350,6 @@ struct ib_uobject *rdma_alloc_begin_uobject(const struct uverbs_obj_type *type,
 	return type->type_class->alloc_begin(type, ucontext);
 }
-static void uverbs_uobject_add(struct ib_uobject *uobject)
-{
-	mutex_lock(&uobject->context->uobjects_lock);
-	list_add(&uobject->list, &uobject->context->uobjects);
-	mutex_unlock(&uobject->context->uobjects_lock);
-}
-
 static int __must_check remove_commit_idr_uobject(struct ib_uobject *uobj,
 						  enum rdma_remove_reason why)
 {

@@ -501,7 +494,6 @@ int rdma_explicit_destroy(struct ib_uobject *uobject)
 static void alloc_commit_idr_uobject(struct ib_uobject *uobj)
 {
-	uverbs_uobject_add(uobj);
 	spin_lock(&uobj->context->ufile->idr_lock);
 	/*
 	 * We already allocated this IDR with a NULL object, so

@@ -517,7 +509,6 @@ static void alloc_commit_fd_uobject(struct ib_uobject *uobj)
 	struct ib_uobject_file *uobj_file =
 		container_of(uobj, struct ib_uobject_file, uobj);
-	uverbs_uobject_add(&uobj_file->uobj);
 	fd_install(uobj_file->uobj.id, uobj->object);
 	/* This shouldn't be used anymore. Use the file object instead */
 	uobj_file->uobj.id = 0;

@@ -544,6 +535,10 @@ int rdma_alloc_commit_uobject(struct ib_uobject *uobj)
 	assert_uverbs_usecnt(uobj, true);
 	atomic_set(&uobj->usecnt, 0);
+	mutex_lock(&uobj->context->uobjects_lock);
+	list_add(&uobj->list, &uobj->context->uobjects);

Quoted 8 lines

drivers/infiniband/core/rdma_core.c int rdma_alloc_commit_uobject(struct ib_uobject *uobj) · line 544
Quoted 5 lines
drivers/infiniband/core/rdma_core.c +2 -0
+	mutex_unlock(&uobj->context->uobjects_lock);
+
 	uobj->type->type_class->alloc_commit(uobj);
 	up_read(&uobj->context->cleanup_rwsem);
 
From the subject it sounds like you are doing a cosmetic clean-up, but this is doing a bit more.

The behavior is identical before/after.

There are only two implementations of alloc_commit and both of them do the list_add as the first operation.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Jason Gunthorpe

2018-02-13 17:23 UTC #39 lore raw

Re: [PATCH rdma-rc 12/15] IB/uverbs: Tidy lockdep_check

In reply to a message not in the archive

Quoted 59 lines · Dennis Dalessandro

Quoted 58 lines · Leon Romanovsky

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Fixes 3832125624b7 ("IB/core: Add support for idr types")

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

 drivers/infiniband/core/rdma_core.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

drivers/infiniband/core/rdma_core.c +7 -6

index 3ec5c7926f1c..7aee1c0b9884 100644
+++ b/drivers/infiniband/core/rdma_core.c

@@ -412,13 +412,13 @@ static int __must_check remove_commit_fd_uobject(struct ib_uobject *uobj,
 	return ret;
 }
-static void lockdep_check(struct ib_uobject *uobj, bool exclusive)
+static void assert_uverbs_usecnt(struct ib_uobject *uobj, bool exclusive)
 {
 #ifdef CONFIG_LOCKDEP
 	if (exclusive)
-		WARN_ON(atomic_read(&uobj->usecnt) > 0);
+		WARN_ON(atomic_read(&uobj->usecnt) != -1);
 	else
-		WARN_ON(atomic_read(&uobj->usecnt) == -1);
+		WARN_ON(atomic_read(&uobj->usecnt) <= 0);
 #endif
 }

@@ -457,7 +457,7 @@ int __must_check rdma_remove_commit_uobject(struct ib_uobject *uobj)
 		WARN(true, "ib_uverbs: Cleanup is running while removing an uobject\n");
 		return 0;
 	}
-	lockdep_check(uobj, true);
+	assert_uverbs_usecnt(uobj, true);
 	ret = _rdma_remove_commit_uobject(uobj, RDMA_REMOVE_DESTROY);
 	up_read(&ucontext->cleanup_rwsem);

@@ -487,7 +487,7 @@ int rdma_explicit_destroy(struct ib_uobject *uobject)
 		WARN(true, "ib_uverbs: Cleanup is running while removing an uobject\n");
 		return 0;
 	}
-	lockdep_check(uobject, true);
+	assert_uverbs_usecnt(uobject, true);
 	ret = uobject->type->type_class->remove_commit(uobject,
 						       RDMA_REMOVE_DESTROY);
 	if (ret)

@@ -541,7 +541,7 @@ int rdma_alloc_commit_uobject(struct ib_uobject *uobj)
 	}
 	/* matches atomic_set(-1) in alloc_uobj */
-	lockdep_check(uobj, true);
+	assert_uverbs_usecnt(uobj, true);
 	atomic_set(&uobj->usecnt, 0);
 	uobj->type->type_class->alloc_commit(uobj);

@@ -578,7 +578,7 @@ static void lookup_put_fd_uobject(struct ib_uobject *uobj, bool exclusive)
 void rdma_lookup_put_uobject(struct ib_uobject *uobj, bool exclusive)
 {

Quoted 8 lines

drivers/infiniband/core/rdma_core.c static void lookup_put_fd_uobject(struct ib_uobject *uobj, bool exclusive)
Quoted 6 lines
drivers/infiniband/core/rdma_core.c +1 -1
-	lockdep_check(uobj, exclusive);
+	assert_uverbs_usecnt(uobj, exclusive);
 	uobj->type->type_class->lookup_put(uobj, exclusive);
 	/*
 	 * In order to unlock an object, either decrease its usecnt for
 
-rc material?

This one is a bit border line - but as coded the asserts were not working right, eg:

- WARN_ON(atomic_read(&uobj->usecnt) > 0);
+ WARN_ON(atomic_read(&uobj->usecnt) != -1);

Didn't trigger if usecnt == 0, which is wrong.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2018-02-13 18:09 UTC #13 lore raw

Re: [PATCH rdma-rc 02/15] RDMA/restrack: Remove unimplemented XRCD object

In reply to a message not in the archive

Quoted 46 lines · Dennis Dalessandro

Quoted 45 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

Resource tracking of XRCD objects is not implemented in current version of restrack and hence can be removed.

Fixes 02d8883f520e ("RDMA/restrack: Add general infrastructure to track RDMA resources")

Signed-off-by Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
  drivers/infiniband/core/restrack.c | 5 -----
  include/rdma/restrack.h            | 4 ----
  2 files changed, 9 deletions(-)

drivers/infiniband/core/restrack.c +0 -5

index 857637bf46da..d8dc709a3715 100644
@@ -63,7 +63,6 @@ static struct ib_device *res_to_dev(struct rdma_restrack_entry *res)
  {
  	enum rdma_restrack_type type = res->type;
  	struct ib_device *dev;
-	struct ib_xrcd *xrcd;
  	struct ib_pd *pd;
  	struct ib_cq *cq;
  	struct ib_qp *qp;

@@ -81,10 +80,6 @@ static struct ib_device *res_to_dev(struct rdma_restrack_entry *res)
  		qp = container_of(res, struct ib_qp, res);
  		dev = qp->device;
  		break;
-	case RDMA_RESTRACK_XRCD:
-		xrcd = container_of(res, struct ib_xrcd, res);
-		dev = xrcd->device;
-		break;
  	default:
  		WARN_ONCE(true, "Wrong resource tracking type %u\n", type);
  		return NULL;

include/rdma/restrack.h +0 -2

index c2d81167c858..2cdf8dcf4bdc 100644
@@ -28,10 +28,6 @@ enum rdma_restrack_type {
  	 * @RDMA_RESTRACK_QP: Queue pair (QP)
  	 */
  	RDMA_RESTRACK_QP,
-	/**
-	 * @RDMA_RESTRACK_XRCD: XRC domain (XRCD)

Quoted 8 lines

include/rdma/restrack.h enum rdma_restrack_type { · line 33
Quoted 6 lines
include/rdma/restrack.h +0 -2
-	 */
-	RDMA_RESTRACK_XRCD,
  	/**
  	 * @RDMA_RESTRACK_MAX: Last entry, used for array dclarations
  	 */
Not -rc material in my opinion.

We are in -rc1 and it is exactly the time to clean up the code which was entered a couple of weeks before.

Quoted 2 lines

-Denny

Leon Romanovsky

author

2018-02-13 18:12 UTC #6 lore raw

Re: [PATCH rdma-rc 01/15] IB/ipoib: Do not warn if IPoIB debugfs doesn't exist

In reply to a message not in the archive

Quoted 12 lines · Dennis Dalessandro

drivers/infiniband/ulp/ipoib/ipoib_fs.c void ipoib_delete_debug_files(struct net_device *dev) · line 286
Quoted 7 lines · Leon Romanovsky
  	struct ipoib_dev_priv *priv = ipoib_priv(dev);
drivers/infiniband/ulp/ipoib/ipoib_fs.c +0 -2
-	WARN_ONCE(!priv->mcg_dentry, "null mcg debug file\n");
-	WARN_ONCE(!priv->path_dentry, "null path debug file\n");
  	debugfs_remove(priv->mcg_dentry);
  	debugfs_remove(priv->path_dentry);
  	priv->mcg_dentry = priv->path_dentry = NULL;
Do we really want to keep sending stuff like this for RC? I get that it's a trivial patch and all, but it's not actually fixing anything. Just a console spam that only happens once.

It is more than simple console spam, but actual crash on the system with warning == panic.

Thanks

Quoted 2 lines

-Denny

Dennis Dalessandro

2018-02-13 18:16 UTC #15 lore raw

Re: [PATCH rdma-rc 02/15] RDMA/restrack: Remove unimplemented XRCD object

In reply to a message not in the archive

Quoted 5 lines · Leon Romanovsky

Quoted 2 lines
Not -rc material in my opinion.
We are in -rc1 and it is exactly the time to clean up the code which was entered a couple of weeks before.

Ah if that's the case then yes I totally agree.

-Denny
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Dennis Dalessandro

Reviewed-by

2018-02-13 18:44 UTC #46 lore raw

Re: [PATCH rdma-rc 13/15] IB/uverbs: Tidy uverbs_uobject_add

In reply to a message not in the archive

Quoted 57 lines · Jason Gunthorpe

Quoted 47 lines · Dennis Dalessandro

Quoted 46 lines · Leon Romanovsky

From: Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Maintaining the uobjects list is mandatory, hoist it into the common rdma_alloc_commit_uobject() function and inline it as there is now only one caller.

Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Leon Romanovsky <[email protected]>

  drivers/infiniband/core/rdma_core.c | 13 ++++---------
  1 file changed, 4 insertions(+), 9 deletions(-)

drivers/infiniband/core/rdma_core.c +3 -9

index 7aee1c0b9884..1cb6849b2231 100644
+++ b/drivers/infiniband/core/rdma_core.c

@@ -350,13 +350,6 @@ struct ib_uobject *rdma_alloc_begin_uobject(const struct uverbs_obj_type *type,
  	return type->type_class->alloc_begin(type, ucontext);
  }
-static void uverbs_uobject_add(struct ib_uobject *uobject)
-{
-	mutex_lock(&uobject->context->uobjects_lock);
-	list_add(&uobject->list, &uobject->context->uobjects);
-	mutex_unlock(&uobject->context->uobjects_lock);
-}
-
  static int __must_check remove_commit_idr_uobject(struct ib_uobject *uobj,
  						  enum rdma_remove_reason why)
  {

@@ -501,7 +494,6 @@ int rdma_explicit_destroy(struct ib_uobject *uobject)
  static void alloc_commit_idr_uobject(struct ib_uobject *uobj)
  {
-	uverbs_uobject_add(uobj);
  	spin_lock(&uobj->context->ufile->idr_lock);
  	/*
  	 * We already allocated this IDR with a NULL object, so

@@ -517,7 +509,6 @@ static void alloc_commit_fd_uobject(struct ib_uobject *uobj)
  	struct ib_uobject_file *uobj_file =
  		container_of(uobj, struct ib_uobject_file, uobj);
-	uverbs_uobject_add(&uobj_file->uobj);
  	fd_install(uobj_file->uobj.id, uobj->object);
  	/* This shouldn't be used anymore. Use the file object instead */
  	uobj_file->uobj.id = 0;

@@ -544,6 +535,10 @@ int rdma_alloc_commit_uobject(struct ib_uobject *uobj)
  	assert_uverbs_usecnt(uobj, true);
  	atomic_set(&uobj->usecnt, 0);
+	mutex_lock(&uobj->context->uobjects_lock);
+	list_add(&uobj->list, &uobj->context->uobjects);

Quoted 8 lines

drivers/infiniband/core/rdma_core.c int rdma_alloc_commit_uobject(struct ib_uobject *uobj) · line 544
Quoted 5 lines
drivers/infiniband/core/rdma_core.c +2 -0
+	mutex_unlock(&uobj->context->uobjects_lock);
+
  	uobj->type->type_class->alloc_commit(uobj);
  	up_read(&uobj->context->cleanup_rwsem);
 
From the subject it sounds like you are doing a cosmetic clean-up, but this
is doing a bit more.

Quoted 4 lines

The behavior is identical before/after.
There are only two implementations of alloc_commit and both of them do the list_add as the first operation.

Agree. To me the value of this patch is that you remove two separate function calls and do the work in a common place. That's more important than to just "Tidy" up the code. Either way I'm totally fine with the patch.

Reviewed-by Dennis Dalessandro <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Dennis Dalessandro

Reviewed-by

2018-02-13 18:45 UTC #8 lore raw

Re: [PATCH rdma-rc 01/15] IB/ipoib: Do not warn if IPoIB debugfs doesn't exist

In reply to a message not in the archive

Quoted 14 lines · Leon Romanovsky

Quoted 12 lines · Dennis Dalessandro
drivers/infiniband/ulp/ipoib/ipoib_fs.c void ipoib_delete_debug_files(struct net_device *dev) · line 286
Quoted 7 lines · Leon Romanovsky
   	struct ipoib_dev_priv *priv = ipoib_priv(dev);
drivers/infiniband/ulp/ipoib/ipoib_fs.c +0 -2
-	WARN_ONCE(!priv->mcg_dentry, "null mcg debug file\n");
-	WARN_ONCE(!priv->path_dentry, "null path debug file\n");
   	debugfs_remove(priv->mcg_dentry);
   	debugfs_remove(priv->path_dentry);
   	priv->mcg_dentry = priv->path_dentry = NULL;
Do we really want to keep sending stuff like this for RC? I get that it's a trivial patch and all, but it's not actually fixing anything. Just a console spam that only happens once.

Quoted 2 lines

It is more than simple console spam, but actual crash on the system with warning == panic.

Ok. I'll buy that.

Reviewed-by Dennis Dalessandro <[email protected]>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Dennis Dalessandro

2018-02-14 11:31 UTC #31 lore raw

Re: [PATCH rdma-rc 08/15] IB/uverbs: Add ioctl support for 32bit processes

In reply to a message not in the archive

Quoted 43 lines · Jason Gunthorpe

Quoted 31 lines · Dennis Dalessandro
Quoted 30 lines · Leon Romanovsky
From: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>
32 bit processes running on a 64 bit kernel call compat_ioctl so that implementations can revise any structure layout issues. Point compat_ioctl at our normal ioctl because:
- All our structures are designed to be the same on 32 and 64 bit, ie we use __aligned_u64 when required and are careful to manage padding.
- Any pointers are stored in u64's and userspace is expected to prepare them properly.
Signed-off-by Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Jason Gunthorpe <jgg-VPRAkNaXOzVWk0Htik3J/[email protected]>
Signed-off-by Leon Romanovsky <[email protected]>
  drivers/infiniband/core/uverbs_main.c | 2 ++
  1 file changed, 2 insertions(+)
drivers/infiniband/core/uverbs_main.c +2 -0
index 395a3b091229..cd72555ad457 100644
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -942,6 +942,7 @@ static const struct file_operations uverbs_fops = {
  	.llseek	 = no_llseek,
  #if IS_ENABLED(CONFIG_INFINIBAND_EXP_USER_ACCESS)
  	.unlocked_ioctl = ib_uverbs_ioctl,
+	.compat_ioctl = ib_uverbs_ioctl,
  #endif
  };
@@ -954,6 +955,7 @@ static const struct file_operations uverbs_mmap_fops = {
  	.llseek	 = no_llseek,
Quoted 8 lines
drivers/infiniband/core/uverbs_main.c static const struct file_operations uverbs_fops = { · line 948
Quoted 6 lines
drivers/infiniband/core/uverbs_main.c +1 -0
  #if IS_ENABLED(CONFIG_INFINIBAND_EXP_USER_ACCESS)
  	.unlocked_ioctl = ib_uverbs_ioctl,
+	.compat_ioctl = ib_uverbs_ioctl,
  #endif
  };
 
Is this patch really necessary?
Yes. Without it 32 bit gets:

Quoted 8 lines

ioctl(3, _IOC(_IOC_READ|_IOC_WRITE, 0x1b, 0x1, 0x10), 0xffcaf5f8) = -1 ENOTTY (Inappropriate ioctl for device)
Quoted 3 lines
Doesn't sys_ioctl() just end up calling unlocked_ioctl() if .compat_ioctl is not set? I guess nothing wrong with being explicit here, but this is just a clean-up then, and not an -rc patch.
No, the control flow is really tortured in compat_ioctl.c, but it will only call compat_ioctl for our case.

Ok, thanks, I'll go take another look.

-Denny

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Jason Gunthorpe

Applied

2018-02-15 22:26 UTC #51 lore raw

In reply to a message not in the archive

Quoted 26 lines · Leon Romanovsky

Hi,
This is mostly fixes for uverbs layers with two extra patches: one from Alaa, who removed annoying warning prints in IPoIB and another from Leon who removed unimplented part of restrack code.
Thanks
Alaa Hleihel (1):
IB/ipoib: Do not warn if IPoIB debugfs doesn't exist
Jason Gunthorpe (7):
IB/uverbs: Use inline data transfer for UHW_IN IB/uverbs: Use u64_to_user_ptr() not a union IB/uverbs: Use __aligned_u64 for uapi headers IB/uverbs: Hold the uobj write lock after allocate IB/uverbs: Tidy lockdep_check
IB/uverbs: Tidy uverbs_uobject_add IB/uverbs: Fix unbalanced unlock on error path for
rdma_explicit_destroy
Leon Romanovsky (3):
RDMA/restrack: Remove unimplemented XRCD object RDMA/uverbs: Protect from races between lookup and destroy of uobjects RDMA/uverbs: Protect from command mask overflow

Quoted 5 lines

Matan Barak (4):
IB/uverbs: Always the attribute size provided by the user IB/uverbs: Fix method merging in uverbs_ioctl_merge IB/uverbs: Add ioctl support for 32bit processes IB/uverbs: Fix possible oops with duplicate ioctl attributes

I applied all of these to for-rc, with some modifications to the commit messages. Thanks

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Jason Gunthorpe

2018-02-15 22:30 UTC #52 lore raw

In reply to a message not in the archive

Quoted 1 line · Leon Romanovsky

IB/uverbs: Tidy uverbs_uobject_add

Actually, I was writing my notes for the PR and I just can't defend this one as -rc, especially since I wrote it, so to -next it goes.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html