[PATCH rdma-next 00/14] Refactor ib_uverbs_write path

[PATCH rdma-next 00/14] Refactor ib_uverbs_write path

[Leon Romanovsky]

Hi,

This series comes as an outcome of [1] which was sent to rdma-rc
and it should be applied on top of that patch.

During review process of that patch, we realized that the
ib_uverbs_write() has number of flaws and this series
tries to fix them:
1. Localize all processing and verification code in specific functions.
2. Fix the command mask insufficient checks.
3. Simplify code to allow drop uverbs_ex_mask and in the future we
   will drop uverbs_mask too.
4. Fix differences in error codes.

Thanks

[1] https://marc.info/?l=linux-rdma&m=151851719021404&w=2

Leon Romanovsky (14):
  RDMA/uverbs: Convert command mask validity check function to be bool
  RDMA/uverbs: Update sizeof users
  RDMA/uverbs: Refactor flags checks and update return value
  RDMA/uverbs: Fail as early as possible if not enough header data was
    provided
  RDMA/uverbs: Return not supported error code for unsupported commands
  RDMA/uverbs: Unify return values of not supported command
  RDMA/uverbs: Refactor command header processing
  RDMA/uverbs: Properly check command supported mask
  RDMA/uverbs: Move uncontext check before SRCU read lock
  RDMa/uverbs: Copy ex_hdr outside of SRCU read lock
  RDMA/uverbs: Refactor the header validation logic
  RDMA/verbs: Return proper error code for not supported system call
  RDMA/verbs: Check existence of function prior to accessing it
  RDMA/verbs: Drop uverbs_ex_mask

 drivers/infiniband/core/core_priv.h   |   3 +
 drivers/infiniband/core/uverbs_cmd.c  |  21 ++++
 drivers/infiniband/core/uverbs_main.c | 219 ++++++++++++++++------------------
 drivers/infiniband/core/verbs.c       |  46 +++----
 drivers/infiniband/hw/mlx4/main.c     |  18 ---
 drivers/infiniband/hw/mlx5/main.c     |  15 ---
 include/rdma/ib_verbs.h               |   1 -
 7 files changed, 150 insertions(+), 173 deletions(-)

--
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next 01/14] RDMA/uverbs: Convert command mask validity check function to be bool

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

The function validate_command_mask() returns only two results: success
or failure, so convert it to return bool instead of 0 and -1.

Reported-by: Noa Osherovich <noaos-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Reviewed-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index b36cb12b3f38..1be9a93f628c 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -635,7 +635,7 @@ struct file *ib_uverbs_alloc_async_event_file(struct ib_uverbs_file *uverbs_file
 	return filp;
 }
 
-static int verify_command_mask(struct ib_device *ib_dev, __u32 command)
+static bool verify_command_mask(struct ib_device *ib_dev, __u32 command)
 {
 	u64 mask;
 
@@ -645,9 +645,9 @@ static int verify_command_mask(struct ib_device *ib_dev, __u32 command)
 		mask = ib_dev->uverbs_ex_cmd_mask;
 
 	if (mask & ((u64)1 << command))
-		return 0;
+		return true;
 
-	return -1;
+	return false;
 }
 
 static bool verify_command_idx(__u32 command, bool extended)
@@ -706,7 +706,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		goto out;
 	}
 
-	if (verify_command_mask(ib_dev, command)) {
+	if (!verify_command_mask(ib_dev, command)) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 01/14] RDMA/uverbs: Convert command mask validity check function to be bool

[Dennis Dalessandro]

On 2/14/2018 7:38 AM, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> The function validate_command_mask() returns only two results: success
> or failure, so convert it to return bool instead of 0 and -1.
> 
> Reported-by: Noa Osherovich <noaos-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---

Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 01/14] RDMA/uverbs: Convert command mask validity check function to be bool

[Jason Gunthorpe]

On Wed, Feb 14, 2018 at 02:38:31PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> The function validate_command_mask() returns only two results: success
> or failure, so convert it to return bool instead of 0 and -1.
> 
> Reported-by: Noa Osherovich <noaos-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>  drivers/infiniband/core/uverbs_main.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index b36cb12b3f38..1be9a93f628c 100644
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -635,7 +635,7 @@ struct file *ib_uverbs_alloc_async_event_file(struct ib_uverbs_file *uverbs_file
>  	return filp;
>  }
>  
> -static int verify_command_mask(struct ib_device *ib_dev, __u32 command)
> +static bool verify_command_mask(struct ib_device *ib_dev, __u32 command)

Shouldn't be __u32, just u32 for in-kernel stuff

Same comment for all patches

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 01/14] RDMA/uverbs: Convert command mask validity check function to be bool

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1324 bytes --]

On Wed, Feb 14, 2018 at 04:41:41PM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 14, 2018 at 02:38:31PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > The function validate_command_mask() returns only two results: success
> > or failure, so convert it to return bool instead of 0 and -1.
> >
> > Reported-by: Noa Osherovich <noaos-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Reviewed-by: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >  drivers/infiniband/core/uverbs_main.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> > index b36cb12b3f38..1be9a93f628c 100644
> > +++ b/drivers/infiniband/core/uverbs_main.c
> > @@ -635,7 +635,7 @@ struct file *ib_uverbs_alloc_async_event_file(struct ib_uverbs_file *uverbs_file
> >  	return filp;
> >  }
> >
> > -static int verify_command_mask(struct ib_device *ib_dev, __u32 command)
> > +static bool verify_command_mask(struct ib_device *ib_dev, __u32 command)
>
> Shouldn't be __u32, just u32 for in-kernel stuff
>
> Same comment for all patches

Yes, it was __u32 from the beginning.

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH rdma-next 02/14] RDMA/uverbs: Update sizeof users

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Update sizeof() users to be consistent with coding style.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 1be9a93f628c..06ddb93f9c75 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -468,7 +468,7 @@ void ib_uverbs_comp_handler(struct ib_cq *cq, void *cq_context)
 		return;
 	}
 
-	entry = kmalloc(sizeof *entry, GFP_ATOMIC);
+	entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
 	if (!entry) {
 		spin_unlock_irqrestore(&ev_queue->lock, flags);
 		return;
@@ -501,7 +501,7 @@ static void ib_uverbs_async_handler(struct ib_uverbs_file *file,
 		return;
 	}
 
-	entry = kmalloc(sizeof *entry, GFP_ATOMIC);
+	entry = kmalloc(sizeof(*entry), GFP_ATOMIC);
 	if (!entry) {
 		spin_unlock_irqrestore(&file->async_file->ev_queue.lock, flags);
 		return;
@@ -676,10 +676,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		return -EACCES;
 	}
 
-	if (count < sizeof hdr)
+	if (count < sizeof(hdr))
 		return -EINVAL;
 
-	if (copy_from_user(&hdr, buf, sizeof hdr))
+	if (copy_from_user(&hdr, buf, sizeof(hdr)))
 		return -EFAULT;
 
 	srcu_key = srcu_read_lock(&file->device->disassociate_srcu);
@@ -1032,7 +1032,7 @@ static void ib_uverbs_add_one(struct ib_device *device)
 	if (!device->alloc_ucontext)
 		return;
 
-	uverbs_dev = kzalloc(sizeof *uverbs_dev, GFP_KERNEL);
+	uverbs_dev = kzalloc(sizeof(*uverbs_dev), GFP_KERNEL);
 	if (!uverbs_dev)
 		return;
 
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 02/14] RDMA/uverbs: Update sizeof users

[Dennis Dalessandro]

