[PATCH v2 00/13] Provide udata helpers and use them in bnxt_re

[PATCH v2 00/13] Provide udata helpers and use them in bnxt_re

[Jason Gunthorpe]

Add new helpers that entirely execute the expected common patterns for
driver data uAPI forward and backwards compatibility so that drivers don't
have to open code these.

The helpers were developed by looking at the entire tree and moving every
driver to use them, but this series only converts bnxt_re as it has a
pending series to extend the driver data uAPI and the lack of correct
compatibility handling will be problematic.

This handles both the request and response side of the udata using the
following general rules:

1) The userspace can provide a longer request so long as the trailing
   part the kernel doesn't understand is all zeros.

   This provides a degree of safety if the userspace wrongly tries to use
   a new feature the kernel does not understand with some non-zero value.

   It allows a simpler rdma-core implementation because the library can
   simply always use the latest structs for the request, even if they are
   bigger. It simply has to avoid using the new members if they are not
   supported/required.

2) The userspace can provide a shorter request, the kernel will 0 pad it
   out to fill the storage. The newer kernel should understand that older
   userspace will provide 0 to new fields. The kernel has three options
   to enable new request fields:
     - Input comp_mask that says the field is supported
     - Look for non-zero values
     - Check if the udata->inlen size covers the field

   This also corrects any bugs related to not filling in request
   structures as the new helper always fully writes to the struct.

 3) The userspace can provide a shorter or longer response struct.
    If shorter the kernel reply is truncated. The kernel should be
    designed to not write to new reply field unless the userspace has
    affirmatively requested them.

    If the user buffer is longer then the kernel will zero fill it.

    Userspace has three options to enable new response fields:
      - Output comp_mask that says the field is supported
      - Look for non-zero values
      - Infer the output must be valid because the request contents demand
        it and old kernels will fail the request

Since bnxt_re has never implemented these rules correctly and now does,
provide a UCTX flag to tell userspace about it. If
BNXT_RE_UCNTX_CMASK_UAPI_COMPAT_SUPPORTED is not set then userspace must
not use any request or response fields beyond the current kernel uAPI.

Using any new fields is only possible on kernels with the flag.

A series converting all drivers to these new helpers is on github, I will
send it later:

https://github.com/jgunthorpe/linux/commits/rdma_uapi/

v2:
 - Revise the core function implementations completely to use
   EXPORT_SYMBOLs and integrate debug logging
 - Have ib_is_udata_in_empty() return errno so it can propogate the right
   code in all cases
 - Remove debug prints
 - Rebase on linus's tree
v1: https://patch.msgid.link/r/0-v1-89ea7d615ba4+636-bnxt_re_uapi_jgg@nvidia.com

Jason Gunthorpe (13):
  RDMA: Use copy_struct_from_user() instead of open coding
  RDMA/core: Add rdma_udata_to_dev()
  RDMA: Add ib_copy_validate_udata_in()
  RDMA: Add ib_copy_validate_udata_in_cm()
  RDMA: Add ib_respond_udata()
  RDMA: Add ib_is_udata_in_empty()
  RDMA: Provide documentation about the uABI compatibility rules
  RDMA/bnxt_re: Add compatibility checks to the uapi path
  RDMA/bnxt_re: Add compatibility checks to the uapi path for no data
  RDMA/bnxt_re: Add missing comp_mask validation
  RDMA/bnxt_re: Use ib_respond_udata()
  RDMA/bnxt_re: Use ib_respond_empty_udata()
  RDMA/bnxt_re: Add BNXT_RE_UCNTX_CMASK_UAPI_COMPAT_SUPPORTED

 drivers/infiniband/core/ib_core_uverbs.c |  27 +++++
 drivers/infiniband/core/rdma_core.h      |   3 +
 drivers/infiniband/core/uverbs_cmd.c     |  24 +---
 drivers/infiniband/core/uverbs_ioctl.c   |  87 +++++++++++++
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 148 ++++++++++++++++-------
 include/rdma/ib_verbs.h                  |  87 +++++++++++++
 include/rdma/uverbs_ioctl.h              | 101 ++++++++++++++++
 include/uapi/rdma/bnxt_re-abi.h          |   1 +
 8 files changed, 414 insertions(+), 64 deletions(-)


base-commit: 3f4a08e64442340f4807de63e30aef22cc308830
-- 
2.43.0

[PATCH v2 01/13] RDMA: Use copy_struct_from_user() instead of open coding

[Jason Gunthorpe]

This entire function is just open coding copy_struct_from_user(), call it
directly, it is faster anyhow.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/infiniband/core/uverbs_cmd.c | 24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 758ed4ae5f7a85..5e5f049c3845a3 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -83,28 +83,16 @@ static int uverbs_response(struct uverbs_attr_bundle *attrs, const void *resp,
 	return 0;
 }
 
-/*
- * Copy a request from userspace. If the provided 'req' is larger than the
- * user buffer then the user buffer is zero extended into the 'req'. If 'req'
- * is smaller than the user buffer then the uncopied bytes in the user buffer
- * must be zero.
- */
 static int uverbs_request(struct uverbs_attr_bundle *attrs, void *req,
 			  size_t req_len)
 {
-	if (copy_from_user(req, attrs->ucore.inbuf,
-			   min(attrs->ucore.inlen, req_len)))
-		return -EFAULT;
+	int ret;
 
-	if (attrs->ucore.inlen < req_len) {
-		memset(req + attrs->ucore.inlen, 0,
-		       req_len - attrs->ucore.inlen);
-	} else if (attrs->ucore.inlen > req_len) {
-		if (!ib_is_buffer_cleared(attrs->ucore.inbuf + req_len,
-					  attrs->ucore.inlen - req_len))
-			return -EOPNOTSUPP;
-	}
-	return 0;
+	ret = copy_struct_from_user(req, req_len, attrs->ucore.inbuf,
+				    attrs->ucore.inlen);
+	if (ret == -E2BIG)
+		ret = -EOPNOTSUPP;
+	return ret;
 }
 
 /*
-- 
2.43.0

[PATCH v2 02/13] RDMA/core: Add rdma_udata_to_dev()

[Jason Gunthorpe]

Get an ib_device out of a udata so it can be used for debug prints.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/infiniband/core/ib_core_uverbs.c | 27 ++++++++++++++++++++++++
 include/rdma/uverbs_ioctl.h              |  2 ++
 2 files changed, 29 insertions(+)

diff --git a/drivers/infiniband/core/ib_core_uverbs.c b/drivers/infiniband/core/ib_core_uverbs.c
index d3836a62a00495..bfe37a9c8a72bf 100644
--- a/drivers/infiniband/core/ib_core_uverbs.c
+++ b/drivers/infiniband/core/ib_core_uverbs.c
@@ -389,3 +389,30 @@ int rdma_user_mmap_entry_insert(struct ib_ucontext *ucontext,
 						 U32_MAX);
 }
 EXPORT_SYMBOL(rdma_user_mmap_entry_insert);
+
+/**
+ * rdma_udata_to_dev - Get a ib_device from a udata
+ * @udata: The system calls ib_udata struct
+ *
+ * The struct ib_device that is handling the uverbs call. Must not be called if
+ * udata is NULL. The result can be NULL.
+ */
+struct ib_device *rdma_udata_to_dev(struct ib_udata *udata)
+{
+	struct uverbs_attr_bundle *bundle =
+		rdma_udata_to_uverbs_attr_bundle(udata);
+
+	lockdep_assert_held(&bundle->ufile->device->disassociate_srcu);
+
+	if (bundle->context)
+		return bundle->context->device;
+
+	/*
+	 * If the context hasn't been created yet use the ufile's dev, but it
+	 * might be NULL if we are racing with disassociate.
+	 */
+	return srcu_dereference(bundle->ufile->device->ib_dev,
+				&bundle->ufile->device->disassociate_srcu);
+}
+EXPORT_SYMBOL(rdma_udata_to_dev);
+
diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index e6c0de227fad3d..bb86d8ae8a834b 100644
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -667,6 +667,8 @@ rdma_udata_to_uverbs_attr_bundle(struct ib_udata *udata)
 	(udata ? container_of(rdma_udata_to_uverbs_attr_bundle(udata)->context, \
 			      drv_dev_struct, member) : (drv_dev_struct *)NULL)
 
