[PATCH ib-next 0/3] IB core uverbs support for leagacy commands

[PATCH ib-next 0/3] IB core uverbs support for leagacy commands

[Eli Cohen]

Hi Doug,

this patcheset is addresses comments from both Jason and Yann.

Eli

Eli Cohen (3):
  IB/core: Avoid duplicate code
  IB/core: IB/core: Allow legacy verbs through extended interfaces
  IB/core: Modify conditional on ucontext existence

 drivers/infiniband/core/uverbs_main.c | 70 +++++++++++++++++------------------
 1 file changed, 34 insertions(+), 36 deletions(-)

-- 
2.6.2

--
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 ib-next 1/3] IB/core: Avoid duplicate code

[Eli Cohen]

Move the check on the validity of the command to a common area.

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

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index e3ef28861be6..e93ba9125198 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;
@@ -696,20 +697,18 @@ 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;
 
 	if (!flags) {
-		__u32 command;
-
-		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_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;
-- 
2.6.2

--
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 ib-next 2/3] IB/core: 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.

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

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index e93ba9125198..1740a03e6ac6 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -672,6 +672,21 @@ out:
 	return ev_file;
 }
 
+static int verify_command_mask(struct ib_device *ib_dev, __u32 command)
+{
+	u64 mask;
+
+	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 0;
+
+	return -1;
+}
+
 static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 			     size_t count, loff_t *pos)
 {
@@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 	}
 
 	command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
+	if (verify_command_mask(ib_dev, command)) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	flags = (hdr.command &
 		 IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
@@ -721,11 +740,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 			goto out;
 		}
 
-		if (!(ib_dev->uverbs_cmd_mask & (1ull << command))) {
-			ret = -ENOSYS;
-			goto out;
-		}
-
 		if (hdr.in_words * 4 != count) {
 			ret = -EINVAL;
 			goto out;
@@ -753,11 +767,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 			goto out;
 		}
 