On 2/14/2018 7:38 AM, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> Update sizeof() users to be consistent with coding style.
> 
> Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Reviewed-by: Dennis Dalessandro <dennis.dalessandro-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next 03/14] RDMA/uverbs: Refactor flags checks and update return value

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Since commit f21519b23c1b ("IB/core: extended command: an
improved infrastructure for uverbs commands"), the uverbs
supports extra flags as an input to the command interface.

However actually, there is only one flag available and used,
so it is better to refactor the code, so the resolution and
report to the users is done as early as possible.

As part of this change, we changed the return value of failure case
from ENOSYS to be EINVAL to be consistent with the rest flags checks.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 06ddb93f9c75..c12608b2d1cc 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -701,6 +701,11 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		 IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
 
 	extended_command = flags & IB_USER_VERBS_CMD_FLAG_EXTENDED;
+	if (flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	if (!verify_command_idx(command, extended_command)) {
 		ret = -EINVAL;
 		goto out;
@@ -732,8 +737,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 						 buf + sizeof(hdr),
 						 hdr.in_words * 4,
 						 hdr.out_words * 4);
-
-	} else if (flags == IB_USER_VERBS_CMD_FLAG_EXTENDED) {
+	} else {
 		struct ib_uverbs_ex_cmd_hdr ex_hdr;
 		struct ib_udata ucore;
 		struct ib_udata uhw;
@@ -804,8 +808,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		ret = uverbs_ex_cmd_table[command](file, ib_dev, &ucore, &uhw);
 		if (!ret)
 			ret = written_count;
-	} else {
-		ret = -ENOSYS;
 	}
 
 out:
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 03/14] RDMA/uverbs: Refactor flags checks and update return value

[Jason Gunthorpe]

On Wed, Feb 14, 2018 at 02:38:33PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> Since commit f21519b23c1b ("IB/core: extended command: an
> improved infrastructure for uverbs commands"), the uverbs
> supports extra flags as an input to the command interface.
> 
> However actually, there is only one flag available and used,
> so it is better to refactor the code, so the resolution and
> report to the users is done as early as possible.
> 
> As part of this change, we changed the return value of failure case
> from ENOSYS to be EINVAL to be consistent with the rest flags checks.
> 
> Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>  drivers/infiniband/core/uverbs_main.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index 06ddb93f9c75..c12608b2d1cc 100644
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -701,6 +701,11 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>  		 IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
>  
>  	extended_command = flags & IB_USER_VERBS_CMD_FLAG_EXTENDED;
> +	if (flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED) {

This is not strict enough for flags checking.

It should be

 extended = hdr.command & IB_USER_VERBS_CMD_FLAG_EXTENDED <<  IB_USER_VERBS_CMD_FLAGS_SHIFT;
 flags = hdr.command & ~(u32)IB_USER_VERBS_CMD_COMMAND_MASK;
 if (flags != 0 && flags != IB_USER_VERBS_CMD_FLAG_EXTENDED << IB_USER_VERBS_CMD_FLAGS_SHIFT) {
     ret = -EINVAL;
     goto out;
 }

We need to check all the bits that are not command bits for validity.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next 04/14] RDMA/uverbs: Fail as early as possible if not enough header data was provided

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Fail as early as possible if not enough header data
was provided.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index c12608b2d1cc..f50ad3688790 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -662,6 +662,7 @@ 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_uverbs_ex_cmd_hdr ex_hdr;
 	struct ib_device *ib_dev;
 	struct ib_uverbs_cmd_hdr hdr;
 	bool extended_command;
@@ -706,6 +707,12 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		goto out;
 	}
 
+	if (extended_command &&
+	    count < (sizeof(hdr) + sizeof(ex_hdr))) {
+		ret = -EINVAL;
+		goto out;
+	}
+
 	if (!verify_command_idx(command, extended_command)) {
 		ret = -EINVAL;
 		goto out;
@@ -738,7 +745,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 						 hdr.in_words * 4,
 						 hdr.out_words * 4);
 	} else {
-		struct ib_uverbs_ex_cmd_hdr ex_hdr;
 		struct ib_udata ucore;
 		struct ib_udata uhw;
 		size_t written_count = count;
@@ -753,11 +759,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 			goto out;
 		}
 
-		if (count < (sizeof(hdr) + sizeof(ex_hdr))) {
-			ret = -EINVAL;
-			goto out;
-		}
-
 		if (copy_from_user(&ex_hdr, buf + sizeof(hdr), sizeof(ex_hdr))) {
 			ret = -EFAULT;
 			goto out;
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next 05/14] RDMA/uverbs: Return not supported error code for unsupported commands

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Command that doesn't exist means that it is not supported,
so update code to return -EOPNOTSUPP in case of failure.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index f50ad3688790..d8d6ac91981b 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -714,7 +714,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	}
 
 	if (!verify_command_idx(command, extended_command)) {
-		ret = -EINVAL;
+		ret = -EOPNOTSUPP;
 		goto out;
 	}
 
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next 06/14] RDMA/uverbs: Unify return values of not supported command

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

The non-existing command is supposed to return -EOPNOTSUPP, but the
current code returns different errors for different flows for the
same failure. This patch unifies those flows.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 16 ++++------------
 1 file changed, 4 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index d8d6ac91981b..90e0b16aed1a 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -653,9 +653,11 @@ static bool verify_command_mask(struct ib_device *ib_dev, __u32 command)
 static bool verify_command_idx(__u32 command, bool extended)
 {
 	if (extended)
-		return command < ARRAY_SIZE(uverbs_ex_cmd_table);
+		return command < ARRAY_SIZE(uverbs_ex_cmd_table) &&
+		       uverbs_ex_cmd_table[command];
 
-	return command < ARRAY_SIZE(uverbs_cmd_table);
+	return command < ARRAY_SIZE(uverbs_cmd_table) &&
+	       uverbs_cmd_table[command];
 }
 
 static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
@@ -730,11 +732,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	}
 
 	if (!flags) {
-		if (!uverbs_cmd_table[command]) {
-			ret = -EINVAL;
-			goto out;
-		}
-
 		if (hdr.in_words * 4 != count) {
 			ret = -EINVAL;
 			goto out;
@@ -749,11 +746,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		struct ib_udata uhw;
 		size_t written_count = count;
 
-		if (!uverbs_ex_cmd_table[command]) {
-			ret = -ENOSYS;
-			goto out;
-		}
-
 		if (!file->ucontext) {
 			ret = -EINVAL;
 			goto out;
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next 07/14] RDMA/uverbs: Refactor command header processing

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Move all command header processing into separate function
and perform those checks before acquiring SRCU read lock.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 62 ++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 30 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 90e0b16aed1a..50d43c300112 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -660,6 +660,29 @@ static bool verify_command_idx(__u32 command, bool extended)
 	       uverbs_cmd_table[command];
 }
 
+static ssize_t process_hdr(struct ib_uverbs_cmd_hdr *hdr,
+			   __u32 *command, bool *extended)
+{
+	__u32 flags;
+
+	if (hdr->command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
+				   IB_USER_VERBS_CMD_COMMAND_MASK))
+		return -EINVAL;
+
+	*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 = flags & IB_USER_VERBS_CMD_FLAG_EXTENDED;
+	if (flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED)
+		return -EINVAL;
+
+	if (!verify_command_idx(*command, *extended))
+		return -EOPNOTSUPP;
+
+	return 0;
+}
+
 static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 			     size_t count, loff_t *pos)
 {
@@ -667,9 +690,8 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	struct ib_uverbs_ex_cmd_hdr ex_hdr;
 	struct ib_device *ib_dev;
 	struct ib_uverbs_cmd_hdr hdr;
-	bool extended_command;
+	bool extended;
 	__u32 command;
-	__u32 flags;
 	int srcu_key;
 	ssize_t ret;
 
@@ -685,6 +707,13 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	if (copy_from_user(&hdr, buf, sizeof(hdr)))
 		return -EFAULT;
 
+	ret = process_hdr(&hdr, &command, &extended);
+	if (ret)
+		return ret;
+
+	if (extended && count < (sizeof(hdr) + sizeof(ex_hdr)))
+		return -EINVAL;
+
 	srcu_key = srcu_read_lock(&file->device->disassociate_srcu);
 	ib_dev = srcu_dereference(file->device->ib_dev,
 				  &file->device->disassociate_srcu);
@@ -693,33 +722,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		goto out;
 	}
 