+struct ib_device *rdma_udata_to_dev(struct ib_udata *udata);
+
 #define IS_UVERBS_COPY_ERR(_ret)		((_ret) && (_ret) != -ENOENT)
 
 static inline const struct uverbs_attr *uverbs_attr_get(const struct uverbs_attr_bundle *attrs_bundle,
-- 
2.43.0

[PATCH v2 03/13] RDMA: Add ib_copy_validate_udata_in()

[Jason Gunthorpe]

Add a new function to consolidate the required compatibility pattern for
driver data of checking against a minimum size, and checking for unknown
trailing bytes to be zero into a function.

This new function uses the faster copy_struct_from_user() instead of
trying to directly check for zero.

Incorporate the common ibdev_dbg() logging directly into the error paths
of the helper.

Tested-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/infiniband/core/rdma_core.h    |  3 ++
 drivers/infiniband/core/uverbs_ioctl.c | 51 ++++++++++++++++++++++++++
 include/rdma/uverbs_ioctl.h            | 26 +++++++++++++
 3 files changed, 80 insertions(+)

diff --git a/drivers/infiniband/core/rdma_core.h b/drivers/infiniband/core/rdma_core.h
index 55f1e3558856f1..269b393799abbc 100644
--- a/drivers/infiniband/core/rdma_core.h
+++ b/drivers/infiniband/core/rdma_core.h
@@ -151,6 +151,9 @@ void uapi_compute_bundle_size(struct uverbs_api_ioctl_method *method_elm,
 			      unsigned int num_attrs);
 void uverbs_user_mmap_disassociate(struct ib_uverbs_file *ufile);
 
+typedef int (*uverbs_api_ioctl_handler_fn)(struct uverbs_attr_bundle *attrs);
+uverbs_api_ioctl_handler_fn uverbs_get_handler_fn(struct ib_udata *udata);
+
 extern const struct uapi_definition uverbs_def_obj_async_fd[];
 extern const struct uapi_definition uverbs_def_obj_counters[];
 extern const struct uapi_definition uverbs_def_obj_cq[];
diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index f37bb447c2306b..81798c0875ed3a 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -70,6 +70,19 @@ struct bundle_priv {
 	u64 internal_buffer[32];
 };
 
+uverbs_api_ioctl_handler_fn uverbs_get_handler_fn(struct ib_udata *udata)
+{
+	struct uverbs_attr_bundle *bundle =
+		rdma_udata_to_uverbs_attr_bundle(udata);
+	struct bundle_priv *pbundle =
+		container_of(&bundle->hdr, struct bundle_priv, bundle);
+
+	lockdep_assert_held(&bundle->ufile->device->disassociate_srcu);
+
+	return srcu_dereference(pbundle->method_elm->handler,
+				&bundle->ufile->device->disassociate_srcu);
+}
+
 /*
  * Each method has an absolute minimum amount of memory it needs to allocate,
  * precompute that amount and determine if the onstack memory can be used or
@@ -847,3 +860,41 @@ void uverbs_finalize_uobj_create(const struct uverbs_attr_bundle *bundle,
 		  pbundle->uobj_hw_obj_valid);
 }
 EXPORT_SYMBOL(uverbs_finalize_uobj_create);
+
+int _ib_copy_validate_udata_in(struct ib_udata *udata, void *req,
+			       size_t kernel_size, size_t minimum_size)
+{
+	int err;
+
+	if (udata->inlen < minimum_size) {
+		ibdev_dbg(
+			rdma_udata_to_dev(udata),
+			"System call driver input udata too small (%zu < %zu) for ioctl %ps called by %pSR\n",
+			udata->inlen, minimum_size,
+			uverbs_get_handler_fn(udata),
+			__builtin_return_address(0));
+		return -EINVAL;
+	}
+
+	err = copy_struct_from_user(req, kernel_size, udata->inbuf,
+				    udata->inlen);
+	if (err) {
+		if (err == -E2BIG) {
+			ibdev_dbg(
+				rdma_udata_to_dev(udata),
+				"System call driver input udata not zero from %zu -> %zu for ioctl %ps called by %pSR\n",
+				minimum_size, udata->inlen,
+				uverbs_get_handler_fn(udata),
+				__builtin_return_address(0));
+			return -EOPNOTSUPP;
+		}
+		ibdev_dbg(
+			rdma_udata_to_dev(udata),
+			"System call driver input udata EFAULT for ioctl %ps called by %pSR\n",
+			uverbs_get_handler_fn(udata),
+			__builtin_return_address(0));
+		return err;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(_ib_copy_validate_udata_in);
diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index bb86d8ae8a834b..5ea53a49b7c6b5 100644
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -897,6 +897,9 @@ int _uverbs_get_const_unsigned(u64 *to,
 			       size_t idx, u64 upper_bound, u64 *def_val);
 int uverbs_copy_to_struct_or_zero(const struct uverbs_attr_bundle *bundle,
 				  size_t idx, const void *from, size_t size);
+
+int _ib_copy_validate_udata_in(struct ib_udata *udata, void *req,
+			       size_t kernel_size, size_t minimum_size);
 #else
 static inline int
 uverbs_get_flags64(u64 *to, const struct uverbs_attr_bundle *attrs_bundle,
@@ -953,6 +956,14 @@ _uverbs_get_const_unsigned(u64 *to,
 {
 	return -EINVAL;
 }
+
+static inline int _ib_copy_validate_udata_in(struct ib_udata *udata, void *req,
+					     size_t kernel_size,
+					     size_t minimum_size)
+{
+	return -EVINAL;
+}
+
 #endif
 
 #define uverbs_get_const_signed(_to, _attrs_bundle, _idx)                      \
@@ -1018,4 +1029,19 @@ uverbs_get_raw_fd(int *to, const struct uverbs_attr_bundle *attrs_bundle,
 	return uverbs_get_const_signed(to, attrs_bundle, idx);
 }
 
+/**
+ * ib_copy_validate_udata_in - Copy and validate that the request structure is
+ *                             compatible with this kernel
+ * @_udata: The system calls ib_udata struct
+ * @_req: The name of an on-stack structure that holds the driver data
+ * @_end_member: The member in the struct that is the original end of struct
+ *               from the first kernel to introduce it.
+ *
+ * Check that the udata input request struct is properly formed for this kernel.
+ * Then copy it into req
+ */
+#define ib_copy_validate_udata_in(_udata, _req, _end_member)      \
+	_ib_copy_validate_udata_in(_udata, &(_req), sizeof(_req), \
+				   offsetofend(typeof(_req), _end_member))
+
 #endif
-- 
2.43.0

[PATCH v2 04/13] RDMA: Add ib_copy_validate_udata_in_cm()

[Jason Gunthorpe]

For structures with comp_mask also absorb the check of comp_mask valid
bits into the helper. This is slightly tricky because ~ might not fully
extend to 64 bits, the helper inserts an explicit type to ensure that ~
covers all bits.

Tested-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/infiniband/core/uverbs_ioctl.c | 12 ++++++++++++
 include/rdma/uverbs_ioctl.h            | 25 +++++++++++++++++++++++++
 2 files changed, 37 insertions(+)

diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index 81798c0875ed3a..5e5b00c6236fa8 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -898,3 +898,15 @@ int _ib_copy_validate_udata_in(struct ib_udata *udata, void *req,
 	return 0;
 }
 EXPORT_SYMBOL(_ib_copy_validate_udata_in);
+
+int _ib_copy_validate_udata_cm_fail(struct ib_udata *udata, u64 req_cm,
+				    u64 valid_cm)
+{
+	ibdev_dbg(
+		rdma_udata_to_dev(udata),
+		"System call driver input udata has unsupported comp_mask %llx & ~%llx = %llx for ioctl %ps called by %pSR\n",
+		req_cm, valid_cm, req_cm & ~valid_cm,
+		uverbs_get_handler_fn(udata), __builtin_return_address(0));
+	return -EOPNOTSUPP;
+}
+EXPORT_SYMBOL(_ib_copy_validate_udata_cm_fail);
diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index 5ea53a49b7c6b5..e4719419460422 100644
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -1044,4 +1044,29 @@ uverbs_get_raw_fd(int *to, const struct uverbs_attr_bundle *attrs_bundle,
 	_ib_copy_validate_udata_in(_udata, &(_req), sizeof(_req), \
 				   offsetofend(typeof(_req), _end_member))
 
+int _ib_copy_validate_udata_cm_fail(struct ib_udata *udata, u64 req_cm,
+				    u64 valid_cm);
+
+/**
+ * ib_copy_validate_udata_in_cm - Copy the req structure and check the comp_mask
+ * @_udata: The system calls ib_udata struct
+ * @_req: The name of an on-stack structure that holds the driver data
+ * @_end_member: The member in the struct that is the original end of struct
+ *               from the first kernel to introduce it.
+ * @_valid_cm: A bitmask of bits permitted in the comp_mask_field.
+ *
+ * Check that the udata input request struct is properly formed for this kernel.
+ * Then copy it into req
+ */
+#define ib_copy_validate_udata_in_cm(_udata, _req, _end_member, _valid_cm)    \
+	({                                                                    \
+		typeof((_req).comp_mask) __valid_cm = _valid_cm;              \
+		int ret =                                                     \
+			ib_copy_validate_udata_in(_udata, _req, _end_member); \
+		if (!ret && ((_req).comp_mask & ~__valid_cm))                 \
+			ret = _ib_copy_validate_udata_cm_fail(                \
+				_udata, (_req).comp_mask, __valid_cm);        \
+		ret;                                                          \
+	})
+
 #endif
-- 
2.43.0

[PATCH v2 05/13] RDMA: Add ib_respond_udata()

[Jason Gunthorpe]

Wrap the common copy_to_user() pattern used in drivers and enhance it
to zero pad as well. Include debug logging on failures.

Tested-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/infiniband/core/uverbs_ioctl.c | 24 +++++++++++++++++++
 include/rdma/uverbs_ioctl.h            | 33 ++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/drivers/infiniband/core/uverbs_ioctl.c b/drivers/infiniband/core/uverbs_ioctl.c
index 5e5b00c6236fa8..b61af625e679b2 100644
--- a/drivers/infiniband/core/uverbs_ioctl.c
+++ b/drivers/infiniband/core/uverbs_ioctl.c
@@ -910,3 +910,27 @@ int _ib_copy_validate_udata_cm_fail(struct ib_udata *udata, u64 req_cm,
 	return -EOPNOTSUPP;
 }
 EXPORT_SYMBOL(_ib_copy_validate_udata_cm_fail);
+
+int _ib_respond_udata(struct ib_udata *udata, const void *src, size_t len)
+{
+	size_t copy_len;
+
+	/* 0 length copy_len is a NOP for copy_to_user() and doesn't fail. */
+	copy_len = min(len, udata->outlen);
+	if (copy_to_user(udata->outbuf, src, copy_len))
+		goto err_fault;
+	if (copy_len < udata->outlen) {
+		if (clear_user(udata->outbuf + copy_len,
+			       udata->outlen - copy_len))
+			goto err_fault;
+	}
+	return 0;
+err_fault:
+	ibdev_dbg(
+		rdma_udata_to_dev(udata),
+		"System call driver out udata has EFAULT (%zu into %zu) for ioctl %ps called by %pSR\n",
+		len, udata->outlen, uverbs_get_handler_fn(udata),
+		__builtin_return_address(0));
+	return -EFAULT;
+}
+EXPORT_SYMBOL(_ib_respond_udata);
diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index e4719419460422..720f173c2b3fc7 100644
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -900,6 +900,7 @@ int uverbs_copy_to_struct_or_zero(const struct uverbs_attr_bundle *bundle,
 
 int _ib_copy_validate_udata_in(struct ib_udata *udata, void *req,
 			       size_t kernel_size, size_t minimum_size);
+int _ib_respond_udata(struct ib_udata *udata, const void *src, size_t len);
 #else
 static inline int
 uverbs_get_flags64(u64 *to, const struct uverbs_attr_bundle *attrs_bundle,
@@ -964,6 +965,11 @@ static inline int _ib_copy_validate_udata_in(struct ib_udata *udata, void *req,
 	return -EVINAL;
 }
 
+static inline int _ib_respond_udata(struct ib_udata *udata, const void *src,
+				    size_t len)
+{
+	return -EINVAL;
+}
 #endif
 
 #define uverbs_get_const_signed(_to, _attrs_bundle, _idx)                      \
@@ -1069,4 +1075,31 @@ int _ib_copy_validate_udata_cm_fail(struct ib_udata *udata, u64 req_cm,
 		ret;                                                          \
 	})
 
+/**
+ * ib_respond_udata - Copy a driver data response to userspace
+ * @_udata: The system calls ib_udata struct
+ * @_rep: Kernel buffer containing the response driver data on the stack
+ *
+ * Copy driver data response structures back to userspace in a way that
+ * is forwards and backwards compatible. Longer kernel structs are truncated,
+ * userspace has made some kind of error if it needed the truncated information.
+ * Shorter structs are zero padded.
+ */
+#define ib_respond_udata(_udata, _rep) \
+	_ib_respond_udata(_udata, &(_rep), sizeof(_rep))
+
+/**
+ * ib_respond_empty_udata - Zero fill the response buffer to userspace
+ * @_udata: The system calls ib_udata struct
+ *
+ * Used when there is no driver response data to return. Provides forward
+ * compatability by zeroing any buffer the user may have provided.
+ */
+static inline int ib_respond_empty_udata(struct ib_udata *udata)
+{
+	if (udata && udata->outlen && clear_user(udata->outbuf, udata->outlen))
+		return -EFAULT;
+	return 0;
+}
+
 #endif
-- 
2.43.0

[PATCH v2 06/13] RDMA: Add ib_is_udata_in_empty()

[Jason Gunthorpe]

If the driver doesn't yet support any request driver data it should check
that it is all zeroed. This is a common pattern, add a helper around
_ib_copy_validate_udata_in() to do this.

Tested-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 include/rdma/uverbs_ioctl.h | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h
index 720f173c2b3fc7..cf395fa8c36174 100644
--- a/include/rdma/uverbs_ioctl.h
+++ b/include/rdma/uverbs_ioctl.h
@@ -1075,6 +1075,21 @@ int _ib_copy_validate_udata_cm_fail(struct ib_udata *udata, u64 req_cm,
 		ret;                                                          \
 	})
 
+/**
+ * ib_is_udata_in_empty - Check if the udata input buffer is all zeros
+ * @udata: The system calls ib_udata struct
+ *
+ * This should be used if the driver does not currently define a driver data
+ * struct. Returns 0 if the buffer is empty or all zeros, -EOPNOTSUPP if
+ * non-zero data is present, or a negative error code on failure.
+ */
+static inline int ib_is_udata_in_empty(struct ib_udata *udata)
+{
+	if (!udata || udata->inlen == 0)
+		return 0;
+	return _ib_copy_validate_udata_in(udata, NULL, 0, 0);
+}
+
 /**
  * ib_respond_udata - Copy a driver data response to userspace
  * @_udata: The system calls ib_udata struct
-- 
2.43.0

[PATCH v2 07/13] RDMA: Provide documentation about the uABI compatibility rules

[Jason Gunthorpe]

Write down how all of this is supposed to work using the new helpers.

Tested-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 include/rdma/ib_verbs.h | 87 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 3f3827e1c711d8..e1ba20c3974f08 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -1576,6 +1576,93 @@ struct ib_uobject {
 	const struct uverbs_api_object *uapi_object;
 };
 
+/**
+ * struct ib_udata - Driver request/response data from userspace
+ * @inbuf: Pointer to request data from userspace
+ * @outbuf: Pointer to response buffer in userspace
+ * @inlen: Length of request data
+ * @outlen: Length of response buffer
+ *
+ * struct ib_udata is used to hold the driver data request and response
+ * structures defined in the uapi. They follow these rules for forwards and
+ * backwards compatibility:
+ *
+ * 1) Userspace can provide a longer request so long as the trailing part the
+ *    kernel doesn't understand is all zeros.
+ *
+ *    This provides a degree of safety if userspace wrongly tries to use a new
+ *    feature the kernel does not understand with some non-zero value.
+ *
+ *    It allows a simpler rdma-core implementation because the library can
+ *    simply always use the latest structs for the request, even if they are
+ *    bigger. It simply has to avoid using the new members if they are not
+ *    supported/required.
+ *
+ * 2) Userspace can provide a shorter request; the kernel will zero-pad it out
+ *    to fill the storage. The newer kernel should understand that older
+ *    userspace will provide 0 to new fields. The kernel has three options to
+ *    enable new request fields:
+ *
+ *    - Input comp_mask that says the field is supported
+ *    - Look for non-zero values
+ *    - Check if the udata->inlen size covers the field
+ *
+ *    This also corrects any bugs related to not filling in request structures
+ *    as the new helper always fully writes to the struct.
+ *
+ * 3) Userspace can provide a shorter or longer response struct. If shorter,
+ *    the kernel reply is truncated. The kernel should be designed to not write
+ *    to new reply fields unless userspace has affirmatively requested them.
+ *
+ *    If the user buffer is longer, the kernel will zero-fill it.
+ *
+ *    Userspace has three options to enable new response fields:
+ *
+ *    - Output comp_mask that says the field is supported
+ *    - Look for non-zero values
+ *    - Infer the output must be valid because the request contents demand it
+ *      and old kernels will fail the request
+ *
+ * The following helper functions implement these semantics:
+ *
+ * ib_copy_validate_udata_in() - Checks the minimum length, and zero trailing::
+ *
+ *     struct driver_create_cq_req req;
+ *     int err;
+ *
+ *     err = ib_copy_validate_udata_in(udata, req, end_member);
+ *     if (err)
+ *         return err;
+ *
+ * The third argument specifies the last member of the struct in the first
+ * kernel version that introduced it, establishing the minimum required size.
+ *
+ * ib_copy_validate_udata_in_cm() - The above but also validate a
+ * comp_mask member only has supported bits set::
+ *
+ *     err = ib_copy_validate_udata_in_cm(udata, req, first_version_last_member,
+ *                                        DRIVER_CREATE_CQ_MASK_FEATURE_A |
+ *                                        DRIVER_CREATE_CQ_MASK_FEATURE_B);
+ *
+ * ib_respond_udata() - Implements the response rules::
+ *
+ *     struct driver_create_cq_resp resp = {};
+ *
+ *     resp.some_field = value;
+ *     return ib_respond_udata(udata, resp);
+ *
+ * ib_is_udata_in_empty() - Used instead of ib_copy_validate_udata_in() if the
+ * driver does not have a request structure::
+ *
+ *     ret = ib_is_udata_in_empty(udata);
+ *     if (ret)
+ *         return ret;
+ *
+ * Similarly ib_respond_empty_udata() is used instead of ib_respond_udata() if
+ * the driver does not have a response structure::
+ *
+ *    return ib_respond_empty_udata(udata);
+ */
 struct ib_udata {
 	const void __user *inbuf;
 	void __user *outbuf;
-- 
2.43.0

[PATCH v2 08/13] RDMA/bnxt_re: Add compatibility checks to the uapi path

[Jason Gunthorpe]

Check that the driver data is properly sized and properly zeroed by
calling ib_copy_validate_udata_in().

Use git history to find the commit introducing each req struct and use
that to select the end member.

Tested-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 29 +++++++++++++-----------
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 5c5ecfacf50612..e1d72ae8261192 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -1671,9 +1671,11 @@ int bnxt_re_create_qp(struct ib_qp *ib_qp, struct ib_qp_init_attr *qp_init_attr,
 	qp = container_of(ib_qp, struct bnxt_re_qp, ib_qp);
 
 	uctx = rdma_udata_to_drv_context(udata, struct bnxt_re_ucontext, ib_uctx);
-	if (udata)
-		if (ib_copy_from_udata(&ureq, udata,  min(udata->inlen, sizeof(ureq))))
-			return -EFAULT;
+	if (udata) {
+		rc = ib_copy_validate_udata_in(udata, ureq, qp_handle);
+		if (rc)
+			return rc;
+	}
 
 	rc = bnxt_re_test_qp_limits(rdev, qp_init_attr, dev_attr);
 	if (!rc) {
@@ -1863,9 +1865,11 @@ static int bnxt_re_init_user_srq(struct bnxt_re_dev *rdev,
 	int bytes = 0;
 	struct bnxt_re_ucontext *cntx = rdma_udata_to_drv_context(
 		udata, struct bnxt_re_ucontext, ib_uctx);
+	int rc;
 
-	if (ib_copy_from_udata(&ureq, udata, sizeof(ureq)))
-		return -EFAULT;
+	rc = ib_copy_validate_udata_in(udata, ureq, srq_handle);
+	if (rc)
+		return rc;
 
 	bytes = (qplib_srq->max_wqe * qplib_srq->wqe_size);
 	bytes = PAGE_ALIGN(bytes);
@@ -3177,10 +3181,10 @@ int bnxt_re_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 	cq->qplib_cq.sg_info.pgshft = PAGE_SHIFT;
 	if (udata) {
 		struct bnxt_re_cq_req req;
-		if (ib_copy_from_udata(&req, udata, sizeof(req))) {
-			rc = -EFAULT;
+
+		rc = ib_copy_validate_udata_in(udata, req, cq_handle);
+		if (rc)
 			goto fail;
-		}
 
 		cq->umem = ib_umem_get(&rdev->ibdev, req.cq_va,
 				       entries * sizeof(struct cq_base),
@@ -3309,10 +3313,9 @@ int bnxt_re_resize_cq(struct ib_cq *ibcq, int cqe, struct ib_udata *udata)
 		entries = dev_attr->max_cq_wqes + 1;
 
 	/* uverbs consumer */
-	if (ib_copy_from_udata(&req, udata, sizeof(req))) {
-		rc = -EFAULT;
+	rc = ib_copy_validate_udata_in(udata, req, cq_va);
+	if (rc)
 		goto fail;
-	}
 
 	cq->resize_umem = ib_umem_get(&rdev->ibdev, req.cq_va,
 				      entries * sizeof(struct cq_base),
@@ -4414,8 +4417,8 @@ int bnxt_re_alloc_ucontext(struct ib_ucontext *ctx, struct ib_udata *udata)
 	if (_is_modify_qp_rate_limit_supported(dev_attr->dev_cap_flags2))
 		resp.comp_mask |= BNXT_RE_UCNTX_CMASK_QP_RATE_LIMIT_ENABLED;
 
-	if (udata->inlen >= sizeof(ureq)) {
-		rc = ib_copy_from_udata(&ureq, udata, min(udata->inlen, sizeof(ureq)));
+	if (udata->inlen) {
+		rc = ib_copy_validate_udata_in(udata, ureq, comp_mask);
 		if (rc)
 			goto cfail;
 		if (ureq.comp_mask & BNXT_RE_COMP_MASK_REQ_UCNTX_POW2_SUPPORT) {
-- 
2.43.0

[PATCH v2 09/13] RDMA/bnxt_re: Add compatibility checks to the uapi path for no data

[Jason Gunthorpe]

If drivers ever want to go from an empty drvdata to something with them
they need to have called ib_is_udata_in_empty(). Add the missing calls to
all the system calls that don't have req structures.

Tested-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 57 ++++++++++++++++++++++++
 1 file changed, 57 insertions(+)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index e1d72ae8261192..6d751febb28c8b 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -190,6 +190,10 @@ int bnxt_re_query_device(struct ib_device *ibdev,
 	size_t outlen = (udata) ? udata->outlen : 0;
 	int rc = 0;
 
+	rc = ib_is_udata_in_empty(udata);
+	if (rc)
+		return rc;
+
 	memset(ib_attr, 0, sizeof(*ib_attr));
 	memcpy(&ib_attr->fw_ver, dev_attr->fw_ver,
 	       min(sizeof(dev_attr->fw_ver),
@@ -692,6 +696,11 @@ int bnxt_re_dealloc_pd(struct ib_pd *ib_pd, struct ib_udata *udata)
 {
 	struct bnxt_re_pd *pd = container_of(ib_pd, struct bnxt_re_pd, ib_pd);
 	struct bnxt_re_dev *rdev = pd->rdev;
+	int ret;
+
+	ret = ib_is_udata_in_empty(udata);
+	if (ret)
+		return ret;
 
 	if (udata) {
 		rdma_user_mmap_entry_remove(pd->pd_db_mmap);
@@ -720,6 +729,10 @@ int bnxt_re_alloc_pd(struct ib_pd *ibpd, struct ib_udata *udata)
 	u32 active_pds;
 	int rc = 0;
 
+	rc = ib_is_udata_in_empty(udata);
+	if (rc)
+		return rc;
+
 	pd->rdev = rdev;
 	if (bnxt_qplib_alloc_pd(&rdev->qplib_res, &pd->qplib_pd)) {
 		ibdev_err(&rdev->ibdev, "Failed to allocate HW PD");
@@ -834,6 +847,10 @@ int bnxt_re_create_ah(struct ib_ah *ib_ah, struct rdma_ah_init_attr *init_attr,
 	u8 nw_type;
 	int rc;
 
+	rc = ib_is_udata_in_empty(udata);
+	if (rc)
+		return rc;
+
 	if (!(rdma_ah_get_ah_flags(ah_attr) & IB_AH_GRH)) {
 		ibdev_err(&rdev->ibdev, "Failed to alloc AH: GRH not set");
 		return -EINVAL;
@@ -995,6 +1012,10 @@ int bnxt_re_destroy_qp(struct ib_qp *ib_qp, struct ib_udata *udata)
 	unsigned int flags;
 	int rc;
 
+	rc = ib_is_udata_in_empty(udata);
+	if (rc)
+		return rc;
+
 	bnxt_re_debug_rem_qpinfo(rdev, qp);
 
 	bnxt_qplib_flush_cqn_wq(&qp->qplib_qp);
@@ -1843,6 +1864,11 @@ int bnxt_re_destroy_srq(struct ib_srq *ib_srq, struct ib_udata *udata)
 					       ib_srq);
 	struct bnxt_re_dev *rdev = srq->rdev;
 	struct bnxt_qplib_srq *qplib_srq = &srq->qplib_srq;
+	int ret;
+
+	ret = ib_is_udata_in_empty(udata);
+	if (ret)
+		return ret;
 
 	if (rdev->chip_ctx->modes.toggle_bits & BNXT_QPLIB_SRQ_TOGGLE_BIT) {
 		free_page((unsigned long)srq->uctx_srq_page);
@@ -1992,6 +2018,11 @@ int bnxt_re_modify_srq(struct ib_srq *ib_srq, struct ib_srq_attr *srq_attr,
 	struct bnxt_re_srq *srq = container_of(ib_srq, struct bnxt_re_srq,
 					       ib_srq);
 	struct bnxt_re_dev *rdev = srq->rdev;
+	int ret;
+
+	ret = ib_is_udata_in_empty(udata);
+	if (ret)
+		return ret;
 
 	switch (srq_attr_mask) {
 	case IB_SRQ_MAX_WR:
@@ -2109,6 +2140,10 @@ int bnxt_re_modify_qp(struct ib_qp *ib_qp, struct ib_qp_attr *qp_attr,
 	unsigned int flags;
 	u8 nw_type;
 
+	rc = ib_is_udata_in_empty(udata);
+	if (rc)
+		return rc;
+
 	if (qp_attr_mask & ~(IB_QP_ATTR_STANDARD_BITS | IB_QP_RATE_LIMIT))
 		return -EOPNOTSUPP;
 
@@ -3126,12 +3161,17 @@ int bnxt_re_destroy_cq(struct ib_cq *ib_cq, struct ib_udata *udata)
 	struct bnxt_qplib_nq *nq;
 	struct bnxt_re_dev *rdev;
 	struct bnxt_re_cq *cq;
+	int ret;
 
 	cq = container_of(ib_cq, struct bnxt_re_cq, ib_cq);
 	rdev = cq->rdev;
 	nq = cq->qplib_cq.nq;
 	cctx = rdev->chip_ctx;
 
+	ret = ib_is_udata_in_empty(udata);
+	if (ret)
+		return ret;
+
 	if (cctx->modes.toggle_bits & BNXT_QPLIB_CQ_TOGGLE_BIT) {
 		free_page((unsigned long)cq->uctx_cq_page);
 		hash_del(&cq->hash_entry);
@@ -4078,6 +4118,10 @@ int bnxt_re_dereg_mr(struct ib_mr *ib_mr, struct ib_udata *udata)
 	struct bnxt_re_dev *rdev = mr->rdev;
 	int rc;
 
+	rc = ib_is_udata_in_empty(udata);
+	if (rc)
+		return rc;
+
 	rc = bnxt_qplib_free_mrw(&rdev->qplib_res, &mr->qplib_mr);
 	if (rc) {
 		ibdev_err(&rdev->ibdev, "Dereg MR failed: %#x\n", rc);
@@ -4186,6 +4230,10 @@ struct ib_mw *bnxt_re_alloc_mw(struct ib_pd *ib_pd, enum ib_mw_type type,
 	u32 active_mws;
 	int rc;
 
+	rc = ib_is_udata_in_empty(udata);
+	if (rc)
+		return ERR_PTR(rc);
+
 	mw = kzalloc_obj(*mw);
 	if (!mw)
 		return ERR_PTR(-ENOMEM);
@@ -4313,6 +4361,11 @@ struct ib_mr *bnxt_re_reg_user_mr(struct ib_pd *ib_pd, u64 start, u64 length,
 	struct bnxt_re_dev *rdev = pd->rdev;
 	struct ib_umem *umem;
 	struct ib_mr *ib_mr;
+	int ret;
+
+	ret = ib_is_udata_in_empty(udata);
+	if (ret)
+		return ERR_PTR(ret);
 
 	if (dmah)
 		return ERR_PTR(-EOPNOTSUPP);
@@ -4497,6 +4550,10 @@ struct ib_flow *bnxt_re_create_flow(struct ib_qp *ib_qp,
 	struct bnxt_re_flow *flow;
 	int rc;
 
+	rc = ib_is_udata_in_empty(udata);
+	if (rc)
+		return ERR_PTR(rc);
+
 	if (attr->type != IB_FLOW_ATTR_SNIFFER ||
 	    !rdev->rcfw.roce_mirror)
 		return ERR_PTR(-EOPNOTSUPP);
-- 
2.43.0

[PATCH v2 10/13] RDMA/bnxt_re: Add missing comp_mask validation

[Jason Gunthorpe]

Two existing req driver data structures have comp_mask but nothing
checks them for valid contents. Add the missing checks.

Tested-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 6d751febb28c8b..412b99658d9073 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -1693,7 +1693,7 @@ int bnxt_re_create_qp(struct ib_qp *ib_qp, struct ib_qp_init_attr *qp_init_attr,
 
 	uctx = rdma_udata_to_drv_context(udata, struct bnxt_re_ucontext, ib_uctx);
 	if (udata) {
-		rc = ib_copy_validate_udata_in(udata, ureq, qp_handle);
+		rc = ib_copy_validate_udata_in_cm(udata, ureq, qp_handle, 0);
 		if (rc)
 			return rc;
 	}
@@ -4471,7 +4471,10 @@ int bnxt_re_alloc_ucontext(struct ib_ucontext *ctx, struct ib_udata *udata)
 		resp.comp_mask |= BNXT_RE_UCNTX_CMASK_QP_RATE_LIMIT_ENABLED;
 
 	if (udata->inlen) {
-		rc = ib_copy_validate_udata_in(udata, ureq, comp_mask);
+		rc = ib_copy_validate_udata_in_cm(
+			udata, ureq, comp_mask,
+			BNXT_RE_COMP_MASK_REQ_UCNTX_POW2_SUPPORT |
+				BNXT_RE_COMP_MASK_REQ_UCNTX_VAR_WQE_SUPPORT);
 		if (rc)
 			goto cfail;
 		if (ureq.comp_mask & BNXT_RE_COMP_MASK_REQ_UCNTX_POW2_SUPPORT) {
-- 
2.43.0

[PATCH v2 11/13] RDMA/bnxt_re: Use ib_respond_udata()

[Jason Gunthorpe]

All the calls to ib_copy_to_udata() can use this helper safely.

Tested-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 31 +++++++-----------------
 1 file changed, 9 insertions(+), 22 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 412b99658d9073..663f452946c782 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -187,7 +187,6 @@ int bnxt_re_query_device(struct ib_device *ibdev,
 	struct bnxt_re_dev *rdev = to_bnxt_re_dev(ibdev, ibdev);
 	struct bnxt_qplib_dev_attr *dev_attr = rdev->dev_attr;
 	struct bnxt_re_query_device_ex_resp resp = {};
-	size_t outlen = (udata) ? udata->outlen : 0;
 	int rc = 0;
 
 	rc = ib_is_udata_in_empty(udata);
@@ -258,8 +257,7 @@ int bnxt_re_query_device(struct ib_device *ibdev,
 	ib_attr->max_pkeys = 1;
 	ib_attr->local_ca_ack_delay = BNXT_RE_DEFAULT_ACK_DELAY;
 
-	if ((offsetofend(typeof(resp), packet_pacing_caps) <= outlen) &&
-	    _is_modify_qp_rate_limit_supported(dev_attr->dev_cap_flags2)) {
+	if (_is_modify_qp_rate_limit_supported(dev_attr->dev_cap_flags2)) {
 		resp.packet_pacing_caps.qp_rate_limit_min =
 			dev_attr->rate_limit_min;
 		resp.packet_pacing_caps.qp_rate_limit_max =
@@ -267,11 +265,7 @@ int bnxt_re_query_device(struct ib_device *ibdev,
 		resp.packet_pacing_caps.supported_qpts =
 			1 << IB_QPT_RC;
 	}
-	if (outlen)
-		rc = ib_copy_to_udata(udata, &resp,
-				      min(sizeof(resp), outlen));
-
-	return rc;
+	return ib_respond_udata(udata, resp);
 }
 
 int bnxt_re_modify_device(struct ib_device *ibdev,
@@ -769,7 +763,7 @@ int bnxt_re_alloc_pd(struct ib_pd *ibpd, struct ib_udata *udata)
 
 		pd->pd_db_mmap = &entry->rdma_entry;
 
-		rc = ib_copy_to_udata(udata, &resp, min(sizeof(resp), udata->outlen));
+		rc = ib_respond_udata(udata, resp);
 		if (rc) {
 			rdma_user_mmap_entry_remove(pd->pd_db_mmap);
 			rc = -EFAULT;
@@ -1727,11 +1721,9 @@ int bnxt_re_create_qp(struct ib_qp *ib_qp, struct ib_qp_init_attr *qp_init_attr,
 
 			resp.qpid = qp->qplib_qp.id;
 			resp.rsvd = 0;
-			rc = ib_copy_to_udata(udata, &resp, sizeof(resp));
-			if (rc) {
-				ibdev_err(&rdev->ibdev, "Failed to copy QP udata");
+			rc = ib_respond_udata(udata, resp);
+			if (rc)
 				goto qp_destroy;
-			}
 		}
 	}
 
@@ -1990,9 +1982,8 @@ int bnxt_re_create_srq(struct ib_srq *ib_srq,
 			}
 			resp.comp_mask |= BNXT_RE_SRQ_TOGGLE_PAGE_SUPPORT;
 		}
-		rc = ib_copy_to_udata(udata, &resp, sizeof(resp));
+		rc = ib_respond_udata(udata, resp);
 		if (rc) {
-			ibdev_err(&rdev->ibdev, "SRQ copy to udata failed!");
 			bnxt_qplib_destroy_srq(&rdev->qplib_res,
 					       &srq->qplib_srq);
 			goto fail;
@@ -3281,9 +3272,8 @@ int bnxt_re_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
 		resp.tail = cq->qplib_cq.hwq.cons;
 		resp.phase = cq->qplib_cq.period;
 		resp.rsvd = 0;
-		rc = ib_copy_to_udata(udata, &resp, min(sizeof(resp), udata->outlen));
+		rc = ib_respond_udata(udata, resp);
 		if (rc) {
-			ibdev_err(&rdev->ibdev, "Failed to copy CQ udata");
 			bnxt_qplib_destroy_cq(&rdev->qplib_res, &cq->qplib_cq);
 			goto free_mem;
 		}
@@ -4489,12 +4479,9 @@ int bnxt_re_alloc_ucontext(struct ib_ucontext *ctx, struct ib_udata *udata)
 		}
 	}
 
-	rc = ib_copy_to_udata(udata, &resp, min(udata->outlen, sizeof(resp)));
-	if (rc) {
-		ibdev_err(ibdev, "Failed to copy user context");
-		rc = -EFAULT;
+	rc = ib_respond_udata(udata, resp);
+	if (rc)
 		goto cfail;
-	}
 
 	return 0;
 cfail:
-- 
2.43.0

[PATCH v2 12/13] RDMA/bnxt_re: Use ib_respond_empty_udata()

[Jason Gunthorpe]

Like ib_is_udata_in_empty() for the request side ib_respond_empty_udata()
is called on the response side if there no response struct.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 25 ++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 663f452946c782..62286a06db8168 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -709,7 +709,7 @@ int bnxt_re_dealloc_pd(struct ib_pd *ib_pd, struct ib_udata *udata)
 					   &pd->qplib_pd))
 			atomic_dec(&rdev->stats.res.pd_count);
 	}
-	return 0;
+	return ib_respond_empty_udata(udata);
 }
 
 int bnxt_re_alloc_pd(struct ib_pd *ibpd, struct ib_udata *udata)
@@ -898,7 +898,7 @@ int bnxt_re_create_ah(struct ib_ah *ib_ah, struct rdma_ah_init_attr *init_attr,
 	if (active_ahs > rdev->stats.res.ah_watermark)
 		rdev->stats.res.ah_watermark = active_ahs;
 
-	return 0;
+	return ib_respond_empty_udata(udata);
 }
 
 int bnxt_re_query_ah(struct ib_ah *ib_ah, struct rdma_ah_attr *ah_attr)
@@ -1053,7 +1053,7 @@ int bnxt_re_destroy_qp(struct ib_qp *ib_qp, struct ib_udata *udata)
 	if (scq_nq != rcq_nq)
 		bnxt_re_synchronize_nq(rcq_nq);
 
-	return 0;
+	return ib_respond_empty_udata(udata);
 }
 
 static u8 __from_ib_qp_type(enum ib_qp_type type)
@@ -1869,7 +1869,7 @@ int bnxt_re_destroy_srq(struct ib_srq *ib_srq, struct ib_udata *udata)
 	bnxt_qplib_destroy_srq(&rdev->qplib_res, qplib_srq);
 	ib_umem_release(srq->umem);
 	atomic_dec(&rdev->stats.res.srq_count);
-	return 0;
+	return ib_respond_empty_udata(udata);
 }
 
 static int bnxt_re_init_user_srq(struct bnxt_re_dev *rdev,
@@ -2030,7 +2030,7 @@ int bnxt_re_modify_srq(struct ib_srq *ib_srq, struct ib_srq_attr *srq_attr,
 		/* On success, update the shadow */
 		srq->srq_limit = srq_attr->srq_limit;
 		/* No need to Build and send response back to udata */
-		return 0;
+		return ib_respond_empty_udata(udata);
 	default:
 		ibdev_err(&rdev->ibdev,
 			  "Unsupported srq_attr_mask 0x%x", srq_attr_mask);
@@ -2375,9 +2375,12 @@ int bnxt_re_modify_qp(struct ib_qp *ib_qp, struct ib_qp_attr *qp_attr,
 		ibdev_err(&rdev->ibdev, "Failed to modify HW QP");
 		return rc;
 	}
-	if (ib_qp->qp_type == IB_QPT_GSI && rdev->gsi_ctx.gsi_sqp)
+	if (ib_qp->qp_type == IB_QPT_GSI && rdev->gsi_ctx.gsi_sqp) {
 		rc = bnxt_re_modify_shadow_qp(rdev, qp, qp_attr_mask);
-	return rc;
+		if (rc)
+			return rc;
+	}
+	return ib_respond_empty_udata(udata);
 }
 
 int bnxt_re_query_qp(struct ib_qp *ib_qp, struct ib_qp_attr *qp_attr,
@@ -3174,7 +3177,7 @@ int bnxt_re_destroy_cq(struct ib_cq *ib_cq, struct ib_udata *udata)
 
 	atomic_dec(&rdev->stats.res.cq_count);
 	kfree(cq->cql);
-	return 0;
+	return ib_respond_empty_udata(udata);
 }
 
 int bnxt_re_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr,
@@ -3376,7 +3379,7 @@ int bnxt_re_resize_cq(struct ib_cq *ibcq, int cqe, struct ib_udata *udata)
 	cq->ib_cq.cqe = cq->resize_cqe;
 	atomic_inc(&rdev->stats.res.resize_count);
 
-	return 0;
+	return ib_respond_empty_udata(udata);
 
 fail:
 	if (cq->resize_umem) {
@@ -4129,7 +4132,9 @@ int bnxt_re_dereg_mr(struct ib_mr *ib_mr, struct ib_udata *udata)
 
 	kfree(mr);
 	atomic_dec(&rdev->stats.res.mr_count);
-	return rc;
+	if (rc)
+		return rc;
+	return ib_respond_empty_udata(udata);
 }
 
 static int bnxt_re_set_page(struct ib_mr *ib_mr, u64 addr)
-- 
2.43.0

[PATCH v2 13/13] RDMA/bnxt_re: Add BNXT_RE_UCNTX_CMASK_UAPI_COMPAT_SUPPORTED

[Jason Gunthorpe]

Now that the driver properly implements the uAPI forwards and backwards
compatibility checks tell userspace about it.

If this flag is not set the userspace may not use any of the uAPI newer
than this commit as it will behave in unexpected ways on older kernels
that lack validation.

Userspace should test this flag before invoking any future enhanced calls
relying on changes to the driver data.

Tested-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Acked-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/infiniband/hw/bnxt_re/ib_verbs.c | 3 ++-
 include/uapi/rdma/bnxt_re-abi.h          | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
index 62286a06db8168..8582c8a6030317 100644
--- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
+++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
@@ -4433,7 +4433,8 @@ int bnxt_re_alloc_ucontext(struct ib_ucontext *ctx, struct ib_udata *udata)
 	}
 	spin_lock_init(&uctx->sh_lock);
 
-	resp.comp_mask = BNXT_RE_UCNTX_CMASK_HAVE_CCTX;
+	resp.comp_mask = BNXT_RE_UCNTX_CMASK_HAVE_CCTX |
+			 BNXT_RE_UCNTX_CMASK_UAPI_COMPAT_SUPPORTED;
 	chip_met_rev_num = rdev->chip_ctx->chip_num;
 	chip_met_rev_num |= ((u32)rdev->chip_ctx->chip_rev & 0xFF) <<
 			     BNXT_RE_CHIP_ID0_CHIP_REV_SFT;
diff --git a/include/uapi/rdma/bnxt_re-abi.h b/include/uapi/rdma/bnxt_re-abi.h
index f24edf1c75eb36..66d7edc88f8887 100644
--- a/include/uapi/rdma/bnxt_re-abi.h
+++ b/include/uapi/rdma/bnxt_re-abi.h
@@ -57,6 +57,7 @@ enum {
 	BNXT_RE_UCNTX_CMASK_POW2_DISABLED = 0x10ULL,
 	BNXT_RE_UCNTX_CMASK_MSN_TABLE_ENABLED = 0x40,
 	BNXT_RE_UCNTX_CMASK_QP_RATE_LIMIT_ENABLED = 0x80ULL,
+	BNXT_RE_UCNTX_CMASK_UAPI_COMPAT_SUPPORTED = 0x100ULL,
 };
 
 enum bnxt_re_wqe_mode {
-- 
2.43.0

Re: [PATCH v2 03/13] RDMA: Add ib_copy_validate_udata_in()

[kernel test robot]

Hi Jason,

kernel test robot noticed the following build errors:

[auto build test ERROR on 3f4a08e64442340f4807de63e30aef22cc308830]

url:    https://github.com/intel-lab-lkp/linux/commits/Jason-Gunthorpe/RDMA-Use-copy_struct_from_user-instead-of-open-coding/20260227-093947
base:   3f4a08e64442340f4807de63e30aef22cc308830
patch link:    https://lore.kernel.org/r/3-v2-13af4a900857%2B4f13-bnxt_re_uapi_jgg%40nvidia.com
patch subject: [PATCH v2 03/13] RDMA: Add ib_copy_validate_udata_in()
config: loongarch-defconfig (https://download.01.org/0day-ci/archive/20260227/202602271548.rksoOQrN-lkp@intel.com/config)
compiler: clang version 19.1.7 (https://github.com/llvm/llvm-project cd708029e0b2869e80abe31ddb175f7c35361f90)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260227/202602271548.rksoOQrN-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602271548.rksoOQrN-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/infiniband/core/nldev.c:44:
   In file included from drivers/infiniband/core/uverbs.h:49:
   In file included from include/rdma/uverbs_std_types.h:10:
>> include/rdma/uverbs_ioctl.h:964:10: error: use of undeclared identifier 'EVINAL'
     964 |         return -EVINAL;
         |                 ^
   1 error generated.


vim +/EVINAL +964 include/rdma/uverbs_ioctl.h

   959	
   960	static inline int _ib_copy_validate_udata_in(struct ib_udata *udata, void *req,
   961						     size_t kernel_size,
   962						     size_t minimum_size)
   963	{
 > 964		return -EVINAL;
   965	}
   966	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 03/13] RDMA: Add ib_copy_validate_udata_in()

[kernel test robot]

Hi Jason,

kernel test robot noticed the following build errors:

[auto build test ERROR on 3f4a08e64442340f4807de63e30aef22cc308830]

url:    https://github.com/intel-lab-lkp/linux/commits/Jason-Gunthorpe/RDMA-Use-copy_struct_from_user-instead-of-open-coding/20260227-093947
base:   3f4a08e64442340f4807de63e30aef22cc308830
patch link:    https://lore.kernel.org/r/3-v2-13af4a900857%2B4f13-bnxt_re_uapi_jgg%40nvidia.com
patch subject: [PATCH v2 03/13] RDMA: Add ib_copy_validate_udata_in()
config: i386-randconfig-141-20260227 (https://download.01.org/0day-ci/archive/20260228/202602281856.cOumhTuc-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.4.0-5) 12.4.0
smatch version: v0.5.0-8994-gd50c5a4c
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260228/202602281856.cOumhTuc-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602281856.cOumhTuc-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from include/rdma/uverbs_std_types.h:10,
                    from drivers/infiniband/core/uverbs.h:49,
                    from drivers/infiniband/core/nldev.c:44:
   include/rdma/uverbs_ioctl.h: In function '_ib_copy_validate_udata_in':
>> include/rdma/uverbs_ioctl.h:964:17: error: 'EVINAL' undeclared (first use in this function); did you mean 'EINVAL'?
     964 |         return -EVINAL;
         |                 ^~~~~~
         |                 EINVAL
   include/rdma/uverbs_ioctl.h:964:17: note: each undeclared identifier is reported only once for each function it appears in


vim +964 include/rdma/uverbs_ioctl.h

   959	
   960	static inline int _ib_copy_validate_udata_in(struct ib_udata *udata, void *req,
   961						     size_t kernel_size,
   962						     size_t minimum_size)
   963	{
 > 964		return -EVINAL;
   965	}
   966	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 00/13] Provide udata helpers and use them in bnxt_re

[Leon Romanovsky]

On Thu, Feb 26, 2026 at 09:11:03PM -0400, Jason Gunthorpe wrote:
> Add new helpers that entirely execute the expected common patterns for
> driver data uAPI forward and backwards compatibility so that drivers don't
> have to open code these.

<...>

> Since bnxt_re has never implemented these rules correctly and now does,
> provide a UCTX flag to tell userspace about it. If
> BNXT_RE_UCNTX_CMASK_UAPI_COMPAT_SUPPORTED is not set then userspace must
> not use any request or response fields beyond the current kernel uAPI.
> 
> Using any new fields is only possible on kernels with the flag.

Does a similar flag exist in any other driver? If so, we should consider
making it global to ib_uverbs. If not, keeping it bnxt_re‑specific is
sufficient.

Thanks

> 
> A series converting all drivers to these new helpers is on github, I will
> send it later:
> 
> https://github.com/jgunthorpe/linux/commits/rdma_uapi/

Re: [PATCH v2 02/13] RDMA/core: Add rdma_udata_to_dev()

[Leon Romanovsky]

On Thu, Feb 26, 2026 at 09:11:05PM -0400, Jason Gunthorpe wrote:
> Get an ib_device out of a udata so it can be used for debug prints.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/infiniband/core/ib_core_uverbs.c | 27 ++++++++++++++++++++++++
>  include/rdma/uverbs_ioctl.h              |  2 ++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/drivers/infiniband/core/ib_core_uverbs.c b/drivers/infiniband/core/ib_core_uverbs.c
> index d3836a62a00495..bfe37a9c8a72bf 100644
> --- a/drivers/infiniband/core/ib_core_uverbs.c
> +++ b/drivers/infiniband/core/ib_core_uverbs.c
> @@ -389,3 +389,30 @@ int rdma_user_mmap_entry_insert(struct ib_ucontext *ucontext,
>  						 U32_MAX);
>  }
>  EXPORT_SYMBOL(rdma_user_mmap_entry_insert);
> +
> +/**
> + * rdma_udata_to_dev - Get a ib_device from a udata
> + * @udata: The system calls ib_udata struct
> + *
> + * The struct ib_device that is handling the uverbs call. Must not be called if
> + * udata is NULL. The result can be NULL.
> + */
> +struct ib_device *rdma_udata_to_dev(struct ib_udata *udata)
> +{
> +	struct uverbs_attr_bundle *bundle =
> +		rdma_udata_to_uverbs_attr_bundle(udata);
> +
> +	lockdep_assert_held(&bundle->ufile->device->disassociate_srcu);
> +
> +	if (bundle->context)
> +		return bundle->context->device;
> +
> +	/*
> +	 * If the context hasn't been created yet use the ufile's dev, but it
> +	 * might be NULL if we are racing with disassociate.
> +	 */
> +	return srcu_dereference(bundle->ufile->device->ib_dev,
> +				&bundle->ufile->device->disassociate_srcu);
> +}
> +EXPORT_SYMBOL(rdma_udata_to_dev);

Thanks for this function, looks great, however I wonder about use of it
in debug prints. What about changing:

+       ibdev_dbg(
+               rdma_udata_to_dev(udata),
...

to be something
+       udata_dbg(
+               udata,
....

Together with keeping this rdma_udata_to_dev() function.

Thanks

Re: [PATCH v2 00/13] Provide udata helpers and use them in bnxt_re

[Jason Gunthorpe]

On Mon, Mar 02, 2026 at 09:24:13PM +0200, Leon Romanovsky wrote:
> On Thu, Feb 26, 2026 at 09:11:03PM -0400, Jason Gunthorpe wrote:
> > Add new helpers that entirely execute the expected common patterns for
> > driver data uAPI forward and backwards compatibility so that drivers don't
> > have to open code these.
> 
> <...>
> 
> > Since bnxt_re has never implemented these rules correctly and now does,
> > provide a UCTX flag to tell userspace about it. If
> > BNXT_RE_UCNTX_CMASK_UAPI_COMPAT_SUPPORTED is not set then userspace must
> > not use any request or response fields beyond the current kernel uAPI.
> > 
> > Using any new fields is only possible on kernels with the flag.
> 
> Does a similar flag exist in any other driver? If so, we should consider
> making it global to ib_uverbs. If not, keeping it bnxt_re‑specific is
> sufficient.

I've been thinking about this too..

I think I'm pretty close in other drivers to being able to make the
same statement.

Maybe it should be a global flag in the uctx uAPI

Jason

Re: [PATCH v2 02/13] RDMA/core: Add rdma_udata_to_dev()

[Jason Gunthorpe]

On Mon, Mar 02, 2026 at 09:30:56PM +0200, Leon Romanovsky wrote:

> > +struct ib_device *rdma_udata_to_dev(struct ib_udata *udata)
> > +{
> > +	struct uverbs_attr_bundle *bundle =
> > +		rdma_udata_to_uverbs_attr_bundle(udata);
> > +
> > +	lockdep_assert_held(&bundle->ufile->device->disassociate_srcu);
> > +
> > +	if (bundle->context)
> > +		return bundle->context->device;
> > +
> > +	/*
> > +	 * If the context hasn't been created yet use the ufile's dev, but it
> > +	 * might be NULL if we are racing with disassociate.
> > +	 */
> > +	return srcu_dereference(bundle->ufile->device->ib_dev,
> > +				&bundle->ufile->device->disassociate_srcu);
> > +}
> > +EXPORT_SYMBOL(rdma_udata_to_dev);
> 
> Thanks for this function, looks great, however I wonder about use of it
> in debug prints. What about changing:
> 
> +       ibdev_dbg(
> +               rdma_udata_to_dev(udata),
> ...
> 
> to be something
> +       udata_dbg(
> +               udata,
> ....
> 
> Together with keeping this rdma_udata_to_dev() function.

I thought about that, and looked at it, but the entire flow to build
the dbg prints goes through alot of common infrastructure and it
rapidly became a big pain to put the udata through like that.

All drivers should be able to get the dev from their own information
so shouldn't be using the udata version at all.

Thus I'd prefer this since it probably won't be more than these couple
callers from the core code.

Arguably discouraging drivers from using it is a positive.

Jason