Re: [PATCH v3] IB/uverbs: Add UVERBS_ATTR_FLAGS_IN to the specs language

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Aug 09, 2018 at 12:54:00PM +0000, Ruhl, Michael J wrote:
> >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
> >+++ 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?

Indeed, this was missed during testing apparently. It should read

-               memcpy(&flags, &attr->ptr_attr.data, 4);
+               flags = *(u32 *)&attr->ptr_attr.data;

Jason
--
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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux