[PATCH] IB/core: Allow legacy verbs through extended interfaces

[PATCH] IB/core: Allow legacy verbs through extended interfaces

[Eli Cohen]

When an extended verbs is an extension to a legacy verb, the original
functionality is preserved. Hence we do not require each hardware driver
to set the extended capability. This will allow to use the extended verb
in its simple form with drivers that do not support the extended
capability.

In addition, avoid code duplication by moving sanity checks to a common
area.

Change-Id: Iedba714224fa07b85325c146621c07e0dbf349fb
Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index e3ef28861be6..0ae934d81b04 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -678,6 +678,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	struct ib_uverbs_file *file = filp->private_data;
 	struct ib_device *ib_dev;
 	struct ib_uverbs_cmd_hdr hdr;
+	__u32 command;
 	__u32 flags;
 	int srcu_key;
 	ssize_t ret;
@@ -699,17 +700,15 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	flags = (hdr.command &
 		 IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
 
-	if (!flags) {
-		__u32 command;
-
-		if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
-					   IB_USER_VERBS_CMD_COMMAND_MASK)) {
-			ret = -EINVAL;
-			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;
+	command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
 
+	if (!flags) {
 		if (command >= ARRAY_SIZE(uverbs_cmd_table) ||
 		    !uverbs_cmd_table[command]) {
 			ret = -EINVAL;
@@ -738,21 +737,11 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 						 hdr.out_words * 4);
 
 	} else if (flags == IB_USER_VERBS_CMD_FLAG_EXTENDED) {
-		__u32 command;
-
 		struct ib_uverbs_ex_cmd_hdr ex_hdr;
 		struct ib_udata ucore;
 		struct ib_udata uhw;
 		size_t written_count = count;
 
-		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;
-
 		if (command >= ARRAY_SIZE(uverbs_ex_cmd_table) ||
 		    !uverbs_ex_cmd_table[command]) {
 			ret = -ENOSYS;
@@ -764,7 +753,8 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 			goto out;
 		}
 
-		if (!(ib_dev->uverbs_ex_cmd_mask & (1ull << command))) {
+		if ((command > IB_USER_VERBS_CMD_OPEN_QP) &&
+		    !(ib_dev->uverbs_ex_cmd_mask & (1ull << command))) {
 			ret = -ENOSYS;
 			goto out;
 		}
-- 
2.6.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] IB/core: Allow legacy verbs through extended interfaces

[Yann Droneaud]

Hi,

Le mercredi 04 novembre 2015 à 21:36 +0200, Eli Cohen a écrit :
> When an extended verbs is an extension to a legacy verb, the original
> functionality is preserved. Hence we do not require each hardware 
> driver to set the extended capability. This will allow to use the 
> extended verb in its simple form with drivers that do not support the 
> extended capability.
> 
> In addition, avoid code duplication by moving sanity checks to a
> common
> area.
> 

Those checks were written with the assumption the command and flags
masks could be different between the legacy and the new verb format.

But since it did happen (yet), it's ok to avoid the duplication (I
would have prefer a separate preliminary patch for this).


> Change-Id: Iedba714224fa07b85325c146621c07e0dbf349fb
> Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/infiniband/core/uverbs_main.c | 30 ++++++++++---------------
> -----
>  1 file changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/infiniband/core/uverbs_main.c
> b/drivers/infiniband/core/uverbs_main.c
> index e3ef28861be6..0ae934d81b04 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -678,6 +678,7 @@ static ssize_t ib_uverbs_write(struct file *filp,
> const char __user *buf,
>  	struct ib_uverbs_file *file = filp->private_data;
>  	struct ib_device *ib_dev;
>  	struct ib_uverbs_cmd_hdr hdr;
> +	__u32 command;
>  	__u32 flags;
>  	int srcu_key;
>  	ssize_t ret;
> @@ -699,17 +700,15 @@ static ssize_t ib_uverbs_write(struct file
> *filp, const char __user *buf,
>  	flags = (hdr.command &
>  		 IB_USER_VERBS_CMD_FLAGS_MASK) >>
> IB_USER_VERBS_CMD_FLAGS_SHIFT;
>  
> -	if (!flags) {
> -		__u32 command;
> -
> -		if (hdr.command &
> ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
> -					  
>  IB_USER_VERBS_CMD_COMMAND_MASK)) {
> -			ret = -EINVAL;
> -			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;
> +	command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>  
> +	if (!flags) {
>  		if (command >= ARRAY_SIZE(uverbs_cmd_table) ||
>  		    !uverbs_cmd_table[command]) {
>  			ret = -EINVAL;
> @@ -738,21 +737,11 @@ static ssize_t ib_uverbs_write(struct file
> *filp, const char __user *buf,
>  						 hdr.out_words * 4);
>  
>  	} else if (flags == IB_USER_VERBS_CMD_FLAG_EXTENDED) {
> -		__u32 command;
> -
>  		struct ib_uverbs_ex_cmd_hdr ex_hdr;
>  		struct ib_udata ucore;
>  		struct ib_udata uhw;
>  		size_t written_count = count;
>  
> -		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;
> -
>  		if (command >= ARRAY_SIZE(uverbs_ex_cmd_table) ||
>  		    !uverbs_ex_cmd_table[command]) {
>  			ret = -ENOSYS;
> @@ -764,7 +753,8 @@ static ssize_t ib_uverbs_write(struct file *filp,
> const char __user *buf,
>  			goto out;
>  		}
>  
> -		if (!(ib_dev->uverbs_ex_cmd_mask & (1ull <<
> command))) {
> +		if ((command > IB_USER_VERBS_CMD_OPEN_QP) &&

What about IB_USER_VERBS_CMD_THRESHOLD instead of
 IB_USER_VERBS_CMD_OPEN_QP ?

> +		    !(ib_dev->uverbs_ex_cmd_mask & (1ull <<
> command))) {
>  			ret = -ENOSYS;
>  			goto out;
>  		}


uverbs_ex_cmd_table[] has already been looked up at this point, so that
can only work if uverbs_ex_cmd_table[] is extended to support the
legacy verbs.

Currently there's only two verbs with dual implementation

  IB_USER_VERBS_EX_CMD_QUERY_DEVICE
  IB_USER_VERBS_EX_CMD_CREATE_CQ


If we want to have a 1:1 mapping legacy verbs as extended verbs, then I
would suggest to check the legacy mask to not allow a legacy verb not
supported by the hw provider to be called:

	if (! (((command <= IB_USER_VERBS_CMD_OPEN_QP) ?
		 ib_dev->uverbs_cmd_mask :
		 ib_dev->uverbs_ex_cmd_mask) & (1 ull << command))) {
		ret = -ENOSYS;
		goto out;
	}

Perhaps we could drop uverbs_ex_cmd_mask and set bit for extended verbs
in uverbs_cmd_mask instead. Then, there will be no need to check againt
the command threshold.

Anyway, this is a new assumption that extended verbs will have to be
somewhat retro compatible with the legacy hw provider implementation
used by the legacy verbs they intend to replace: an extended verbs
matching a legacy one will need to be written in such way it will not
be calling into a hw provider function pointer that can be NULL (in
case there's new function pointer is added for an updated verbs in
struct ib_device  while we keep in place the legacy one ... but for in
-kernel drivers that should never happen: it usual better to replace
the previous one by the newer and the existing drivers have to be
updated to provide the new function and remove the previous one, so I
think it's quite safe).

Regards.

-- 
Yann Droneaud
OPTEYA

--
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] IB/core: Allow legacy verbs through extended interfaces

[Eli Cohen]

On Thu, Nov 05, 2015 at 11:41:57AM +0100, Yann Droneaud wrote:
> 
> Those checks were written with the assumption the command and flags
> masks could be different between the legacy and the new verb format.
> 
> But since it did happen (yet), it's ok to avoid the duplication (I
> would have prefer a separate preliminary patch for this).

OK, will send in a different patch.
> 
> 
> 
> What about IB_USER_VERBS_CMD_THRESHOLD instead of
>  IB_USER_VERBS_CMD_OPEN_QP ?
> 
I wanted to be more restrictive here. After all there can't be any
more legacy commands.
> > +		    !(ib_dev->uverbs_ex_cmd_mask & (1ull <<
> > command))) {
> >  			ret = -ENOSYS;
> >  			goto out;
> >  		}
> 
> 
> uverbs_ex_cmd_table[] has already been looked up at this point, so that
> can only work if uverbs_ex_cmd_table[] is extended to support the
> legacy verbs.
> 
> Currently there's only two verbs with dual implementation
> 
>   IB_USER_VERBS_EX_CMD_QUERY_DEVICE
>   IB_USER_VERBS_EX_CMD_CREATE_CQ
> 
> 
> If we want to have a 1:1 mapping legacy verbs as extended verbs, then I
> would suggest to check the legacy mask to not allow a legacy verb not
> supported by the hw provider to be called:
> 
> 	if (! (((command <= IB_USER_VERBS_CMD_OPEN_QP) ?
> 		 ib_dev->uverbs_cmd_mask :
> 		 ib_dev->uverbs_ex_cmd_mask) & (1 ull << command))) {
> 		ret = -ENOSYS;
> 		goto out;
> 	}
> 
Yes this looks even safer. So I will apply this idea and re-send.

> Perhaps we could drop uverbs_ex_cmd_mask and set bit for extended verbs
> in uverbs_cmd_mask instead. Then, there will be no need to check againt
> the command threshold.
> 
> Anyway, this is a new assumption that extended verbs will have to be
> somewhat retro compatible with the legacy hw provider implementation
> used by the legacy verbs they intend to replace: an extended verbs
> matching a legacy one will need to be written in such way it will not
> be calling into a hw provider function pointer that can be NULL (in
> case there's new function pointer is added for an updated verbs in
> struct ib_device  while we keep in place the legacy one ... but for in
> -kernel drivers that should never happen: it usual better to replace
> the previous one by the newer and the existing drivers have to be
> updated to provide the new function and remove the previous one, so I
> think it's quite safe).
> 
> Regards.
> 
> -- 
> Yann Droneaud
> OPTEYA
> 
> --
> 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
--
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