-	if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
-				   IB_USER_VERBS_CMD_COMMAND_MASK)) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	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 (flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	if (extended_command &&
-	    count < (sizeof(hdr) + sizeof(ex_hdr))) {
-		ret = -EINVAL;
-		goto out;
-	}
-
-	if (!verify_command_idx(command, extended_command)) {
-		ret = -EOPNOTSUPP;
-		goto out;
-	}
-
 	if (!verify_command_mask(ib_dev, command)) {
 		ret = -EOPNOTSUPP;
 		goto out;
@@ -731,7 +733,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		goto out;
 	}
 
-	if (!flags) {
+	if (!extended) {
 		if (hdr.in_words * 4 != count) {
 			ret = -EINVAL;
 			goto out;
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 07/14] RDMA/uverbs: Refactor command header processing

[Jason Gunthorpe]

On Wed, Feb 14, 2018 at 02:38:37PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> Move all command header processing into separate function
> and perform those checks before acquiring SRCU read lock.
> 
> Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>  drivers/infiniband/core/uverbs_main.c | 62 ++++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index 90e0b16aed1a..50d43c300112 100644
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -660,6 +660,29 @@ static bool verify_command_idx(__u32 command, bool extended)
>  	       uverbs_cmd_table[command];
>  }
>  
> +static ssize_t process_hdr(struct ib_uverbs_cmd_hdr *hdr,
> +			   __u32 *command, bool *extended)
> +{
> +	__u32 flags;
> +
> +	if (hdr->command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> +				   IB_USER_VERBS_CMD_COMMAND_MASK))
> +		return -EINVAL;
> +
> +	*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 = flags & IB_USER_VERBS_CMD_FLAG_EXTENDED;
> +	if (flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED)
> +		return -EINVAL;

er..

We don't need both

	if (hdr->command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
				   IB_USER_VERBS_CMD_COMMAND_MASK))

and

        if (flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED)
		return -EINVAL;

They test the same condition.

So my earlier comment about needing a stronger flags check is bogus -
we already have the stronger check. The original code was just
checking it twice, so it doesn't need this extra if.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 07/14] RDMA/uverbs: Refactor command header processing

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2398 bytes --]

On Wed, Feb 14, 2018 at 04:49:51PM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 14, 2018 at 02:38:37PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > Move all command header processing into separate function
> > and perform those checks before acquiring SRCU read lock.
> >
> > Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >  drivers/infiniband/core/uverbs_main.c | 62 ++++++++++++++++++-----------------
> >  1 file changed, 32 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> > index 90e0b16aed1a..50d43c300112 100644
> > +++ b/drivers/infiniband/core/uverbs_main.c
> > @@ -660,6 +660,29 @@ static bool verify_command_idx(__u32 command, bool extended)
> >  	       uverbs_cmd_table[command];
> >  }
> >
> > +static ssize_t process_hdr(struct ib_uverbs_cmd_hdr *hdr,
> > +			   __u32 *command, bool *extended)
> > +{
> > +	__u32 flags;
> > +
> > +	if (hdr->command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> > +				   IB_USER_VERBS_CMD_COMMAND_MASK))
> > +		return -EINVAL;
> > +
> > +	*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 = flags & IB_USER_VERBS_CMD_FLAG_EXTENDED;
> > +	if (flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED)
> > +		return -EINVAL;
>
> er..
>
> We don't need both
>
> 	if (hdr->command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> 				   IB_USER_VERBS_CMD_COMMAND_MASK))
>
> and
>
>         if (flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED)
> 		return -EINVAL;
>
> They test the same condition.

Not exactly,
~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK | IB_USER_VERBS_CMD_COMMAND_MASK)
== ~(__u32)(0xff000000u| 0xff) == 0x00ffff00

=> we are testing that middle bits of hdr->command are cleared

flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED == ((hdr->command &
IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT) &
~IB_USER_VERBS_CMD_FLAG_EXTENDED == 0xff000000u >> 24 & ~0x80 ==
0xff & 0x7F == 0x7F

=> we are testing that only extended flag is enable.

>
> So my earlier comment about needing a stronger flags check is bogus -
> we already have the stronger check. The original code was just
> checking it twice, so it doesn't need this extra if.
>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next 07/14] RDMA/uverbs: Refactor command header processing

[Jason Gunthorpe]

On Thu, Feb 15, 2018 at 03:51:20PM +0200, Leon Romanovsky wrote:
> On Wed, Feb 14, 2018 at 04:49:51PM -0700, Jason Gunthorpe wrote:
> > On Wed, Feb 14, 2018 at 02:38:37PM +0200, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > >
> > > Move all command header processing into separate function
> > > and perform those checks before acquiring SRCU read lock.
> > >
> > > Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > >  drivers/infiniband/core/uverbs_main.c | 62 ++++++++++++++++++-----------------
> > >  1 file changed, 32 insertions(+), 30 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> > > index 90e0b16aed1a..50d43c300112 100644
> > > +++ b/drivers/infiniband/core/uverbs_main.c
> > > @@ -660,6 +660,29 @@ static bool verify_command_idx(__u32 command, bool extended)
> > >  	       uverbs_cmd_table[command];
> > >  }
> > >
> > > +static ssize_t process_hdr(struct ib_uverbs_cmd_hdr *hdr,
> > > +			   __u32 *command, bool *extended)
> > > +{
> > > +	__u32 flags;
> > > +
> > > +	if (hdr->command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> > > +				   IB_USER_VERBS_CMD_COMMAND_MASK))
> > > +		return -EINVAL;
> > > +
> > > +	*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 = flags & IB_USER_VERBS_CMD_FLAG_EXTENDED;
> > > +	if (flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED)
> > > +		return -EINVAL;
> >
> > er..
> >
> > We don't need both
> >
> > 	if (hdr->command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> > 				   IB_USER_VERBS_CMD_COMMAND_MASK))
> >
> > and
> >
> >         if (flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED)
> > 		return -EINVAL;
> >
> > They test the same condition.
> 
> Not exactly,
> ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK | IB_USER_VERBS_CMD_COMMAND_MASK)
> == ~(__u32)(0xff000000u| 0xff) == 0x00ffff00
> 
> => we are testing that middle bits of hdr->command are cleared
> 
> flags & ~IB_USER_VERBS_CMD_FLAG_EXTENDED == ((hdr->command &
> IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT) &
> ~IB_USER_VERBS_CMD_FLAG_EXTENDED == 0xff000000u >> 24 & ~0x80 ==
> 0xff & 0x7F == 0x7F
> 
> => we are testing that only extended flag is enable.

Well, I ment it should have just been:

 	if (hdr->command & ~(u32)(IB_USER_VERBS_CMD_FLAG_EXTENDED |
 				   IB_USER_VERBS_CMD_COMMAND_MASK))

Then we don't need two ifs

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next 08/14] RDMA/uverbs: Properly check command supported mask

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

The check based on index is not sufficient because

  IB_USER_VERBS_EX_CMD_CREATE_CQ = IB_USER_VERBS_CMD_CREATE_CQ

and IB_USER_VERBS_CMD_CREATE_CQ <= IB_USER_VERBS_CMD_OPEN_QP,
so if we execute IB_USER_VERBS_EX_CMD_CREATE_CQ this code checks
ib_dev->uverbs_cmd_mask not ib_dev->uverbs_ex_cmd_mask.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 50d43c300112..0cf3664cf7d4 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -635,19 +635,13 @@ struct file *ib_uverbs_alloc_async_event_file(struct ib_uverbs_file *uverbs_file
 	return filp;
 }
 
-static bool verify_command_mask(struct ib_device *ib_dev, __u32 command)
+static bool verify_command_mask(struct ib_device *ib_dev,
+				__u32 command, bool extended)
 {
-	u64 mask;
+	if (!extended)
+		return ib_dev->uverbs_cmd_mask & BIT_ULL(command);
 
-	if (command <= IB_USER_VERBS_CMD_OPEN_QP)
-		mask = ib_dev->uverbs_cmd_mask;
-	else
-		mask = ib_dev->uverbs_ex_cmd_mask;
-
-	if (mask & ((u64)1 << command))
-		return true;
-
-	return false;
+	return ib_dev->uverbs_ex_cmd_mask & BIT_ULL(command);
 }
 
 static bool verify_command_idx(__u32 command, bool extended)
@@ -722,7 +716,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		goto out;
 	}
 