-		if (!(ib_dev->uverbs_ex_cmd_mask & (1ull << command))) {
-			ret = -ENOSYS;
-			goto out;
-		}
-
 		if (count < (sizeof(hdr) + sizeof(ex_hdr))) {
 			ret = -EINVAL;
 			goto out;
-- 
2.6.2

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

[Matan Barak]

On Thu, Nov 5, 2015 at 7:40 PM, Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> 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.
>
> Signed-off-by: Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/infiniband/core/uverbs_main.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index e93ba9125198..1740a03e6ac6 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -672,6 +672,21 @@ out:
>         return ev_file;
>  }
>
> +static int verify_command_mask(struct ib_device *ib_dev, __u32 command)
> +{
> +       u64 mask;
> +
> +       if (command <= IB_USER_VERBS_CMD_OPEN_QP)

I think using IB_USER_VERBS_CMD_THRESHOLD is clearer, but I don't
think we need two uverbs_cmd_mask vendor variables.
IMHO, a vendor shouldn't care if it's an extended/legacy uverb
command. Maybe we should replace uverbs_cmd_mask with a bitmap.

> +               mask = ib_dev->uverbs_cmd_mask;
> +       else
> +               mask = ib_dev->uverbs_ex_cmd_mask;
> +
> +       if (mask & ((u64)1 << command))
> +               return 0;
> +
> +       return -1;
> +}
> +
>  static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>                              size_t count, loff_t *pos)
>  {
> @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>         }
>
>         command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
> +       if (verify_command_mask(ib_dev, command)) {
> +               ret = -EINVAL;

Why did you replace ENOSYS with EINVAL?

> +               goto out;
> +       }
>
>         flags = (hdr.command &
>                  IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
> @@ -721,11 +740,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>                         goto out;
>                 }
>
> -               if (!(ib_dev->uverbs_cmd_mask & (1ull << command))) {
> -                       ret = -ENOSYS;
> -                       goto out;
> -               }
> -
>                 if (hdr.in_words * 4 != count) {
>                         ret = -EINVAL;
>                         goto out;
> @@ -753,11 +767,6 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>                         goto out;
>                 }
>
> -               if (!(ib_dev->uverbs_ex_cmd_mask & (1ull << command))) {
> -                       ret = -ENOSYS;
> -                       goto out;
> -               }
> -
>                 if (count < (sizeof(hdr) + sizeof(ex_hdr))) {
>                         ret = -EINVAL;
>                         goto out;
> --
> 2.6.2
>
> --
> 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

Re: [PATCH ib-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces

[Haggai Eran]

On 08/11/2015 17:04, Matan Barak wrote:
>> @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>> >         }
>> >
>> >         command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>> > +       if (verify_command_mask(ib_dev, command)) {
>> > +               ret = -EINVAL;
> Why did you replace ENOSYS with EINVAL?

I think ENOSYS is meant only to represent a system call number that
isn't supported. There was a change in checkpatch that now warns about
it [1]. I'm not sure the intention was to fix existing uses though.

Haggai

[1] http://www.gossamer-threads.com/lists/linux/kernel/2148343

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

[Jason Gunthorpe]

On Mon, Nov 09, 2015 at 08:48:36AM +0200, Haggai Eran wrote:
> On 08/11/2015 17:04, Matan Barak wrote:
> >> @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> >> >         }
> >> >
> >> >         command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
> >> > +       if (verify_command_mask(ib_dev, command)) {
> >> > +               ret = -EINVAL;
> > Why did you replace ENOSYS with EINVAL?
> 
> I think ENOSYS is meant only to represent a system call number that
> isn't supported. There was a change in checkpatch that now warns about
> it [1]. I'm not sure the intention was to fix existing uses though.

Within verbs we should have two kinds of return - not supported
request, and supported request with invalid parameters.

Maybe use EOPNOTSUPP ?

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

[Eli Cohen]

On Mon, Nov 09, 2015 at 03:35:31PM -0700, Jason Gunthorpe wrote:
> On Mon, Nov 09, 2015 at 08:48:36AM +0200, Haggai Eran wrote:
> > On 08/11/2015 17:04, Matan Barak wrote:
> > >> @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> > >> >         }
> > >> >
> > >> >         command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
> > >> > +       if (verify_command_mask(ib_dev, command)) {
> > >> > +               ret = -EINVAL;
> > > Why did you replace ENOSYS with EINVAL?
> > 
> > I think ENOSYS is meant only to represent a system call number that
> > isn't supported. There was a change in checkpatch that now warns about
> > it [1]. I'm not sure the intention was to fix existing uses though.
> 
> Within verbs we should have two kinds of return - not supported
> request, and supported request with invalid parameters.
> 
> Maybe use EOPNOTSUPP ?
> 

What about Matan's concern regarding legacy code? Maybe we should
leave ENOSYS as that's how it is all over the IB stack code.

quote:
I think it could break old applications:
err = extended_verb(the_first_extension_we_added);
if (err == ENOSYS)
      err = legacy_verb();
if (err)
    return err;

Such applications used the first extension (that was added during the
addition of the extended verb) and when they realized it's not
supported, they dropped to the legacy verb. This change can now cause
the return of -EINVAL an early termination with an error.

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

[Jason Gunthorpe]

On Tue, Nov 10, 2015 at 01:05:31AM +0200, Eli Cohen wrote:
> On Mon, Nov 09, 2015 at 03:35:31PM -0700, Jason Gunthorpe wrote:
> > On Mon, Nov 09, 2015 at 08:48:36AM +0200, Haggai Eran wrote:
> > > On 08/11/2015 17:04, Matan Barak wrote:
> > > >> @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> > > >> >         }
> > > >> >
> > > >> >         command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
> > > >> > +       if (verify_command_mask(ib_dev, command)) {
> > > >> > +               ret = -EINVAL;
> > > > Why did you replace ENOSYS with EINVAL?
> > > 
> > > I think ENOSYS is meant only to represent a system call number that
> > > isn't supported. There was a change in checkpatch that now warns about
> > > it [1]. I'm not sure the intention was to fix existing uses though.
> > 
> > Within verbs we should have two kinds of return - not supported
> > request, and supported request with invalid parameters.
> > 
> > Maybe use EOPNOTSUPP ?
> > 
> 
> What about Matan's concern regarding legacy code? Maybe we should
> leave ENOSYS as that's how it is all over the IB stack code.
> 
> quote:
> I think it could break old applications:
> err = extended_verb(the_first_extension_we_added);
> if (err == ENOSYS)
>       err = legacy_verb();
> if (err)
>     return err;
> 
> Such applications used the first extension (that was added during the
> addition of the extended verb) and when they realized it's not
> supported, they dropped to the legacy verb. This change can now cause
> the return of -EINVAL an early termination with an error.

Since the change is to make the kernel do the above fall back
internally, this specific example doesn't make alot of sense to worry
about. Ie the extended verb won't fail anymore, and if it does the
legacy one won't work anyhow.

But if there is something out there that does care about ENOSYS we
should try to keep it, but don't convert ENOSYS to EINVAL.

Also, when the driver tests the ex flags for support it should be
returning EOPNOTSUPP or such not EINVAL.. Return codes for the ex
stuff could stand a good sanity audit.

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

[Eli Cohen]

On Mon, Nov 09, 2015 at 04:15:42PM -0700, Jason Gunthorpe wrote:
> 
> Since the change is to make the kernel do the above fall back
> internally, this specific example doesn't make alot of sense to worry
> about. Ie the extended verb won't fail anymore, and if it does the
> legacy one won't work anyhow.
> 

Makes sense.

> But if there is something out there that does care about ENOSYS we
> should try to keep it, but don't convert ENOSYS to EINVAL.
> 
> Also, when the driver tests the ex flags for support it should be
> returning EOPNOTSUPP or such not EINVAL.. Return codes for the ex
> stuff could stand a good sanity audit.
> 

#define EOPNOTSUPP      95      /* Operation not supported on
transport endpoint */

This does not seem like an ideal choise either. I think ENOSYS in this
case is a better choise.
--
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-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces

[Jason Gunthorpe]

On Tue, Nov 10, 2015 at 01:24:31AM +0200, Eli Cohen wrote:
> > Also, when the driver tests the ex flags for support it should be
> > returning EOPNOTSUPP or such not EINVAL.. Return codes for the ex
> > stuff could stand a good sanity audit.
> > 
> 
> #define EOPNOTSUPP      95      /* Operation not supported on
> transport endpoint */
> 
> This does not seem like an ideal choise either. I think ENOSYS in this
> case is a better choise.

Call it ENOTSUP then:

       ENOTSUP         Operation not supported (POSIX.1)

Same value on Linux.

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

[Eli Cohen]

On Mon, Nov 09, 2015 at 04:30:59PM -0700, Jason Gunthorpe wrote:
> On Tue, Nov 10, 2015 at 01:24:31AM +0200, Eli Cohen wrote:
> > > Also, when the driver tests the ex flags for support it should be
> > > returning EOPNOTSUPP or such not EINVAL.. Return codes for the ex
> > > stuff could stand a good sanity audit.
> > > 
> > 
> > #define EOPNOTSUPP      95      /* Operation not supported on
> > transport endpoint */
> > 
> > This does not seem like an ideal choise either. I think ENOSYS in this
> > case is a better choise.
> 
> Call it ENOTSUP then:
> 
>        ENOTSUP         Operation not supported (POSIX.1)
> 
> Same value on Linux.
> 

Yes, that's better. I will resend.
--
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-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces

[Eli Cohen]

On Tue, Nov 10, 2015 at 05:23:10PM +0200, Eli Cohen wrote:
> > 
> > Call it ENOTSUP then:
> > 
> >        ENOTSUP         Operation not supported (POSIX.1)
> > 
> > Same value on Linux.
> > 
> 
> Yes, that's better. I will resend.


Well it seems like ENOTSUP is defined only here:

./arch/parisc/include/uapi/asm/errno.h:115:#define ENOTSUP 252     /* Function not implemented (POSIX.4 / HPUX) */

Which obviously I cannot use. Jason, do you have another idea?
--
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-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces

[Jason Gunthorpe]

On Tue, Nov 10, 2015 at 05:40:26PM +0200, Eli Cohen wrote:
> On Tue, Nov 10, 2015 at 05:23:10PM +0200, Eli Cohen wrote:
> > > 
> > > Call it ENOTSUP then:
> > > 
> > >        ENOTSUP         Operation not supported (POSIX.1)
> > > 
> > > Same value on Linux.
> > 
> > Yes, that's better. I will resend.
> 
> 
> Well it seems like ENOTSUP is defined only here:
> 
> ./arch/parisc/include/uapi/asm/errno.h:115:#define ENOTSUP 252     /* Function not implemented (POSIX.4 / HPUX) */

> Which obviously I cannot use. Jason, do you have another idea?

I would stick with EOPNOTSUPP in the kernel and userspace can view
that as ENOTSUP. My remark was more along the lines that EOPNOTSUPP is
not socket specific because it encompasses ENOTSUP as well on Linux,
due to having the same value.

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

[Bart Van Assche]

On 11/10/2015 07:40 AM, Eli Cohen wrote:
> On Tue, Nov 10, 2015 at 05:23:10PM +0200, Eli Cohen wrote:
>>>
>>> Call it ENOTSUP then:
>>>
>>>         ENOTSUP         Operation not supported (POSIX.1)
>>>
>>> Same value on Linux.
>>>
>>
>> Yes, that's better. I will resend.
> 
> 
> Well it seems like ENOTSUP is defined only here:
> 
> ./arch/parisc/include/uapi/asm/errno.h:115:#define ENOTSUP 252     /* Function not implemented (POSIX.4 / HPUX) */
> 
> Which obviously I cannot use. Jason, do you have another idea?

How about using ENOTSUPP ?

$ PAGER= git grep 'define ENOTSUPP' include
include/linux/errno.h:#define ENOTSUPP 524 /* Operation is not supported */

Bart.
--
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-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces

[Eli Cohen]

On Tue, Nov 10, 2015 at 11:23:35AM -0800, Bart Van Assche wrote:
> 
> How about using ENOTSUPP ?
> 
> $ PAGER= git grep 'define ENOTSUPP' include
> include/linux/errno.h:#define ENOTSUPP 524 /* Operation is not supported */
> 

Appearently this looks a better option but the following appears as a
icomment above this group: /* Defined for the NFSv3 protocol */

I don't mind using this if we can all agree that this is the best
choise.
--
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-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces

[Jason Gunthorpe]

On Tue, Nov 10, 2015 at 10:02:26PM +0200, Eli Cohen wrote:
> On Tue, Nov 10, 2015 at 11:23:35AM -0800, Bart Van Assche wrote:
> > 
> > How about using ENOTSUPP ?
> > 
> > $ PAGER= git grep 'define ENOTSUPP' include
> > include/linux/errno.h:#define ENOTSUPP 524 /* Operation is not supported */
> > 
> 
> Appearently this looks a better option but the following appears as a
> icomment above this group: /* Defined for the NFSv3 protocol */
> 
> I don't mind using this if we can all agree that this is the best
> choise.

We cannot use ENOTSUPP, it is not supported by userspace, there was a
list discussion about that a while back.

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

[Matan Barak]

On Tue, Nov 10, 2015 at 1:15 AM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Tue, Nov 10, 2015 at 01:05:31AM +0200, Eli Cohen wrote:
>> On Mon, Nov 09, 2015 at 03:35:31PM -0700, Jason Gunthorpe wrote:
>> > On Mon, Nov 09, 2015 at 08:48:36AM +0200, Haggai Eran wrote:
>> > > On 08/11/2015 17:04, Matan Barak wrote:
>> > > >> @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>> > > >> >         }
>> > > >> >
>> > > >> >         command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>> > > >> > +       if (verify_command_mask(ib_dev, command)) {
>> > > >> > +               ret = -EINVAL;
>> > > > Why did you replace ENOSYS with EINVAL?
>> > >
>> > > I think ENOSYS is meant only to represent a system call number that
>> > > isn't supported. There was a change in checkpatch that now warns about
>> > > it [1]. I'm not sure the intention was to fix existing uses though.
>> >
>> > Within verbs we should have two kinds of return - not supported
>> > request, and supported request with invalid parameters.
>> >
>> > Maybe use EOPNOTSUPP ?
>> >
>>
>> What about Matan's concern regarding legacy code? Maybe we should
>> leave ENOSYS as that's how it is all over the IB stack code.
>>
>> quote:
>> I think it could break old applications:
>> err = extended_verb(the_first_extension_we_added);
>> if (err == ENOSYS)
>>       err = legacy_verb();
>> if (err)
>>     return err;
>>
>> Such applications used the first extension (that was added during the
>> addition of the extended verb) and when they realized it's not
>> supported, they dropped to the legacy verb. This change can now cause
>> the return of -EINVAL an early termination with an error.
>
> Since the change is to make the kernel do the above fall back
> internally, this specific example doesn't make alot of sense to worry
> about. Ie the extended verb won't fail anymore, and if it does the
> legacy one won't work anyhow.
>

The kernel will do the above fallback if the command is a legacy
command wrapped in an extended command (i.e - no extra flags).
If an application uses one of the extended values, and fall back to
legacy verb on ENOSYS, it'll behave differently after this change:
Re-posting this example:

err = extended_verb(*the_first_extension_we_added*); /* Kernel won't
fall back to legacy verbs here, as we use an actual extended request
*/
if (err == ENOSYS)
      err = legacy_verb();
if (err)
    return err;

> But if there is something out there that does care about ENOSYS we
> should try to keep it, but don't convert ENOSYS to EINVAL.
>
> Also, when the driver tests the ex flags for support it should be
> returning EOPNOTSUPP or such not EINVAL.. Return codes for the ex
> stuff could stand a good sanity audit.
>
> Jason

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

[Jason Gunthorpe]

On Tue, Nov 10, 2015 at 12:25:41PM +0200, Matan Barak wrote:
> The kernel will do the above fallback if the command is a legacy
> command wrapped in an extended command (i.e - no extra flags).
> If an application uses one of the extended values, and fall back to
> legacy verb on ENOSYS, it'll behave differently after this change:
> Re-posting this example:

Again, that doesn't seem like a likely case today, the extended verbs
added so far don't strike me as optional, and even so, if someone is
coding that kind of fall back it is incorrect to just look for ENOSYS,
trying to turn on an optional feature could fail for many different
reasons.

If someone is actually doing this, then we can work around it, but
the kernel seems to be a bit more fluid on error returns - probably
because people get them so wrong so often :(

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

[Eli Cohen]

On Sun, Nov 08, 2015 at 05:04:55PM +0200, Matan Barak wrote:
> > +static int verify_command_mask(struct ib_device *ib_dev, __u32 command)
> > +{
> > +       u64 mask;
> > +
> > +       if (command <= IB_USER_VERBS_CMD_OPEN_QP)
> 
> I think using IB_USER_VERBS_CMD_THRESHOLD is clearer, but I don't
> think we need two uverbs_cmd_mask vendor variables.
> IMHO, a vendor shouldn't care if it's an extended/legacy uverb
> command. Maybe we should replace uverbs_cmd_mask with a bitmap.
> 
> > +               mask = ib_dev->uverbs_cmd_mask;
> > +       else
> > +               mask = ib_dev->uverbs_ex_cmd_mask;
> > +
> > +       if (mask & ((u64)1 << command))
> > +               return 0;
> > +
> > +       return -1;
> > +}
> > +

But IB_USER_VERBS_CMD_OPEN_QP is the last legacy verbs and I prefer a
more restrictive approach to avoid potentail issues in the future.

> >  static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> >                              size_t count, loff_t *pos)
> >  {
> > @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
> >         }
> >
> >         command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
> > +       if (verify_command_mask(ib_dev, command)) {
> > +               ret = -EINVAL;
> 
> Why did you replace ENOSYS with EINVAL?
> 

Like Haggai mentioned in the other response, checkpatch issues error
on this claiming that ENOSYS is reserved to unavailable system calls.
Is it applicable only for new implementations I am not sure. I don't
have clear preference for either ENOSYS or EINAVL.
> > +               goto out;
> > +       }
> >
--
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-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces

[Matan Barak]

On Mon, Nov 9, 2015 at 5:55 PM, Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> On Sun, Nov 08, 2015 at 05:04:55PM +0200, Matan Barak wrote:
>> > +static int verify_command_mask(struct ib_device *ib_dev, __u32 command)
>> > +{
>> > +       u64 mask;
>> > +
>> > +       if (command <= IB_USER_VERBS_CMD_OPEN_QP)
>>
>> I think using IB_USER_VERBS_CMD_THRESHOLD is clearer, but I don't
>> think we need two uverbs_cmd_mask vendor variables.
>> IMHO, a vendor shouldn't care if it's an extended/legacy uverb
>> command. Maybe we should replace uverbs_cmd_mask with a bitmap.
>>
>> > +               mask = ib_dev->uverbs_cmd_mask;
>> > +       else
>> > +               mask = ib_dev->uverbs_ex_cmd_mask;
>> > +
>> > +       if (mask & ((u64)1 << command))
>> > +               return 0;
>> > +
>> > +       return -1;
>> > +}
>> > +
>
> But IB_USER_VERBS_CMD_OPEN_QP is the last legacy verbs and I prefer a
> more restrictive approach to avoid potentail issues in the future.
>

So maybe it's worth adding a IB_USER_VERBS_CMD_LAST_LEGACY_VERB.
It looks a bit weird why you explicitly check for IB_USER_VERBS_CMD_OPEN_QP.

>> >  static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>> >                              size_t count, loff_t *pos)
>> >  {
>> > @@ -704,6 +719,10 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>> >         }
>> >
>> >         command = hdr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
>> > +       if (verify_command_mask(ib_dev, command)) {
>> > +               ret = -EINVAL;
>>
>> Why did you replace ENOSYS with EINVAL?
>>
>
> Like Haggai mentioned in the other response, checkpatch issues error
> on this claiming that ENOSYS is reserved to unavailable system calls.
> Is it applicable only for new implementations I am not sure. I don't
> have clear preference for either ENOSYS or EINAVL.

I think it could break old applications:
err = extended_verb(the_first_extension_we_added);
if (err == ENOSYS)
      err = legacy_verb();
if (err)
    return err;

Such applications used the first extension (that was added during the
addition of the extended verb) and when they realized it's not
supported, they dropped to the legacy verb. This change can now cause
the return of -EINVAL an early termination with an error.

>> > +               goto out;
>> > +       }
>> >
--
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-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces

[Eli Cohen]

On Mon, Nov 09, 2015 at 06:11:49PM +0200, Matan Barak wrote:
> >
> > Like Haggai mentioned in the other response, checkpatch issues error
> > on this claiming that ENOSYS is reserved to unavailable system calls.
> > Is it applicable only for new implementations I am not sure. I don't
> > have clear preference for either ENOSYS or EINAVL.
> 
> I think it could break old applications:
> err = extended_verb(the_first_extension_we_added);
> if (err == ENOSYS)
>       err = legacy_verb();
> if (err)
>     return err;

Can you send a pointer to the code where this could happen?

> 
> Such applications used the first extension (that was added during the
> addition of the extended verb) and when they realized it's not
> supported, they dropped to the legacy verb. This change can now cause
> the return of -EINVAL an early termination with an error.
> 
--
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-next 2/3] IB/core: IB/core: Allow legacy verbs through extended interfaces

