>-----Original Message----- >From: linux-rdma-owner@xxxxxxxxxxxxxxx [mailto:linux-rdma- >owner@xxxxxxxxxxxxxxx] On Behalf Of Jason Gunthorpe >Sent: Thursday, July 26, 2018 6:37 PM >To: linux-rdma@xxxxxxxxxxxxxxx; Leon Romanovsky <leonro@xxxxxxxxxxxx> >Subject: [PATCH v3] IB/uverbs: Add UVERBS_ATTR_FLAGS_IN to the specs >language > >This clearly indicates that the input is a bitwise combination of values >in an enum, and identifies which enum contains the definition of the bits. > >Special accessors are provided that handle the mandatory validation of the >allowed bits and enforce the correct type for bitwise flags. > >If we had introduced this at the start then the kabi would have uniformly >used u64 data to pass flags, however today there is a mixture of u64 and >u32 flags. All places are converted to accept both sizes and the accessor >fixes it. This allows all existing flags to grow to u64 in future without >any hassle. > >Finally all flags are, by definition, optional. If flags are not passed >the accessor does not fail, but provides a value of zero. > >Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxxxx> >--- > drivers/infiniband/core/uverbs_ioctl.c | 51 +++++++++++++++++++ > .../core/uverbs_std_types_counters.c | 10 ++-- > drivers/infiniband/core/uverbs_std_types_cq.c | 13 +++-- > drivers/infiniband/core/uverbs_std_types_mr.c | 10 ++-- > drivers/infiniband/hw/mlx5/devx.c | 16 +++--- > drivers/infiniband/hw/mlx5/main.c | 16 +++--- > include/rdma/uverbs_ioctl.h | 33 ++++++++++++ > 7 files changed, 119 insertions(+), 30 deletions(-) > >- v2 fixes compilation when !CONFIG_INFINIBAND_USER_ACCESS by provding > dummy static inline's. >- v3 uses BUILD_BUG_ON_ZERO to wrapper the spelling checking sizeof > instead of *0 > >diff --git a/drivers/infiniband/core/uverbs_ioctl.c >b/drivers/infiniband/core/uverbs_ioctl.c >index db7a92ea5dbe87..23a1777f26e278 100644 >--- a/drivers/infiniband/core/uverbs_ioctl.c >+++ b/drivers/infiniband/core/uverbs_ioctl.c >@@ -486,3 +486,54 @@ long ib_uverbs_ioctl(struct file *filp, unsigned int >cmd, unsigned long arg) > > return err; > } >+ >+int uverbs_get_flags64(u64 *to, const struct uverbs_attr_bundle >*attrs_bundle, >+ size_t idx, u64 allowed_bits) >+{ >+ const struct uverbs_attr *attr; >+ u64 flags; >+ >+ attr = uverbs_attr_get(attrs_bundle, idx); >+ /* Missing attribute means 0 flags */ >+ if (IS_ERR(attr)) { >+ *to = 0; >+ return 0; >+ } >+ >+ /* >+ * New userspace code should use 8 bytes to pass flags, but we >+ * transparently support old userspaces that were using 4 bytes as >+ * well. >+ */ >+ if (attr->ptr_attr.len == 8) >+ flags = attr->ptr_attr.data; >+ else if (attr->ptr_attr.len == 4) >+ memcpy(&flags, &attr->ptr_attr.data, 4); If you are doing this, can't the top 4 bytes have garbage from the stack? Thanks, Mike >+ else >+ return -EINVAL; >+ >+ if (flags & ~allowed_bits) >+ return -EINVAL; >+ >+ *to = flags; >+ return 0; >+} >+EXPORT_SYMBOL(uverbs_get_flags64); >+ >+int uverbs_get_flags32(u32 *to, const struct uverbs_attr_bundle >*attrs_bundle, >+ size_t idx, u64 allowed_bits) >+{ >+ u64 flags; >+ int ret; >+ >+ ret = uverbs_get_flags64(&flags, attrs_bundle, idx, allowed_bits); >+ if (ret) >+ return ret; >+ >+ if (flags > U32_MAX) >+ return -EINVAL; >+ *to = flags; >+ >+ return 0; >+} >+EXPORT_SYMBOL(uverbs_get_flags32); >diff --git a/drivers/infiniband/core/uverbs_std_types_counters.c >b/drivers/infiniband/core/uverbs_std_types_counters.c >index dfe59ad721f635..34589799f4461a 100644 >--- a/drivers/infiniband/core/uverbs_std_types_counters.c >+++ b/drivers/infiniband/core/uverbs_std_types_counters.c >@@ -97,8 +97,9 @@ static int >UVERBS_HANDLER(UVERBS_METHOD_COUNTERS_READ)(struct ib_device >*ib_dev, > if (!atomic_read(&counters->usecnt)) > return -EINVAL; > >- ret = uverbs_copy_from(&read_attr.flags, attrs, >- UVERBS_ATTR_READ_COUNTERS_FLAGS); >+ ret = uverbs_get_flags32(&read_attr.flags, attrs, >+ UVERBS_ATTR_READ_COUNTERS_FLAGS, >+ >IB_UVERBS_READ_COUNTERS_PREFER_CACHED); > if (ret) > return ret; > >@@ -147,9 +148,8 @@ DECLARE_UVERBS_NAMED_METHOD( > UVERBS_ATTR_PTR_OUT(UVERBS_ATTR_READ_COUNTERS_BUFF, > UVERBS_ATTR_MIN_SIZE(0), > UA_MANDATORY), >- UVERBS_ATTR_PTR_IN(UVERBS_ATTR_READ_COUNTERS_FLAGS, >- UVERBS_ATTR_TYPE(__u32), >- UA_MANDATORY)); >+ UVERBS_ATTR_FLAGS_IN(UVERBS_ATTR_READ_COUNTERS_FLAGS, >+ enum ib_uverbs_read_counters_flags)); > > DECLARE_UVERBS_NAMED_OBJECT(UVERBS_OBJECT_COUNTERS, > UVERBS_TYPE_ALLOC_IDR(uverbs_free_counters), >diff --git a/drivers/infiniband/core/uverbs_std_types_cq.c >b/drivers/infiniband/core/uverbs_std_types_cq.c >index c71305fc043332..3179203a2dd7f6 100644 >--- a/drivers/infiniband/core/uverbs_std_types_cq.c >+++ b/drivers/infiniband/core/uverbs_std_types_cq.c >@@ -84,10 +84,12 @@ static int >UVERBS_HANDLER(UVERBS_METHOD_CQ_CREATE)(struct ib_device *ib_dev, > if (ret) > return ret; > >- /* Optional param, if it doesn't exist, we get -ENOENT and skip it */ >- if (IS_UVERBS_COPY_ERR(uverbs_copy_from(&attr.flags, attrs, >- > UVERBS_ATTR_CREATE_CQ_FLAGS))) >- return -EFAULT; >+ ret = uverbs_get_flags32(&attr.flags, attrs, >+ UVERBS_ATTR_CREATE_CQ_FLAGS, >+ >IB_UVERBS_CQ_FLAGS_TIMESTAMP_COMPLETION | >+ >IB_UVERBS_CQ_FLAGS_IGNORE_OVERRUN); >+ if (ret) >+ return ret; > > ev_file_uobj = uverbs_attr_get_uobject(attrs, >UVERBS_ATTR_CREATE_CQ_COMP_CHANNEL); > if (!IS_ERR(ev_file_uobj)) { >@@ -164,7 +166,8 @@ DECLARE_UVERBS_NAMED_METHOD( > UVERBS_ATTR_PTR_IN(UVERBS_ATTR_CREATE_CQ_COMP_VECTOR, > UVERBS_ATTR_TYPE(u32), > UA_MANDATORY), >- UVERBS_ATTR_PTR_IN(UVERBS_ATTR_CREATE_CQ_FLAGS, >UVERBS_ATTR_TYPE(u32)), >+ UVERBS_ATTR_FLAGS_IN(UVERBS_ATTR_CREATE_CQ_FLAGS, >+ enum ib_uverbs_ex_create_cq_flags), > UVERBS_ATTR_PTR_OUT(UVERBS_ATTR_CREATE_CQ_RESP_CQE, > UVERBS_ATTR_TYPE(u32), > UA_MANDATORY), >diff --git a/drivers/infiniband/core/uverbs_std_types_mr.c >b/drivers/infiniband/core/uverbs_std_types_mr.c >index c1b9124d611e79..d63da0c2a8c175 100644 >--- a/drivers/infiniband/core/uverbs_std_types_mr.c >+++ b/drivers/infiniband/core/uverbs_std_types_mr.c >@@ -62,8 +62,9 @@ static int >UVERBS_HANDLER(UVERBS_METHOD_DM_MR_REG)(struct ib_device >*ib_dev, > if (ret) > return ret; > >- ret = uverbs_copy_from(&attr.access_flags, attrs, >- UVERBS_ATTR_REG_DM_MR_ACCESS_FLAGS); >+ ret = uverbs_get_flags32(&attr.access_flags, attrs, >+ >UVERBS_ATTR_REG_DM_MR_ACCESS_FLAGS, >+ IB_ACCESS_SUPPORTED); > if (ret) > return ret; > >@@ -131,9 +132,8 @@ DECLARE_UVERBS_NAMED_METHOD( > UVERBS_OBJECT_PD, > UVERBS_ACCESS_READ, > UA_MANDATORY), >- > UVERBS_ATTR_PTR_IN(UVERBS_ATTR_REG_DM_MR_ACCESS_FLAGS >, >- UVERBS_ATTR_TYPE(u32), >- UA_MANDATORY), >+ > UVERBS_ATTR_FLAGS_IN(UVERBS_ATTR_REG_DM_MR_ACCESS_FLA >GS, >+ enum ib_access_flags), > UVERBS_ATTR_IDR(UVERBS_ATTR_REG_DM_MR_DM_HANDLE, > UVERBS_OBJECT_DM, > UVERBS_ACCESS_READ, >diff --git a/drivers/infiniband/hw/mlx5/devx.c >b/drivers/infiniband/hw/mlx5/devx.c >index fee800f2fdec9f..c9a7a12a8c13e3 100644 >--- a/drivers/infiniband/hw/mlx5/devx.c >+++ b/drivers/infiniband/hw/mlx5/devx.c >@@ -858,16 +858,21 @@ static int devx_umem_get(struct mlx5_ib_dev *dev, >struct ib_ucontext *ucontext, > { > u64 addr; > size_t size; >- int access; >+ u32 access; > int npages; > int err; > u32 page_mask; > > if (uverbs_copy_from(&addr, attrs, >MLX5_IB_ATTR_DEVX_UMEM_REG_ADDR) || >- uverbs_copy_from(&size, attrs, >MLX5_IB_ATTR_DEVX_UMEM_REG_LEN) || >- uverbs_copy_from(&access, attrs, >MLX5_IB_ATTR_DEVX_UMEM_REG_ACCESS)) >+ uverbs_copy_from(&size, attrs, >MLX5_IB_ATTR_DEVX_UMEM_REG_LEN)) > return -EFAULT; > >+ err = uverbs_get_flags32(&access, attrs, >+ MLX5_IB_ATTR_DEVX_UMEM_REG_ACCESS, >+ IB_ACCESS_SUPPORTED); >+ if (err) >+ return err; >+ > err = ib_check_mr_access(access); > if (err) > return err; >@@ -1012,9 +1017,8 @@ DECLARE_UVERBS_NAMED_METHOD( > UVERBS_ATTR_PTR_IN(MLX5_IB_ATTR_DEVX_UMEM_REG_LEN, > UVERBS_ATTR_TYPE(u64), > UA_MANDATORY), >- UVERBS_ATTR_PTR_IN(MLX5_IB_ATTR_DEVX_UMEM_REG_ACCESS, >- UVERBS_ATTR_TYPE(u32), >- UA_MANDATORY), >+ > UVERBS_ATTR_FLAGS_IN(MLX5_IB_ATTR_DEVX_UMEM_REG_ACCES >S, >+ enum ib_access_flags), > > UVERBS_ATTR_PTR_OUT(MLX5_IB_ATTR_DEVX_UMEM_REG_OUT_I >D, > UVERBS_ATTR_TYPE(u32), > UA_MANDATORY)); >diff --git a/drivers/infiniband/hw/mlx5/main.c >b/drivers/infiniband/hw/mlx5/main.c >index 61c78f4e4ebcdd..06d6309b719a87 100644 >--- a/drivers/infiniband/hw/mlx5/main.c >+++ b/drivers/infiniband/hw/mlx5/main.c >@@ -3859,12 +3859,11 @@ mlx5_ib_create_flow_action_esp(struct ib_device >*device, > u64 flags; > int err = 0; > >- if (IS_UVERBS_COPY_ERR(uverbs_copy_from(&action_flags, attrs, >- > MLX5_IB_ATTR_CREATE_FLOW_ACTION_FLAGS))) >- return ERR_PTR(-EFAULT); >- >- if (action_flags >= >(MLX5_FLOW_ACTION_ESP_CREATE_LAST_SUPPORTED << 1)) >- return ERR_PTR(-EOPNOTSUPP); >+ err = uverbs_get_flags64( >+ &action_flags, attrs, >MLX5_IB_ATTR_CREATE_FLOW_ACTION_FLAGS, >+ ((MLX5_FLOW_ACTION_ESP_CREATE_LAST_SUPPORTED << >1) - 1)); >+ if (err) >+ return ERR_PTR(err); > > flags = mlx5_ib_flow_action_flags_to_accel_xfrm_flags(action_flags); > >@@ -5531,9 +5530,8 @@ ADD_UVERBS_ATTRIBUTES_SIMPLE( > mlx5_ib_flow_action, > UVERBS_OBJECT_FLOW_ACTION, > UVERBS_METHOD_FLOW_ACTION_ESP_CREATE, >- > UVERBS_ATTR_PTR_IN(MLX5_IB_ATTR_CREATE_FLOW_ACTION_FLA >GS, >- UVERBS_ATTR_TYPE(u64), >- UA_MANDATORY)); >+ > UVERBS_ATTR_FLAGS_IN(MLX5_IB_ATTR_CREATE_FLOW_ACTION_F >LAGS, >+ enum mlx5_ib_uapi_flow_action_flags)); > > #define NUM_TREES 5 > static int populate_specs_root(struct mlx5_ib_dev *dev) >diff --git a/include/rdma/uverbs_ioctl.h b/include/rdma/uverbs_ioctl.h >index d16d31d4322d4f..5e6d0569d97c09 100644 >--- a/include/rdma/uverbs_ioctl.h >+++ b/include/rdma/uverbs_ioctl.h >@@ -268,6 +268,19 @@ struct uverbs_object_tree_def { > __VA_ARGS__ }, \ > }) > >+/* >+ * An input value that is a bitwise combination of values of _enum_type. >+ * This permits the flag value to be passed as either a u32 or u64, it must >+ * be retrieved via uverbs_get_flag(). >+ */ >+#define UVERBS_ATTR_FLAGS_IN(_attr_id, _enum_type, ...) \ >+ UVERBS_ATTR_PTR_IN( \ >+ _attr_id, \ >+ UVERBS_ATTR_SIZE(sizeof(u32) + BUILD_BUG_ON_ZERO( >\ >+ !sizeof(_enum_type *)), \ >+ sizeof(u64)), \ >+ __VA_ARGS__) >+ > /* > * This spec is used in order to pass information to the hardware driver in a > * legacy way. Every verb that could get driver specific data should get this >@@ -520,6 +533,26 @@ static inline int _uverbs_copy_from_or_zero(void >*to, > #define uverbs_copy_from_or_zero(to, attrs_bundle, idx) > \ > _uverbs_copy_from_or_zero(to, attrs_bundle, idx, sizeof(*to)) > >+#if IS_ENABLED(CONFIG_INFINIBAND_USER_ACCESS) >+int uverbs_get_flags64(u64 *to, const struct uverbs_attr_bundle >*attrs_bundle, >+ size_t idx, u64 allowed_bits); >+int uverbs_get_flags32(u32 *to, const struct uverbs_attr_bundle >*attrs_bundle, >+ size_t idx, u64 allowed_bits); >+#else >+static inline int >+uverbs_get_flags64(u64 *to, const struct uverbs_attr_bundle >*attrs_bundle, >+ size_t idx, u64 allowed_bits) >+{ >+ return -EINVAL; >+} >+static inline int >+uverbs_get_flags32(u32 *to, const struct uverbs_attr_bundle >*attrs_bundle, >+ size_t idx, u64 allowed_bits) >+{ >+ return -EINVAL; >+} >+#endif >+ > /* ================================================= > * Definitions -> Specs infrastructure > * ================================================= >-- >2.18.0 > >-- >To unsubscribe from this list: send the line "unsubscribe linux-rdma" in >the body of a message to majordomo@xxxxxxxxxxxxxxx >More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