-	if (!verify_command_mask(ib_dev, command)) {
+	if (!verify_command_mask(ib_dev, command, extended)) {
 		ret = -EOPNOTSUPP;
 		goto out;
 	}
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 08/14] RDMA/uverbs: Properly check command supported mask

[Jason Gunthorpe]

On Wed, Feb 14, 2018 at 02:38:38PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> The check based on index is not sufficient because
> 
>   IB_USER_VERBS_EX_CMD_CREATE_CQ = IB_USER_VERBS_CMD_CREATE_CQ
> 
> and IB_USER_VERBS_CMD_CREATE_CQ <= IB_USER_VERBS_CMD_OPEN_QP,
> so if we execute IB_USER_VERBS_EX_CMD_CREATE_CQ this code checks
> ib_dev->uverbs_cmd_mask not ib_dev->uverbs_ex_cmd_mask.
> 
> Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>  drivers/infiniband/core/uverbs_main.c | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)

This seems like an RC fix to me, since we are not properly validating
input commands... ??

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 08/14] RDMA/uverbs: Properly check command supported mask

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

On Wed, Feb 14, 2018 at 04:47:14PM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 14, 2018 at 02:38:38PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > The check based on index is not sufficient because
> >
> >   IB_USER_VERBS_EX_CMD_CREATE_CQ = IB_USER_VERBS_CMD_CREATE_CQ
> >
> > and IB_USER_VERBS_CMD_CREATE_CQ <= IB_USER_VERBS_CMD_OPEN_QP,
> > so if we execute IB_USER_VERBS_EX_CMD_CREATE_CQ this code checks
> > ib_dev->uverbs_cmd_mask not ib_dev->uverbs_ex_cmd_mask.
> >
> > Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >  drivers/infiniband/core/uverbs_main.c | 18 ++++++------------
> >  1 file changed, 6 insertions(+), 12 deletions(-)
>
> This seems like an RC fix to me, since we are not properly validating
> input commands... ??

I don't think so, it looks harmless to me because all vendors except mlx4/mlx5
have zero in uverbs_ex_cmd_mask and mlx4 have all commands implemented.

Thanks

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next 08/14] RDMA/uverbs: Properly check command supported mask

[Jason Gunthorpe]

On Thu, Feb 15, 2018 at 03:56:28PM +0200, Leon Romanovsky wrote:
> On Wed, Feb 14, 2018 at 04:47:14PM -0700, Jason Gunthorpe wrote:
> > On Wed, Feb 14, 2018 at 02:38:38PM +0200, Leon Romanovsky wrote:
> > > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > >
> > > The check based on index is not sufficient because
> > >
> > >   IB_USER_VERBS_EX_CMD_CREATE_CQ = IB_USER_VERBS_CMD_CREATE_CQ
> > >
> > > and IB_USER_VERBS_CMD_CREATE_CQ <= IB_USER_VERBS_CMD_OPEN_QP,
> > > so if we execute IB_USER_VERBS_EX_CMD_CREATE_CQ this code checks
> > > ib_dev->uverbs_cmd_mask not ib_dev->uverbs_ex_cmd_mask.
> > >
> > > Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > >  drivers/infiniband/core/uverbs_main.c | 18 ++++++------------
> > >  1 file changed, 6 insertions(+), 12 deletions(-)
> >
> > This seems like an RC fix to me, since we are not properly validating
> > input commands... ??
> 
> I don't think so, it looks harmless to me because all vendors except mlx4/mlx5
> have zero in uverbs_ex_cmd_mask and mlx4 have all commands implemented.

The issue is we check uverbs_cmd_mask when we should check
uverbs_ex_cmd_mask, so drivers with a 0 in uverbs_ex_cmd_mask will
still pass this check.

and your later patch checks for null, so what happens if, say, rxe
calls an ex command? kernel oops?

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 08/14] RDMA/uverbs: Properly check command supported mask

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1842 bytes --]

On Thu, Feb 15, 2018 at 09:26:04AM -0700, Jason Gunthorpe wrote:
> On Thu, Feb 15, 2018 at 03:56:28PM +0200, Leon Romanovsky wrote:
> > On Wed, Feb 14, 2018 at 04:47:14PM -0700, Jason Gunthorpe wrote:
> > > On Wed, Feb 14, 2018 at 02:38:38PM +0200, Leon Romanovsky wrote:
> > > > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > > >
> > > > The check based on index is not sufficient because
> > > >
> > > >   IB_USER_VERBS_EX_CMD_CREATE_CQ = IB_USER_VERBS_CMD_CREATE_CQ
> > > >
> > > > and IB_USER_VERBS_CMD_CREATE_CQ <= IB_USER_VERBS_CMD_OPEN_QP,
> > > > so if we execute IB_USER_VERBS_EX_CMD_CREATE_CQ this code checks
> > > > ib_dev->uverbs_cmd_mask not ib_dev->uverbs_ex_cmd_mask.
> > > >
> > > > Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > > >  drivers/infiniband/core/uverbs_main.c | 18 ++++++------------
> > > >  1 file changed, 6 insertions(+), 12 deletions(-)
> > >
> > > This seems like an RC fix to me, since we are not properly validating
> > > input commands... ??
> >
> > I don't think so, it looks harmless to me because all vendors except mlx4/mlx5
> > have zero in uverbs_ex_cmd_mask and mlx4 have all commands implemented.
>
> The issue is we check uverbs_cmd_mask when we should check
> uverbs_ex_cmd_mask, so drivers with a 0 in uverbs_ex_cmd_mask will
> still pass this check.
>
> and your later patch checks for null, so what happens if, say, rxe
> calls an ex command? kernel oops?

So actually, my latest patch (addition of NULL checks) should go to the
-rc and not this one.

I still prefer to leave this patch in this series (-next) and avoid
writing completely thrown away code for -rc, which will create only
merge conflicts between rdma-rc and rdma-next without any visible
benefits.

I'll reshuffle this series and resubmit.

Thanks

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH rdma-next 08/14] RDMA/uverbs: Properly check command supported mask

[Jason Gunthorpe]

On Fri, Feb 16, 2018 at 09:31:47AM +0200, Leon Romanovsky wrote:
> On Thu, Feb 15, 2018 at 09:26:04AM -0700, Jason Gunthorpe wrote:
> > On Thu, Feb 15, 2018 at 03:56:28PM +0200, Leon Romanovsky wrote:
> > > On Wed, Feb 14, 2018 at 04:47:14PM -0700, Jason Gunthorpe wrote:
> > > > On Wed, Feb 14, 2018 at 02:38:38PM +0200, Leon Romanovsky wrote:
> > > > > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > > > >
> > > > > The check based on index is not sufficient because
> > > > >
> > > > >   IB_USER_VERBS_EX_CMD_CREATE_CQ = IB_USER_VERBS_CMD_CREATE_CQ
> > > > >
> > > > > and IB_USER_VERBS_CMD_CREATE_CQ <= IB_USER_VERBS_CMD_OPEN_QP,
> > > > > so if we execute IB_USER_VERBS_EX_CMD_CREATE_CQ this code checks
> > > > > ib_dev->uverbs_cmd_mask not ib_dev->uverbs_ex_cmd_mask.
> > > > >
> > > > > Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > > > >  drivers/infiniband/core/uverbs_main.c | 18 ++++++------------
> > > > >  1 file changed, 6 insertions(+), 12 deletions(-)
> > > >
> > > > This seems like an RC fix to me, since we are not properly validating
> > > > input commands... ??
> > >
> > > I don't think so, it looks harmless to me because all vendors except mlx4/mlx5
> > > have zero in uverbs_ex_cmd_mask and mlx4 have all commands implemented.
> >
> > The issue is we check uverbs_cmd_mask when we should check
> > uverbs_ex_cmd_mask, so drivers with a 0 in uverbs_ex_cmd_mask will
> > still pass this check.
> >
> > and your later patch checks for null, so what happens if, say, rxe
> > calls an ex command? kernel oops?
> 
> So actually, my latest patch (addition of NULL checks) should go to the
> -rc and not this one.

Sure, I guess that works too.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next 09/14] RDMA/uverbs: Move uncontext check before SRCU read lock

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