[Matan Barak]

On Mon, Nov 9, 2015 at 6:25 PM, Eli Cohen <eli-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote:
> On Mon, Nov 09, 2015 at 06:11:49PM +0200, Matan Barak wrote:
>> >
>> > Like Haggai mentioned in the other response, checkpatch issues error
>> > on this claiming that ENOSYS is reserved to unavailable system calls.
>> > Is it applicable only for new implementations I am not sure. I don't
>> > have clear preference for either ENOSYS or EINAVL.
>>
>> I think it could break old applications:
>> err = extended_verb(the_first_extension_we_added);
>> if (err == ENOSYS)
>>       err = legacy_verb();
>> if (err)
>>     return err;
>
> Can you send a pointer to the code where this could happen?
>

This is a hypothetical application code that could break.

>>
>> Such applications used the first extension (that was added during the
>> addition of the extended verb) and when they realized it's not
>> supported, they dropped to the legacy verb. This change can now cause
>> the return of -EINVAL an early termination with an error.
>>
--
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 ib-next 3/3] IB/core: Modify conditional on ucontext existence

[Eli Cohen]

Since we allow to call legacy verbs using their extended counterpart,
the check on ucontext has to move up to a common area in case this verb
is ever extended.

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

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 1740a03e6ac6..1c31324af7e4 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -724,6 +724,12 @@ 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;
+	}
+
 	flags = (hdr.command &
 		 IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
 
@@ -734,12 +740,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 (hdr.in_words * 4 != count) {
 			ret = -EINVAL;
 			goto out;
-- 
2.6.2

--
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-next 0/3] IB core uverbs support for leagacy commands

[Eli Cohen]

On Thu, Nov 05, 2015 at 07:40:05PM +0200, Eli Cohen wrote:
> Hi Doug,
> 
> this patcheset is addresses comments from both Jason and Yann.
> 
> Eli
> 
> Eli Cohen (3):
>   IB/core: Avoid duplicate code
>   IB/core: IB/core: Allow legacy verbs through extended interfaces
>   IB/core: Modify conditional on ucontext existence
> 
>  drivers/infiniband/core/uverbs_main.c | 70 +++++++++++++++++------------------
>  1 file changed, 34 insertions(+), 36 deletions(-)
>

Hi Doug,
 
this patch set has been reviewe the community and we came to an
agreement. When are you planning to apply them. We have pending
fucntionality that we want to push which relies on them.
 
Moreover, there have been patches for libibverbs sent by Eran
Ben-Elisha a while ago which were also sent a while ago. We need those
so I can update libmlx5 as well and have new functionality available.
 
Thanks,
Eli
--
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