There is no need to take SRCU lock before checking
file->ucontext, so move it do it before it.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 0cf3664cf7d4..71ffee0dc305 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -705,6 +705,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	if (ret)
 		return ret;
 
+	if (!file->ucontext &&
+	    (command != IB_USER_VERBS_CMD_GET_CONTEXT || extended))
+		return -EINVAL;
+
 	if (extended && count < (sizeof(hdr) + sizeof(ex_hdr)))
 		return -EINVAL;
 
@@ -721,12 +725,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		goto out;
 	}
 
-	if (!file->ucontext &&
-	    command != IB_USER_VERBS_CMD_GET_CONTEXT) {
-		ret = -EINVAL;
-		goto out;
-	}
-
 	if (!extended) {
 		if (hdr.in_words * 4 != count) {
 			ret = -EINVAL;
@@ -742,11 +740,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		struct ib_udata uhw;
 		size_t written_count = count;
 
-		if (!file->ucontext) {
-			ret = -EINVAL;
-			goto out;
-		}
-
 		if (copy_from_user(&ex_hdr, buf + sizeof(hdr), sizeof(ex_hdr))) {
 			ret = -EFAULT;
 			goto out;
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next 10/14] RDMa/uverbs: Copy ex_hdr outside of SRCU read lock

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

The SRCU read lock protects the IB device pointer
and doesn't need to be called before copying user
provided header.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 71ffee0dc305..f8f2182ab86b 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -709,8 +709,12 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	    (command != IB_USER_VERBS_CMD_GET_CONTEXT || extended))
 		return -EINVAL;
 
-	if (extended && count < (sizeof(hdr) + sizeof(ex_hdr)))
-		return -EINVAL;
+	if (extended) {
+		if (count < (sizeof(hdr) + sizeof(ex_hdr)))
+			return -EINVAL;
+		if (copy_from_user(&ex_hdr, buf + sizeof(hdr), sizeof(ex_hdr)))
+			return -EFAULT;
+	}
 
 	srcu_key = srcu_read_lock(&file->device->disassociate_srcu);
 	ib_dev = srcu_dereference(file->device->ib_dev,
@@ -740,11 +744,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		struct ib_udata uhw;
 		size_t written_count = count;
 
-		if (copy_from_user(&ex_hdr, buf + sizeof(hdr), sizeof(ex_hdr))) {
-			ret = -EFAULT;
-			goto out;
-		}
-
 		count -= sizeof(hdr) + sizeof(ex_hdr);
 		buf += sizeof(hdr) + sizeof(ex_hdr);
 
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next 11/14] RDMA/uverbs: Refactor the header validation logic

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Move all header validation logic to be performed before SRCU read lock.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 90 ++++++++++++++++++-----------------
 1 file changed, 47 insertions(+), 43 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index f8f2182ab86b..e07326139ce9 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -677,6 +677,42 @@ static ssize_t process_hdr(struct ib_uverbs_cmd_hdr *hdr,
 	return 0;
 }
 
+static ssize_t verify_hdr(struct ib_uverbs_cmd_hdr *hdr,
+			  struct ib_uverbs_ex_cmd_hdr *ex_hdr,
+			  size_t count, bool extended)
+{
+	if (extended) {
+		count -= sizeof(*hdr) + sizeof(*ex_hdr);
+
+		if ((hdr->in_words + ex_hdr->provider_in_words) * 8 != count)
+			return -EINVAL;
+
+		if (ex_hdr->cmd_hdr_reserved)
+			return -EINVAL;
+
+		if (ex_hdr->response) {
+			if (!hdr->out_words && !ex_hdr->provider_out_words)
+				return -EINVAL;
+
+			if (!access_ok(VERIFY_WRITE,
+				       u64_to_user_ptr(ex_hdr->response),
+				       (hdr->out_words + ex_hdr->provider_out_words) * 8))
+				return -EFAULT;
+		} else {
+			if (hdr->out_words || ex_hdr->provider_out_words)
+				return -EINVAL;
+		}
+
+		return 0;
+	}
+
+	/* not extended command */
+	if (hdr->in_words * 4 != count)
+		return -EINVAL;
+
+	return 0;
+}
+
 static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 			     size_t count, loff_t *pos)
 {
@@ -716,6 +752,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 			return -EFAULT;
 	}
 
+	ret = verify_hdr(&hdr, &ex_hdr, count, extended);
+	if (ret)
+		return ret;
+
 	srcu_key = srcu_read_lock(&file->device->disassociate_srcu);
 	ib_dev = srcu_dereference(file->device->ib_dev,
 				  &file->device->disassociate_srcu);
@@ -729,52 +769,17 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		goto out;
 	}
 
-	if (!extended) {
-		if (hdr.in_words * 4 != count) {
-			ret = -EINVAL;
-			goto out;
-		}
+	buf += sizeof(hdr);
 
-		ret = uverbs_cmd_table[command](file, ib_dev,
-						 buf + sizeof(hdr),
-						 hdr.in_words * 4,
-						 hdr.out_words * 4);
+	if (!extended) {
+		ret = uverbs_cmd_table[command](file, ib_dev, buf,
+						hdr.in_words * 4,
+						hdr.out_words * 4);
 	} else {
 		struct ib_udata ucore;
 		struct ib_udata uhw;
-		size_t written_count = count;
-
-		count -= sizeof(hdr) + sizeof(ex_hdr);
-		buf += sizeof(hdr) + sizeof(ex_hdr);
 
-		if ((hdr.in_words + ex_hdr.provider_in_words) * 8 != count) {
-			ret = -EINVAL;
-			goto out;
-		}
-
-		if (ex_hdr.cmd_hdr_reserved) {
-			ret = -EINVAL;
-			goto out;
-		}
-
-		if (ex_hdr.response) {
-			if (!hdr.out_words && !ex_hdr.provider_out_words) {
-				ret = -EINVAL;
-				goto out;
-			}
-
-			if (!access_ok(VERIFY_WRITE,
-				       u64_to_user_ptr(ex_hdr.response),
-				       (hdr.out_words + ex_hdr.provider_out_words) * 8)) {
-				ret = -EFAULT;
-				goto out;
-			}
-		} else {
-			if (hdr.out_words || ex_hdr.provider_out_words) {
-				ret = -EINVAL;
-				goto out;
-			}
-		}
+		buf += sizeof(ex_hdr);
 
 		ib_uverbs_init_udata_buf_or_null(&ucore, buf,
 					u64_to_user_ptr(ex_hdr.response),
@@ -787,8 +792,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 					ex_hdr.provider_out_words * 8);
 
 		ret = uverbs_ex_cmd_table[command](file, ib_dev, &ucore, &uhw);
-		if (!ret)
-			ret = written_count;
+		ret = (ret) ? : count;
 	}
 
 out:
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 11/14] RDMA/uverbs: Refactor the header validation logic

[Jason Gunthorpe]

On Wed, Feb 14, 2018 at 02:38:41PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> Move all header validation logic to be performed before SRCU read lock.
> 
> Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>  drivers/infiniband/core/uverbs_main.c | 90 ++++++++++++++++++-----------------
>  1 file changed, 47 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index f8f2182ab86b..e07326139ce9 100644
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -677,6 +677,42 @@ static ssize_t process_hdr(struct ib_uverbs_cmd_hdr *hdr,
>  	return 0;
>  }
>  
> +static ssize_t verify_hdr(struct ib_uverbs_cmd_hdr *hdr,
> +			  struct ib_uverbs_ex_cmd_hdr *ex_hdr,
> +			  size_t count, bool extended)
> +{
> +	if (extended) {
> +		count -= sizeof(*hdr) + sizeof(*ex_hdr);
> +
> +		if ((hdr->in_words + ex_hdr->provider_in_words) * 8 != count)
> +			return -EINVAL;
> +
> +		if (ex_hdr->cmd_hdr_reserved)
> +			return -EINVAL;
> +
> +		if (ex_hdr->response) {
> +			if (!hdr->out_words && !ex_hdr->provider_out_words)
> +				return -EINVAL;
> +
> +			if (!access_ok(VERIFY_WRITE,
> +				       u64_to_user_ptr(ex_hdr->response),
> +				       (hdr->out_words + ex_hdr->provider_out_words) * 8))
> +				return -EFAULT;
> +		} else {
> +			if (hdr->out_words || ex_hdr->provider_out_words)
> +				return -EINVAL;
> +		}
> +
> +		return 0;
> +	}
> +
> +	/* not extended command */
> +	if (hdr->in_words * 4 != count)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +

I think you should squish this with the prior two patches

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 11/14] RDMA/uverbs: Refactor the header validation logic

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1956 bytes --]

On Wed, Feb 14, 2018 at 04:46:35PM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 14, 2018 at 02:38:41PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > Move all header validation logic to be performed before SRCU read lock.
> >
> > Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >  drivers/infiniband/core/uverbs_main.c | 90 ++++++++++++++++++-----------------
> >  1 file changed, 47 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> > index f8f2182ab86b..e07326139ce9 100644
> > +++ b/drivers/infiniband/core/uverbs_main.c
> > @@ -677,6 +677,42 @@ static ssize_t process_hdr(struct ib_uverbs_cmd_hdr *hdr,
> >  	return 0;
> >  }
> >
> > +static ssize_t verify_hdr(struct ib_uverbs_cmd_hdr *hdr,
> > +			  struct ib_uverbs_ex_cmd_hdr *ex_hdr,
> > +			  size_t count, bool extended)
> > +{
> > +	if (extended) {
> > +		count -= sizeof(*hdr) + sizeof(*ex_hdr);
> > +
> > +		if ((hdr->in_words + ex_hdr->provider_in_words) * 8 != count)
> > +			return -EINVAL;
> > +
> > +		if (ex_hdr->cmd_hdr_reserved)
> > +			return -EINVAL;
> > +
> > +		if (ex_hdr->response) {
> > +			if (!hdr->out_words && !ex_hdr->provider_out_words)
> > +				return -EINVAL;
> > +
> > +			if (!access_ok(VERIFY_WRITE,
> > +				       u64_to_user_ptr(ex_hdr->response),
> > +				       (hdr->out_words + ex_hdr->provider_out_words) * 8))
> > +				return -EFAULT;
> > +		} else {
> > +			if (hdr->out_words || ex_hdr->provider_out_words)
> > +				return -EINVAL;
> > +		}
> > +
> > +		return 0;
> > +	}
> > +
> > +	/* not extended command */
> > +	if (hdr->in_words * 4 != count)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
>
> I think you should squish this with the prior two patches

I preferred to split patches as much as possible in this area
to minimize chances of errors.

>
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH rdma-next 12/14] RDMA/verbs: Return proper error code for not supported system call

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

The proper return error is -EOPNOTSUPP and not -ENOSYS, so update
all places in verbs.c to match this semantics.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/verbs.c | 46 ++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/infiniband/core/verbs.c b/drivers/infiniband/core/verbs.c
index 16ebc6372c31..95965033cc64 100644
--- a/drivers/infiniband/core/verbs.c
+++ b/drivers/infiniband/core/verbs.c
@@ -655,7 +655,7 @@ int rdma_modify_ah(struct ib_ah *ah, struct rdma_ah_attr *ah_attr)
 
 	return ah->device->modify_ah ?
 		ah->device->modify_ah(ah, ah_attr) :
-		-ENOSYS;
+		-EOPNOTSUPP;
 }
 EXPORT_SYMBOL(rdma_modify_ah);
 
@@ -663,7 +663,7 @@ int rdma_query_ah(struct ib_ah *ah, struct rdma_ah_attr *ah_attr)
 {
 	return ah->device->query_ah ?
 		ah->device->query_ah(ah, ah_attr) :
-		-ENOSYS;
+		-EOPNOTSUPP;
 }
 EXPORT_SYMBOL(rdma_query_ah);
 
@@ -689,7 +689,7 @@ struct ib_srq *ib_create_srq(struct ib_pd *pd,
 	struct ib_srq *srq;
 
 	if (!pd->device->create_srq)
-		return ERR_PTR(-ENOSYS);
+		return ERR_PTR(-EOPNOTSUPP);
 
 	srq = pd->device->create_srq(pd, srq_init_attr, NULL);
 
@@ -722,7 +722,7 @@ int ib_modify_srq(struct ib_srq *srq,
 {
 	return srq->device->modify_srq ?
 		srq->device->modify_srq(srq, srq_attr, srq_attr_mask, NULL) :
-		-ENOSYS;
+		-EOPNOTSUPP;
 }
 EXPORT_SYMBOL(ib_modify_srq);
 
@@ -730,7 +730,7 @@ int ib_query_srq(struct ib_srq *srq,
 		 struct ib_srq_attr *srq_attr)
 {
 	return srq->device->query_srq ?
-		srq->device->query_srq(srq, srq_attr) : -ENOSYS;
+		srq->device->query_srq(srq, srq_attr) : -EOPNOTSUPP;
 }
 EXPORT_SYMBOL(ib_query_srq);
 
@@ -1458,7 +1458,7 @@ int ib_query_qp(struct ib_qp *qp,
 {
 	return qp->device->query_qp ?
 		qp->device->query_qp(qp->real_qp, qp_attr, qp_attr_mask, qp_init_attr) :
-		-ENOSYS;
+		-EOPNOTSUPP;
 }
 EXPORT_SYMBOL(ib_query_qp);
 
@@ -1595,7 +1595,7 @@ EXPORT_SYMBOL(ib_create_cq);
 int rdma_set_cq_moderation(struct ib_cq *cq, u16 cq_count, u16 cq_period)
 {
 	return cq->device->modify_cq ?
-		cq->device->modify_cq(cq, cq_count, cq_period) : -ENOSYS;
+		cq->device->modify_cq(cq, cq_count, cq_period) : -EOPNOTSUPP;
 }
 EXPORT_SYMBOL(rdma_set_cq_moderation);
 
@@ -1612,7 +1612,7 @@ EXPORT_SYMBOL(ib_destroy_cq);
 int ib_resize_cq(struct ib_cq *cq, int cqe)
 {
 	return cq->device->resize_cq ?
-		cq->device->resize_cq(cq, cqe, NULL) : -ENOSYS;
+		cq->device->resize_cq(cq, cqe, NULL) : -EOPNOTSUPP;
 }
 EXPORT_SYMBOL(ib_resize_cq);
 
@@ -1650,7 +1650,7 @@ struct ib_mr *ib_alloc_mr(struct ib_pd *pd,
 	struct ib_mr *mr;
 
 	if (!pd->device->alloc_mr)
-		return ERR_PTR(-ENOSYS);
+		return ERR_PTR(-EOPNOTSUPP);
 
 	mr = pd->device->alloc_mr(pd, mr_type, max_num_sg);
 	if (!IS_ERR(mr)) {
@@ -1674,7 +1674,7 @@ struct ib_fmr *ib_alloc_fmr(struct ib_pd *pd,
 	struct ib_fmr *fmr;
 
 	if (!pd->device->alloc_fmr)
-		return ERR_PTR(-ENOSYS);
+		return ERR_PTR(-EOPNOTSUPP);
 
 	fmr = pd->device->alloc_fmr(pd, mr_access_flags, fmr_attr);
 	if (!IS_ERR(fmr)) {
@@ -1758,7 +1758,7 @@ int ib_attach_mcast(struct ib_qp *qp, union ib_gid *gid, u16 lid)
 	int ret;
 
 	if (!qp->device->attach_mcast)
-		return -ENOSYS;
+		return -EOPNOTSUPP;
 
 	if (!rdma_is_multicast_addr((struct in6_addr *)gid->raw) ||
 	    qp->qp_type != IB_QPT_UD || !is_valid_mcast_lid(qp, lid))
@@ -1776,7 +1776,7 @@ int ib_detach_mcast(struct ib_qp *qp, union ib_gid *gid, u16 lid)
 	int ret;
 
 	if (!qp->device->detach_mcast)
-		return -ENOSYS;
+		return -EOPNOTSUPP;
 
 	if (!rdma_is_multicast_addr((struct in6_addr *)gid->raw) ||
 	    qp->qp_type != IB_QPT_UD || !is_valid_mcast_lid(qp, lid))
@@ -1794,7 +1794,7 @@ struct ib_xrcd *__ib_alloc_xrcd(struct ib_device *device, const char *caller)
 	struct ib_xrcd *xrcd;
 
 	if (!device->alloc_xrcd)
-		return ERR_PTR(-ENOSYS);
+		return ERR_PTR(-EOPNOTSUPP);
 
 	xrcd = device->alloc_xrcd(device, NULL, NULL);
 	if (!IS_ERR(xrcd)) {
@@ -1848,7 +1848,7 @@ struct ib_wq *ib_create_wq(struct ib_pd *pd,
 	struct ib_wq *wq;
 
 	if (!pd->device->create_wq)
-		return ERR_PTR(-ENOSYS);
+		return ERR_PTR(-EOPNOTSUPP);
 
 	wq = pd->device->create_wq(pd, wq_attr, NULL);
 	if (!IS_ERR(wq)) {
@@ -1903,7 +1903,7 @@ int ib_modify_wq(struct ib_wq *wq, struct ib_wq_attr *wq_attr,
 	int err;
 
 	if (!wq->device->modify_wq)
-		return -ENOSYS;
+		return -EOPNOTSUPP;
 
 	err = wq->device->modify_wq(wq, wq_attr, wq_attr_mask, NULL);
 	return err;
@@ -1928,7 +1928,7 @@ struct ib_rwq_ind_table *ib_create_rwq_ind_table(struct ib_device *device,
 	u32 table_size;
 
 	if (!device->create_rwq_ind_table)
-		return ERR_PTR(-ENOSYS);
+		return ERR_PTR(-EOPNOTSUPP);
 
 	table_size = (1 << init_attr->log_ind_tbl_size);
 	rwq_ind_table = device->create_rwq_ind_table(device,
@@ -1978,7 +1978,7 @@ struct ib_flow *ib_create_flow(struct ib_qp *qp,
 {
 	struct ib_flow *flow_id;
 	if (!qp->device->create_flow)
-		return ERR_PTR(-ENOSYS);
+		return ERR_PTR(-EOPNOTSUPP);
 
 	flow_id = qp->device->create_flow(qp, flow_attr, domain);
 	if (!IS_ERR(flow_id)) {
@@ -2005,7 +2005,7 @@ int ib_check_mr_status(struct ib_mr *mr, u32 check_mask,
 		       struct ib_mr_status *mr_status)
 {
 	return mr->device->check_mr_status ?
-		mr->device->check_mr_status(mr, check_mask, mr_status) : -ENOSYS;
+		mr->device->check_mr_status(mr, check_mask, mr_status) : -EOPNOTSUPP;
 }
 EXPORT_SYMBOL(ib_check_mr_status);
 
@@ -2013,7 +2013,7 @@ int ib_set_vf_link_state(struct ib_device *device, int vf, u8 port,
 			 int state)
 {
 	if (!device->set_vf_link_state)
-		return -ENOSYS;
+		return -EOPNOTSUPP;
 
 	return device->set_vf_link_state(device, vf, port, state);
 }
@@ -2023,7 +2023,7 @@ int ib_get_vf_config(struct ib_device *device, int vf, u8 port,
 		     struct ifla_vf_info *info)
 {
 	if (!device->get_vf_config)
-		return -ENOSYS;
+		return -EOPNOTSUPP;
 
 	return device->get_vf_config(device, vf, port, info);
 }
@@ -2033,7 +2033,7 @@ int ib_get_vf_stats(struct ib_device *device, int vf, u8 port,
 		    struct ifla_vf_stats *stats)
 {
 	if (!device->get_vf_stats)
-		return -ENOSYS;
+		return -EOPNOTSUPP;
 
 	return device->get_vf_stats(device, vf, port, stats);
 }
@@ -2043,7 +2043,7 @@ int ib_set_vf_guid(struct ib_device *device, int vf, u8 port, u64 guid,
 		   int type)
 {
 	if (!device->set_vf_guid)
-		return -ENOSYS;
+		return -EOPNOTSUPP;
 
 	return device->set_vf_guid(device, vf, port, guid, type);
 }
@@ -2078,7 +2078,7 @@ int ib_map_mr_sg(struct ib_mr *mr, struct scatterlist *sg, int sg_nents,
 		 unsigned int *sg_offset, unsigned int page_size)
 {
 	if (unlikely(!mr->device->map_mr_sg))
-		return -ENOSYS;
+		return -EOPNOTSUPP;
 
 	mr->page_size = page_size;
 
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH rdma-next 13/14] RDMA/verbs: Check existence of function prior to accessing it

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

Update all the flows to ensure that function pointer exists prior
to accessing it. It will allow to remove uverbs_ex_mask variable
in the next patch.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/core_priv.h  |  3 +++
 drivers/infiniband/core/uverbs_cmd.c | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/infiniband/core/core_priv.h b/drivers/infiniband/core/core_priv.h
index c4560d84dfae..c91f9a80b831 100644
--- a/drivers/infiniband/core/core_priv.h
+++ b/drivers/infiniband/core/core_priv.h
@@ -309,6 +309,9 @@ static inline struct ib_qp *_ib_create_qp(struct ib_device *dev,
 {
 	struct ib_qp *qp;
 
+	if (!dev->create_qp)
+		return ERR_PTR(-EOPNOTSUPP);
+
 	qp = dev->create_qp(pd, attr, udata);
 	if (IS_ERR(qp))
 		return qp;
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 256934d1f64f..2b149e52b355 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -979,6 +979,9 @@ static struct ib_ucq_object *create_cq(struct ib_uverbs_file *file,
 	struct ib_uverbs_ex_create_cq_resp resp;
 	struct ib_cq_init_attr attr = {};
 
+	if (!ib_dev->create_cq)
+		return ERR_PTR(-EOPNOTSUPP);
+
 	if (cmd->comp_vector >= file->device->num_comp_vectors)
 		return ERR_PTR(-EINVAL);
 
@@ -2941,6 +2944,11 @@ int ib_uverbs_ex_create_wq(struct ib_uverbs_file *file,
 		wq_init_attr.create_flags = cmd.create_flags;
 	obj->uevent.events_reported = 0;
 	INIT_LIST_HEAD(&obj->uevent.event_list);
+
+	if (!pd->device->create_wq) {
+		err = -EOPNOTSUPP;
+		goto err_put_cq;
+	}
 	wq = pd->device->create_wq(pd, &wq_init_attr, uhw);
 	if (IS_ERR(wq)) {
 		err = PTR_ERR(wq);
@@ -3084,7 +3092,12 @@ int ib_uverbs_ex_modify_wq(struct ib_uverbs_file *file,
 		wq_attr.flags = cmd.flags;
 		wq_attr.flags_mask = cmd.flags_mask;
 	}
+	if (!wq->device->modify_wq) {
+		ret = -EOPNOTSUPP;
+		goto out;
+	}
 	ret = wq->device->modify_wq(wq, &wq_attr, cmd.attr_mask, uhw);
+out:
 	uobj_put_obj_read(wq);
 	return ret;
 }
@@ -3181,6 +3194,11 @@ int ib_uverbs_ex_create_rwq_ind_table(struct ib_uverbs_file *file,
 
 	init_attr.log_ind_tbl_size = cmd.log_ind_tbl_size;
 	init_attr.ind_tbl = wqs;
+
+	if (!ib_dev->create_rwq_ind_table) {
+		err = -EOPNOTSUPP;
+		goto err_uobj;
+	}
 	rwq_ind_tbl = ib_dev->create_rwq_ind_table(ib_dev, &init_attr, uhw);
 
 	if (IS_ERR(rwq_ind_tbl)) {
@@ -3770,6 +3788,9 @@ int ib_uverbs_ex_query_device(struct ib_uverbs_file *file,
 	struct ib_device_attr attr = {0};
 	int err;
 
+	if (!ib_dev->query_device)
+		return -EOPNOTSUPP;
+
 	if (ucore->inlen < sizeof(cmd))
 		return -EINVAL;
 
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 13/14] RDMA/verbs: Check existence of function prior to accessing it

[Jason Gunthorpe]

On Wed, Feb 14, 2018 at 02:38:43PM +0200, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> 
> Update all the flows to ensure that function pointer exists prior
> to accessing it. It will allow to remove uverbs_ex_mask variable
> in the next patch.
> 
> Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>  drivers/infiniband/core/core_priv.h  |  3 +++
>  drivers/infiniband/core/uverbs_cmd.c | 21 +++++++++++++++++++++
>  2 files changed, 24 insertions(+)

I took this one to for-rc as discussed

Thanks,
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH rdma-next 13/14] RDMA/verbs: Check existence of function prior to accessing it

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 757 bytes --]

On Fri, Feb 16, 2018 at 09:19:56AM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 14, 2018 at 02:38:43PM +0200, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > Update all the flows to ensure that function pointer exists prior
> > to accessing it. It will allow to remove uverbs_ex_mask variable
> > in the next patch.
> >
> > Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >  drivers/infiniband/core/core_priv.h  |  3 +++
> >  drivers/infiniband/core/uverbs_cmd.c | 21 +++++++++++++++++++++
> >  2 files changed, 24 insertions(+)
>
> I took this one to for-rc as discussed

Thanks,

BTW, your mailer mangled the line between my SOB and diffstat.

>
> Thanks,
> Jason

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH rdma-next 14/14] RDMA/verbs: Drop uverbs_ex_mask

[Leon Romanovsky]

From: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>

There is no need to manage mask of available function pointers
while the core is checking that function pointers is set prior
to calling that function, so remove it.

Signed-off-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c |  2 +-
 drivers/infiniband/hw/mlx4/main.c     | 18 ------------------
 drivers/infiniband/hw/mlx5/main.c     | 15 ---------------
 include/rdma/ib_verbs.h               |  1 -
 4 files changed, 1 insertion(+), 35 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index e07326139ce9..00d751121446 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -641,7 +641,7 @@ static bool verify_command_mask(struct ib_device *ib_dev,
 	if (!extended)
 		return ib_dev->uverbs_cmd_mask & BIT_ULL(command);
 
-	return ib_dev->uverbs_ex_cmd_mask & BIT_ULL(command);
+	return true;
 }
 
 static bool verify_command_idx(__u32 command, bool extended)
diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 8d2ee9322f2e..9a083803f81f 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -2759,9 +2759,6 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
 	ibdev->ib_dev.get_dev_fw_str    = get_fw_ver_str;
 	ibdev->ib_dev.disassociate_ucontext = mlx4_ib_disassociate_ucontext;
 
-	ibdev->ib_dev.uverbs_ex_cmd_mask |=
-		(1ull << IB_USER_VERBS_EX_CMD_MODIFY_CQ);
-
 	if ((dev->caps.flags2 & MLX4_DEV_CAP_FLAG2_RSS) &&
 	    ((mlx4_ib_port_link_layer(&ibdev->ib_dev, 1) ==
 	    IB_LINK_LAYER_ETHERNET) ||
@@ -2774,12 +2771,6 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
 			mlx4_ib_create_rwq_ind_table;
 		ibdev->ib_dev.destroy_rwq_ind_table =
 			mlx4_ib_destroy_rwq_ind_table;
-		ibdev->ib_dev.uverbs_ex_cmd_mask |=
-			(1ull << IB_USER_VERBS_EX_CMD_CREATE_WQ)	  |
-			(1ull << IB_USER_VERBS_EX_CMD_MODIFY_WQ)	  |
-			(1ull << IB_USER_VERBS_EX_CMD_DESTROY_WQ)	  |
-			(1ull << IB_USER_VERBS_EX_CMD_CREATE_RWQ_IND_TBL) |
-			(1ull << IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL);
 	}
 
 	if (!mlx4_is_slave(ibdev->dev)) {
@@ -2811,17 +2802,8 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
 		ibdev->steering_support = MLX4_STEERING_MODE_DEVICE_MANAGED;
 		ibdev->ib_dev.create_flow	= mlx4_ib_create_flow;
 		ibdev->ib_dev.destroy_flow	= mlx4_ib_destroy_flow;
-
-		ibdev->ib_dev.uverbs_ex_cmd_mask	|=
-			(1ull << IB_USER_VERBS_EX_CMD_CREATE_FLOW) |
-			(1ull << IB_USER_VERBS_EX_CMD_DESTROY_FLOW);
 	}
 
-	ibdev->ib_dev.uverbs_ex_cmd_mask |=
-		(1ull << IB_USER_VERBS_EX_CMD_QUERY_DEVICE) |
-		(1ull << IB_USER_VERBS_EX_CMD_CREATE_CQ) |
-		(1ull << IB_USER_VERBS_EX_CMD_CREATE_QP);
-
 	mlx4_ib_alloc_eqs(dev, ibdev);
 
 	spin_lock_init(&iboe->lock);
diff --git a/drivers/infiniband/hw/mlx5/main.c b/drivers/infiniband/hw/mlx5/main.c
index 4236c8086820..7074c418e2c9 100644
--- a/drivers/infiniband/hw/mlx5/main.c
+++ b/drivers/infiniband/hw/mlx5/main.c
@@ -4618,12 +4618,6 @@ static int mlx5_ib_stage_caps_init(struct mlx5_ib_dev *dev)
 		(1ull << IB_USER_VERBS_CMD_DESTROY_SRQ)		|
 		(1ull << IB_USER_VERBS_CMD_CREATE_XSRQ)		|
 		(1ull << IB_USER_VERBS_CMD_OPEN_QP);
-	dev->ib_dev.uverbs_ex_cmd_mask =
-		(1ull << IB_USER_VERBS_EX_CMD_QUERY_DEVICE)	|
-		(1ull << IB_USER_VERBS_EX_CMD_CREATE_CQ)	|
-		(1ull << IB_USER_VERBS_EX_CMD_CREATE_QP)	|
-		(1ull << IB_USER_VERBS_EX_CMD_MODIFY_QP)	|
-		(1ull << IB_USER_VERBS_EX_CMD_MODIFY_CQ);
 
 	dev->ib_dev.query_device	= mlx5_ib_query_device;
 	dev->ib_dev.query_port		= mlx5_ib_query_port;
@@ -4704,9 +4698,6 @@ static int mlx5_ib_stage_caps_init(struct mlx5_ib_dev *dev)
 
 	dev->ib_dev.create_flow	= mlx5_ib_create_flow;
 	dev->ib_dev.destroy_flow = mlx5_ib_destroy_flow;
-	dev->ib_dev.uverbs_ex_cmd_mask |=
-			(1ull << IB_USER_VERBS_EX_CMD_CREATE_FLOW) |
-			(1ull << IB_USER_VERBS_EX_CMD_DESTROY_FLOW);
 
 	err = init_node_data(dev);
 	if (err)
@@ -4746,12 +4737,6 @@ static int mlx5_ib_stage_roce_init(struct mlx5_ib_dev *dev)
 		dev->ib_dev.destroy_wq	 = mlx5_ib_destroy_wq;
 		dev->ib_dev.create_rwq_ind_table = mlx5_ib_create_rwq_ind_table;
 		dev->ib_dev.destroy_rwq_ind_table = mlx5_ib_destroy_rwq_ind_table;
-		dev->ib_dev.uverbs_ex_cmd_mask |=
-			(1ull << IB_USER_VERBS_EX_CMD_CREATE_WQ) |
-			(1ull << IB_USER_VERBS_EX_CMD_MODIFY_WQ) |
-			(1ull << IB_USER_VERBS_EX_CMD_DESTROY_WQ) |
-			(1ull << IB_USER_VERBS_EX_CMD_CREATE_RWQ_IND_TBL) |
-			(1ull << IB_USER_VERBS_EX_CMD_DESTROY_RWQ_IND_TBL);
 		err = mlx5_enable_eth(dev, port_num);
 		if (err)
 			return err;
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 73b2387e3f74..f31ba8df5c66 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -2346,7 +2346,6 @@ struct ib_device {
 
 	int			     uverbs_abi_ver;
 	u64			     uverbs_cmd_mask;
-	u64			     uverbs_ex_cmd_mask;
 
 	char			     node_desc[IB_DEVICE_NODE_DESC_MAX];
 	__be64			     node_guid;
-- 
2.16.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html