IB/core: batch of fix against create/destroy_flow uverbs for v3.12

In patch "IB/core: clarify overflow/underflow checks on ib_create/destroy_flow", the meaning of the size field was modified to only represent
the size of the flow_spec appended to the flow_attr structure.

The size of the flow_attr structure must be added when allocating memory for the whole flow_attr + flow_specs buffer.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
  drivers/infiniband/core/uverbs_cmd.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

drivers/infiniband/core/uverbs_cmd.c +2 -1

index 63c2700..3b732f6 100644
@@ -2677,7 +2677,8 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
  		return -EINVAL;
 
  	if (cmd.flow_attr.num_of_specs) {
-		kern_flow_attr = kmalloc(cmd.flow_attr.size, GFP_KERNEL);
+		kern_flow_attr = kmalloc(sizeof(*kern_flow_attr) + cmd.flow_attr.size,
+					 GFP_KERNEL);
  		if (!kern_flow_attr)
  			return -ENOMEM;
 

Quoted 7 lines

In patch "IB/core: clarify overflow/underflow checks on ib_create/destroy_flow", the meaning of the size field was modified to only represent
the size of the flow_spec appended to the flow_attr structure.

The size of the flow_attr structure must be added when allocating memory for the whole flow_attr + flow_specs buffer.

wait, patch #2 fixes a problem introduced in patch #1? if this is the case, why not change patch #1?

In patch "IB/core: clarify overflow/underflow checks on ib_create/destroy_flow", the meaning of the size field was modified to only represent
the size of the flow_spec appended to the flow_attr structure.

The size of the flow_attr structure must be added when allocating memory for the whole flow_attr + flow_specs buffer.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=ver.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
  drivers/infiniband/core/uverbs_cmd.c | 5 +++--
  1 file changed, 3 insertions(+), 2 deletions(-)

drivers/infiniband/core/uverbs_cmd.c +3 -3

index 63c2700..3b732f6 100644
@@ -2677,7 +2677,8 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
                 return -EINVAL;
 
         if (cmd.flow_attr.num_of_specs) {
-               kern_flow_attr =malloc(cmd.flow_attr.size, GFP_KERNEL);
+               kern_flow_attr =malloc(sizeof(*kern_flow_attr) + cmd.flow_attr.size,
+                                        GFP_KERNEL);
                 if (!kern_flow_attr)
                         return -ENOMEM;
 

@@ -2705,7 +2706,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
                 goto err_uobj;
         }
 
-       flow_attr =malloc(cmd.flow_attr.size, GFP_KERNEL);
+       flow_attr =malloc(sizeof(*flow_attr) + cmd.flow_attr.size, GFP_KERNEL);
         if (!flow_attr) {
                 err =ENOMEM;
                 goto err_put;
--

1.8.3.1

Flow spec length don't depend on the size of the command
header: they are part of different layers.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=ver.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
  drivers/infiniband/core/uverbs_cmd.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

drivers/infiniband/core/uverbs_cmd.c +2 -3

index 3b732f6..1e5f0dd 100644
@@ -2671,8 +2671,8 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
         if (cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
                 return -EINVAL;
 
-       if (cmd.flow_attr.size > (in_len - sizeof(cmd) - sizeof(struct
-           ib_uverbs_cmd_hdr_ex)) || cmd.flow_attr.size >
+       if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
+           cmd.flow_attr.size >
             (cmd.flow_attr.num_of_specs * sizeof(struct ib_kern_spec)))
                 return -EINVAL;
 
--

1.8.3.1

Quoted 27 lines

Flow spec length don't depend on the size of the command
header: they are part of different layers.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=ver.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
  drivers/infiniband/core/uverbs_cmd.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

drivers/infiniband/core/uverbs_cmd.c +2 -2

index 3b732f6..1e5f0dd 100644
@@ -2671,8 +2671,8 @@ ssize_t ib_uverbs_create_flow(struct 
 ib_uverbs_file *file,
         if (cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
                 return -EINVAL;
 
-       if (cmd.flow_attr.size > (in_len - sizeof(cmd) - sizeof(struct
-           ib_uverbs_cmd_hdr_ex)) || cmd.flow_attr.size >
+       if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
+           cmd.flow_attr.size >
             (cmd.flow_attr.num_of_specs * sizeof(struct 
 ib_kern_spec)))

                 return -EINVAL;

Quoted 3 lines

--
1.8.3.1

Hi,

I agree that it was better if the command size we pass to the uverb didn't include the header size, but since the current implementation of ib_uverbs_write and its user-space counterpart passes the command size including the header - I think this fix is incorrect. I think it'll actually break the current implementation.

Furthermore, patch 8 overwrites this anyway, so I guess we could reduce this patchset size by removing it.

Let's try for inclusion in v3.12-rc6

Quoted 1 line

Let's try for inclusion in v3.12-rc6

Hi everyone. First of sorry for being incommunicado for so long, been swamped with a lot of stuff.

Anyway, given that everyone seems to agree we want something along the lines of this series, but that the series is not ready yet, it really seems to be that at this point the best option is Yann's simple patch to temporarily disable these commands.

Can anyone talk me out of that?

In addition, commands that have extensible sub-structures (for

extended address handle in QP attributes in IP based addressing

should be given as a different UDATA in the cmd structure.

need a comp_mask in the cmd structure header to describe which UDATA structure are included.

Regarding the last patch, you are right that it simplifies things

creating new uverbs where command parts are in-lined one after

but the infrastructure got a bit more complex.

If we're going to this direction, I think the we should also deal

the problem of extending one of the command parts. Currently, we'll

to put a comp_mask in the in-lined command part, consume this

part and then continue with the other parts. It might be better than using a pointer, but this put the burden of serializing the command buffer into the kernel structures onto the uverb command writer. We might want to avoid this.

Furthermore, the comp_mask of the command is different than the comp_mask of the response. Therefore, I don't think we should pass

command's comp_mask to the uverb as a pointer, but just pass a

to value 0 that the uverb will set.

I think that's a good idea to make the comp_mask field a part of our infrastructure. But, I think we should consider making this

If we use the command's comp_mask as an input parameter, shouldn't

use the response's comp_mask as an output parameter ? What's about the provider's comp_mask ?

This infrastructure (as well as the original one) doesn't deal with

case we discussed about in the previous patches. When a command

a structure that can be extended, we don't have a clean nice way of doing that. Though, because it's a limitation of the original design

this patch cleans up the functionality of the original

I'm in favor of merging it.

But, in the mean time, we failed at providing the required fixes/cleanups before v3.12-rc5.
We have not agreed on a definitive, small, urgent patchset for v3.12-rc5. And I think, and it's easy to think so, that Linus will be very upset to see more
than a couple of patches for the Infiniband subsystem for v3.12-rc6. If there's a v3.12-rc6.

So instead of fighting to squeeze the last good patch hunk for v3.12, we should think
on what command/response scheme we need for v3.13.

Quoted 8 lines

But, in the mean time, we failed at providing the required fixes/cleanups before v3.12-rc5.
We have not agreed on a definitive, small, urgent patchset for v3.12-rc5. And I think, and it's easy to think so, that Linus will be very upset to see more
than a couple of patches for the Infiniband subsystem for v3.12-rc6. If there's a v3.12-rc6.

Quoted 3 lines

So instead of fighting to squeeze the last good patch hunk for v3.12, we should think
on what command/response scheme we need for v3.13.

I agree. So as I said before, I think the best idea is just to disable the half-baked command extension stuff before 3.12, and get things right for 3.13.

Do you still think the patch you provided to disable the new stuff is good?

I agree. So as I said before, I think the best idea is just to disable the half-baked command extension stuff before 3.12, and get things right for 3.13.

Roland, after applying a small fix which was sent to you by Matan on Sep 22nd + another tiny fix by Yann to Matan's patch (the two can be surely squashed into one small patch), things are working OK. Yann is suggesting to enhance them to work and look even better and for some reasons (life) he didn't get to do that on the 3-4 months the patches spent on the list and went through review of Sean/Jason/Shawn and you (RD). I don't see how/why you use the term half-baked here.

Quoted 7 lines · Or Gerlitz

Roland, after applying a small fix which was sent to you by Matan on Sep 22nd + another tiny fix by Yann to Matan's patch (the two can be surely squashed into one small patch), things are working OK. Yann is suggesting to enhance them to work and look even better and for some reasons (life) he didn't get to do that on the 3-4 months the patches spent on the list and went through review of Sean/Jason/Shawn and you (RD). I don't see how/why you use the term half-baked here.

Unless I'm misunderstanding, Yann's suggestion for enhancement is not user/kernel ABI compatible with what is in the tree now. Given that we should really try to find an ABI that lasts for a long time, and that we already have a proposal to improve things, shouldn't we hold off on freezing the new ABI for one more kernel cycle?

Quoted 8 lines · Or Gerlitz

Quoted 7 lines

Roland, after applying a small fix which was sent to you by Matan on Sep 22nd + another tiny fix by Yann to Matan's patch (the two can be surely squashed into one small patch), things are working OK. Yann is suggesting to enhance them to work and look even better and for some reasons (life) he didn't get to do that on the 3-4 months the patches spent on the list and went through review of Sean/Jason/Shawn and you (RD). I don't see how/why you use the term half-baked here.

Quoted 7 lines

Unless I'm misunderstanding, Yann's suggestion for enhancement is not user/kernel ABI compatible with what is in the tree now. Given that we should really try to find an ABI that lasts for a long time, and that we already have a proposal to improve things, shouldn't we hold off on freezing the new ABI for one more kernel cycle?

Roland, Yann, the uverbs extensions + flow-steering patches spent many months on the list, once the review started
we did quick fixes for all the comments and made it for 3.12-rc1 which was delay or two kernel cycles vs. what
could happen if the feedback was on time (*).

Indeed since we do want to have ABI that lasts for long, deferring the uverbs support for extensions to 3.13 along with flow-steering support to user space is something we can live with, conditioned on commitment of Yann to quickly submit
his fixed patches that addresses the issues Matan raised and of you (RD) to pick them once Yann and Matan agree
its done into for-next so they are safe for 3.13

Just to double check, by no means we are talking on reverting the whole series, flow-steering will be in
3.12 in the IB core and mlx4_ib

Quoted 3 lines

Just to double check, by no means we are talking on reverting the whole series, flow-steering will be in
3.12 in the IB core and mlx4_ib

I believe it will disable by #if 0 / #endif.
It could be protected by #ifdef CONFIG_INFINIBAND_EXPERIMENTAL_UVERBS_FLOW_STEERING
instead, but I don't know if it's acceptable practice for such piece of code that should be exposed in its current state.

The structure holding any types of flow_spec is of no use to userspace. It would be wrong for userspace to do:

   struct ib_uverbs_flow_spec flow_spec;

   flow_spec.type =B_FLOW_SPEC_TCP;
   flow_spec.size =izeof(flow_spec);

Instead, userspace should use the dedicated flow_spec structure for
- Ethernet : struct ib_uverbs_flow_spec_eth,
- IPv4 : struct ib_uverbs_flow_spec_ipv4,
- TCP/UDP : struct ib_uverbs_flow_spec_tcp_udp.

In other words, struct ib_uverbs_flow_spec is a "virtual" data structure that can only be use by the kernel as an alias to the other.

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=ver.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
  drivers/infiniband/core/uverbs.h  | 16 ++++++++++++++++
  include/uapi/rdma/ib_user_verbs.h | 16 ----------------
  2 files changed, 16 insertions(+), 16 deletions(-)

drivers/infiniband/core/uverbs.h +16 -0

index d040b87..4ae0307 100644
@@ -178,6 +178,22 @@ void ib_uverbs_event_handler(struct ib_event_handler *handler,
                              struct ib_event *event);
  void ib_uverbs_dealloc_xrcd(struct ib_uverbs_device *dev, struct ib_xrcd *xrcd);
 
+struct ib_uverbs_flow_spec {
+       union {
+               union {
+                       struct ib_uverbs_flow_spec_hdr hdr;
+                       struct {
+                               __u32 type;
+                               __u16 size;
+                               __u16 reserved;
+                       };
+               };
+               struct ib_uverbs_flow_spec_eth     eth;
+               struct ib_uverbs_flow_spec_ipv4    ipv4;
+               struct ib_uverbs_flow_spec_tcp_udp tcp_udp;
+       };
+};
+
  #define IB_UVERBS_DECLARE_CMD(name)                                    \
         ssize_t ib_uverbs_##name(struct ib_uverbs_file *file,           \
                                  const char __user *buf, int in_len,    \

include/uapi/rdma/ib_user_verbs.h +0 -17

index f7f233b5..93577fc 100644
@@ -760,22 +760,6 @@ struct ib_uverbs_flow_spec_tcp_udp {
         struct ib_uverbs_flow_tcp_udp_filter mask;
  };
 
-struct ib_uverbs_flow_spec {
-       union {
-               union {
-                       struct ib_uverbs_flow_spec_hdr hdr;
-                       struct {
-                               __u32 type;
-                               __u16 size;
-                               __u16 reserved;
-                       };
-               };
-               struct ib_uverbs_flow_spec_eth      eth;
-               struct ib_uverbs_flow_spec_ipv4    ipv4;
-               struct ib_uverbs_flow_spec_tcp_udp tcp_udp;
-       };
-};
-
  struct ib_uverbs_flow_attr {
         __u32 type;
         __u16 size;
--

1.8.3.1

Commit 400dbc9 added an infrastructure for extensible uverbs commands while later commit 436f2ad exported ib_create_flow()/ib_destroy_flow() functions using this new infrastructure.

According to the commit 400dbc9, the purpose of this infrastructure is to support passing around provider (eg. hardware) specific buffers when userspace issue commands to the kernel, so that it would be possible to extend uverbs (eg. core) buffers independently from the provider buffers.

But the new kernel command function prototypes were not modified to take advantage of this extension. This issue was exposed by Roland Dreier in a previous review[1].

So the following patch is an attempt to a revised extensible command infrastructure.

This improved extensible command infrastructure distinguish between core (eg. legacy)'s command/response buffers from provider (eg. hardware)'s command/response buffers: each extended command implementing function is given a struct ib_udata to hold core (eg. uverbs) input and output buffers, and another struct ib_udata to hold the hw (eg. provider) input and output buffers.
Having those buffers identified separately make it easier to increase one buffer to support extension without having to add some code to guess the exact size of each command/response parts: This should make the extended functions more reliable.

Additionally, instead of relying on command identifier being greater than IB_USER_VERBS_CMD_THRESHOLD, the proposed infrastructure rely on unused bits in command field: on the 32 bits provided by command field, only 6 bits are really needed to encode the identifier of commands currently supported by the kernel. (Even using only 6 bits leaves room for about 23 new commands).

So this patch makes use of some high order bits in command field to store flags, leaving enough room for more command identifiers than one will ever need (eg. 256).

The new flags are used to specify if the command should be processed as an extended one or a legacy one. While designing the new command format, care was taken to make usage of flags itself extensible.

Using high order bits of the commands field ensure that newer libibverbs on older kernel will properly fail when trying to call extended commands. On the other hand, older libibverbs on newer kernel will never be able to issue calls to extended commands.

The extended command header includes the optional response pointer so that output buffer length and output buffer pointer are located together in the command, allowing proper parameters checking. This should make implementing functions easier and safer.

Additionally the extended header ensure 64bits alignment, while making all sizes multiple of 8 bytes, extending the maximum buffer size:

legacy extended

Maximum command buffer: 256KBytes 1024KBytes (512KBytes + 512KBytes)
Maximum response buffer: 256KBytes 1024KBytes (512KBytes + 512KBytes)

For the purpose of doing proper buffer size accounting, the headers size are no more taken in account in "in_words".

One of the odds of the current extensible infrastructure, reading twice the "legacy" command header, is fixed by removing the "legacy" command header from the extended command header: they are processed as two different parts of the command: memory is read once and information are not duplicated:
it's making clear that's an extended command scheme and not a different command scheme.

The proposed scheme will format input (command) and output (response) buffers this way:

- command:

legacy header +
extended header +
command data (core + hw):

     +----------------------------------------+
     | flags     |   00      00    |  command |
     |        in_words    |   out_words       |
     +----------------------------------------+
     |                 response               |
     |                 response               |
     | provider_in_words | provider_out_words |
     |                 padding                |
     +----------------------------------------+
     |                                        |
     .              <uverbs input>            .
     .              (in_words * 8)            .
     |                                        |
     +----------------------------------------+
     |                                        |
     .             <provider input>           .
     .          (provider_in_words * 8)       .
     |                                        |
     +----------------------------------------+

- response, if present:

     +----------------------------------------+
     |                                        |
     .          <uverbs output space>         .
     .             (out_words * 8)            .
     |                                        |
     +----------------------------------------+
     |                                        |
     .         <provider output space>        .
     .         (provider_out_words * 8)       .
     |                                        |
     +----------------------------------------+

The overall design is to ensure that the extensible infrastructure is itself extensible while begin more reliable with more input and bound checking.

[1]:
http://marc.info/?iÊL1RGDWxmM17W2o_era24A-TTDeKyoL6u3NRu_=t_dhV_ZA9MA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Cc Igor Ivanov <[email protected]>

Cc Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=ver.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
  drivers/infiniband/core/uverbs.h      |  18 ++++-
  drivers/infiniband/core/uverbs_cmd.c  |  56 ++++++++--------
  drivers/infiniband/core/uverbs_main.c | 121 ++++++++++++++++++++++++++--------
  drivers/infiniband/hw/mlx4/main.c     |   6 +-
  include/rdma/ib_verbs.h               |   1 +
  include/uapi/rdma/ib_user_verbs.h     |  21 +++---
  6 files changed, 154 insertions(+), 69 deletions(-)

drivers/infiniband/core/uverbs.h +16 -2

index 4ae0307..bdc842e 100644
@@ -47,6 +47,14 @@
  #include <rdma/ib_umem.h>
  #include <rdma/ib_user_verbs.h>
 
+#define INIT_UDATA(udata, ibuf, obuf, ilen, olen)                      \
+       do {                                                            \
+               (udata)->inbuf  =void __user *) (ibuf);               \
+               (udata)->outbuf =void __user *) (obuf);               \
+               (udata)->inlen  =ilen);                               \
+               (udata)->outlen =olen);                               \
+       } while (0)
+
  /*
   * Our lifetime rules for these structs are the following:
   *

@@ -233,7 +241,13 @@ IB_UVERBS_DECLARE_CMD(destroy_srq);
  IB_UVERBS_DECLARE_CMD(create_xsrq);
  IB_UVERBS_DECLARE_CMD(open_xrcd);
  IB_UVERBS_DECLARE_CMD(close_xrcd);
-IB_UVERBS_DECLARE_CMD(create_flow);
-IB_UVERBS_DECLARE_CMD(destroy_flow);
+
+#define IB_UVERBS_DECLARE_EX_CMD(name)                         \
+       int ib_uverbs_ex_##name(struct ib_uverbs_file *file,    \
+                               struct ib_udata *ucore,         \
+                               struct ib_udata *uhw)
+
+IB_UVERBS_DECLARE_EX_CMD(create_flow);
+IB_UVERBS_DECLARE_EX_CMD(destroy_flow);
 
  #endif /* UVERBS_H */

drivers/infiniband/core/uverbs_cmd.c +26 -30

index 052e07c..e35877d 100644
@@ -56,14 +56,6 @@ static struct uverbs_lock_class srq_lock_class       = .name = "SRQ-uobj" };
  static struct uverbs_lock_class xrcd_lock_class = .name = "XRCD-uobj" };
  static struct uverbs_lock_class rule_lock_class = .name = "RULE-uobj" };
 
-#define INIT_UDATA(udata, ibuf, obuf, ilen, olen)                      \
-       do {                                                            \
-               (udata)->inbuf  =void __user *) (ibuf);               \
-               (udata)->outbuf =void __user *) (obuf);               \
-               (udata)->inlen  =ilen);                               \
-               (udata)->outlen =olen);                               \
-       } while (0)
-
  /*
   * The ib_uobject locking scheme is as follows:
   *

@@ -2639,9 +2631,9 @@ static int kern_spec_to_ib_spec(struct ib_uverbs_flow_spec *kern_spec,
         return 0;
  }
 
-ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
-                             const char __user *buf, int in_len,
-                             int out_len)
+int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
+                            struct ib_udata *ucore,
+                            struct ib_udata *uhw)
  {
         struct ib_uverbs_create_flow      cmd;
         struct ib_uverbs_create_flow_resp resp;

@@ -2655,11 +2647,15 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
         void *ib_spec;
         int i;
 
-       if (out_len < sizeof(resp))
+       if (ucore->outlen < sizeof(resp))
                 return -ENOSPC;
 
-       if (copy_from_user(&cmd, buf, sizeof(cmd)))
-               return -EFAULT;
+       err =b_copy_from_udata(&cmd, ucore, sizeof(cmd));
+       if (err)
+               return err;
+
+       ucore->inbuf +=izeof(cmd);
+       ucore->inlen -=izeof(cmd);
 
         if (cmd.comp_mask)
                 return -EINVAL;

@@ -2671,7 +2667,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
         if (cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
                 return -EINVAL;
 
-       if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
+       if (cmd.flow_attr.size > ucore->inlen ||
             cmd.flow_attr.size >
             (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))
                 return -EINVAL;

@@ -2683,11 +2679,10 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
                         return -ENOMEM;
 
                 memcpy(kern_flow_attr, &cmd.flow_attr, sizeof(*kern_flow_attr));
-               if (copy_from_user(kern_flow_attr + 1, buf + sizeof(cmd),
-                                  cmd.flow_attr.size)) {
-                       err =EFAULT;
+               err =b_copy_from_udata(kern_flow_attr + 1, ucore,
+                                        cmd.flow_attr.size);
+               if (err)
                         goto err_free_attr;
-               }
         } else {
                 kern_flow_attr =cmd.flow_attr;
         }

@@ -2755,11 +2750,10 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
         memset(&resp, 0, sizeof(resp));
         resp.flow_handle =obj->id;
 
-       if (copy_to_user((void __user *)(unsigned long) cmd.response,
-                        &resp, sizeof(resp))) {
-               err =EFAULT;
+       err =b_copy_to_udata(ucore,
+                              &resp, sizeof(resp));
+       if (err)
                 goto err_copy;
-       }
 
         put_qp_read(qp);
         mutex_lock(&file->mutex);

@@ -2772,7 +2766,7 @@ ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
         kfree(flow_attr);
         if (cmd.flow_attr.num_of_specs)
                 kfree(kern_flow_attr);
-       return in_len;
+       return 0;
  err_copy:
         idr_remove_uobj(&ib_uverbs_rule_idr, uobj);
  destroy_flow:

@@ -2789,16 +2783,18 @@ err_free_attr:
         return err;
  }
 
-ssize_t ib_uverbs_destroy_flow(struct ib_uverbs_file *file,
-                              const char __user *buf, int in_len,
-                              int out_len) {
+int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
+                             struct ib_udata *ucore,
+                             struct ib_udata *uhw)
+{
         struct ib_uverbs_destroy_flow   cmd;
         struct ib_flow                  *flow_id;
         struct ib_uobject               *uobj;
         int                             ret;
 
-       if (copy_from_user(&cmd, buf, sizeof(cmd)))
-               return -EFAULT;
+       ret =b_copy_from_udata(&cmd, ucore, sizeof(cmd));
+       if (ret)
+               return ret;
 
         uobj =dr_write_uobj(&ib_uverbs_rule_idr, cmd.flow_handle,
                               file->ucontext);

@@ -2820,7 +2816,7 @@ ssize_t ib_uverbs_destroy_flow(struct ib_uverbs_file *file,
 
         put_uobj(uobj);
 
-       return ret ? ret : in_len;
+       return ret ? ret : 0;
  }
 
  static int __uverbs_create_xsrq(struct ib_uverbs_file *file,

drivers/infiniband/core/uverbs_main.c +95 -26

index 75ad86c..4f00654 100644
@@ -115,8 +115,13 @@ static ssize_t (*uverbs_cmd_table[])(struct ib_uverbs_file *file,
         [IB_USER_VERBS_CMD_CLOSE_XRCD]          =b_uverbs_close_xrcd,
         [IB_USER_VERBS_CMD_CREATE_XSRQ]         =b_uverbs_create_xsrq,
         [IB_USER_VERBS_CMD_OPEN_QP]             =b_uverbs_open_qp,
-       [IB_USER_VERBS_CMD_CREATE_FLOW]         =b_uverbs_create_flow,
-       [IB_USER_VERBS_CMD_DESTROY_FLOW]        =b_uverbs_destroy_flow
+};
+
+static int (*uverbs_ex_cmd_table[])(struct ib_uverbs_file *file,
+                                   struct ib_udata *ucore,
+                                   struct ib_udata *uhw) =
+       [IB_USER_VERBS_EX_CMD_CREATE_FLOW]      =b_uverbs_ex_create_flow,
+       [IB_USER_VERBS_EX_CMD_DESTROY_FLOW]     =b_uverbs_ex_destroy_flow
  };
 
  static void ib_uverbs_add_one(struct ib_device *device);

@@ -587,6 +592,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
  {
         struct ib_uverbs_file *file =ilp->private_data;
         struct ib_uverbs_cmd_hdr hdr;
+       __u32 flags;
 
         if (count < sizeof hdr)
                 return -EINVAL;

@@ -594,41 +600,104 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
         if (copy_from_user(&hdr, buf, sizeof hdr))
                 return -EFAULT;
 
-       if (hdr.command >=RRAY_SIZE(uverbs_cmd_table) ||
-           !uverbs_cmd_table[hdr.command])
-               return -EINVAL;
+       flags =hdr.command &
+                IB_USER_VERBS_CMD_FLAGS_MASK) >> IB_USER_VERBS_CMD_FLAGS_SHIFT;
 
-       if (!file->ucontext &&
-           hdr.command !=B_USER_VERBS_CMD_GET_CONTEXT)
-               return -EINVAL;
+       if (!flags) {
+               __u32 command;
 
-       if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull << hdr.command)))
-               return -ENOSYS;
+               if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
+                                          IB_USER_VERBS_CMD_COMMAND_MASK))
+                       return -EINVAL;
 
-       if (hdr.command >=B_USER_VERBS_CMD_THRESHOLD) {
-               struct ib_uverbs_cmd_hdr_ex hdr_ex;
+               command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
 
-               if (copy_from_user(&hdr_ex, buf, sizeof(hdr_ex)))
-                       return -EFAULT;
+               if (command >=RRAY_SIZE(uverbs_cmd_table) ||
+                   !uverbs_cmd_table[command])
+                       return -EINVAL;
 
-               if (((hdr_ex.in_words + hdr_ex.provider_in_words) * 4) !=ount)
+               if (!file->ucontext &&
+                   command !=B_USER_VERBS_CMD_GET_CONTEXT)
                         return -EINVAL;
 
-               return uverbs_cmd_table[hdr.command](file,
-                                                    buf + sizeof(hdr_ex),
-                                                    (hdr_ex.in_words +
-                                                     hdr_ex.provider_in_words) * 4,
-                                                    (hdr_ex.out_words +
-                                                     hdr_ex.provider_out_words) * 4);
-       } else {
+               if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull << command)))
+                       return -ENOSYS;
+
                 if (hdr.in_words * 4 !=ount)
                         return -EINVAL;
 
-               return uverbs_cmd_table[hdr.command](file,
-                                                    buf + sizeof(hdr),
-                                                    hdr.in_words * 4,
-                                                    hdr.out_words * 4);
+               return uverbs_cmd_table[command](file,
+                                                buf + sizeof(hdr),
+                                                hdr.in_words * 4,
+                                                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;
+               int err;
+
+               if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
+                                          IB_USER_VERBS_CMD_COMMAND_MASK))
+                       return -EINVAL;
+
+               command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
+
+               if (command >=RRAY_SIZE(uverbs_ex_cmd_table) ||
+                   !uverbs_ex_cmd_table[command])
+                       return -ENOSYS;
+
+               if (!file->ucontext)
+                       return -EINVAL;
+
+               if (!(file->device->ib_dev->uverbs_ex_cmd_mask & (1ull << command)))
+                       return -ENOSYS;
+
+               if (count < (sizeof(hdr) + sizeof(ex_hdr)))
+                       return -EINVAL;
+
+               if (copy_from_user(&ex_hdr, buf + sizeof(hdr), sizeof(ex_hdr)))
+                       return -EFAULT;
+
+               count -=izeof(hdr) + sizeof(ex_hdr);
+               buf +=izeof(hdr) + sizeof(ex_hdr);
+
+               if ((hdr.in_words + ex_hdr.provider_in_words) * 8 !=ount)
+                       return -EINVAL;
+
+               if (ex_hdr.response) {
+                       if (!hdr.out_words && !ex_hdr.provider_out_words)
+                               return -EINVAL;
+               } else {
+                       if (hdr.out_words || ex_hdr.provider_out_words)
+                               return -EINVAL;
+               }
+
+               INIT_UDATA(&ucore,
+                          (hdr.in_words) ? buf : 0,
+                          (unsigned long)ex_hdr.response,
+                          hdr.in_words * 8,
+                          hdr.out_words * 8);
+
+               INIT_UDATA(&uhw,
+                          (ex_hdr.provider_in_words) ? buf + ucore.inlen : 0,
+                          (ex_hdr.provider_out_words) ? (unsigned long)ex_hdr.response + ucore.outlen : 0,
+                          ex_hdr.provider_in_words * 8,
+                          ex_hdr.provider_out_words * 8);
+
+               err =verbs_ex_cmd_table[command](file,
+                                                  &ucore,
+                                                  &uhw);
+
+               if (err)
+                       return err;
+
+               return count;
         }
+
+       return -ENOSYS;
  }
 
  static int ib_uverbs_mmap(struct file *filp, struct vm_area_struct *vma)

drivers/infiniband/hw/mlx4/main.c +2 -2

index d6c5a73..1aad9b3 100644
@@ -1691,9 +1691,9 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
                 ibdev->ib_dev.create_flow       =lx4_ib_create_flow;
                 ibdev->ib_dev.destroy_flow      =lx4_ib_destroy_flow;
 
-               ibdev->ib_dev.uverbs_cmd_mask   |-                       (1ull << IB_USER_VERBS_CMD_CREATE_FLOW) |
-                       (1ull << IB_USER_VERBS_CMD_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);
         }
 
         mlx4_ib_alloc_eqs(dev, ibdev);

include/rdma/ib_verbs.h +1 -0

index e393171..a06fc12 100644
@@ -1436,6 +1436,7 @@ struct ib_device {
 
         int                          uverbs_abi_ver;
         u64                          uverbs_cmd_mask;
+       u64                          uverbs_ex_cmd_mask;
 
         char                         node_desc[64];
         __be64                       node_guid;

include/uapi/rdma/ib_user_verbs.h +0 -0

index 93577fc..cbfdd4c 100644
@@ -87,8 +87,11 @@ enum {
         IB_USER_VERBS_CMD_CLOSE_XRCD,

include/uapi/rdma/ib_user_verbs.h +8 -7

  /*

@@ -120,16 +123,20 @@ struct ib_uverbs_comp_event_desc {
   * the rest of the command struct based on these value.
   */
 
+#define IB_USER_VERBS_CMD_COMMAND_MASK 0xff
+#define IB_USER_VERBS_CMD_FLAGS_MASK 0xff000000u
+#define IB_USER_VERBS_CMD_FLAGS_SHIFT 24
+
+#define IB_USER_VERBS_CMD_FLAG_EXTENDED 0x80
+
  struct ib_uverbs_cmd_hdr {
         __u32 command;
         __u16 in_words;
         __u16 out_words;
  };
 
-struct ib_uverbs_cmd_hdr_ex {
-       __u32 command;
-       __u16 in_words;
-       __u16 out_words;
+struct ib_uverbs_ex_cmd_hdr {
+       __u64 response;
         __u16 provider_in_words;
         __u16 provider_out_words;
         __u32 cmd_hdr_reserved;

@@ -777,8 +784,6 @@ struct ib_uverbs_flow_attr {
 
  struct ib_uverbs_create_flow  {
         __u32 comp_mask;
-       __u32 reserved;
-       __u64 response;
         __u32 qp_handle;
         struct ib_uverbs_flow_attr flow_attr;
  };
--

1.8.3.1

Quoted 455 lines · Yann Droneaud

Commit 400dbc9 added an infrastructure for extensible uverbs commands while later commit 436f2ad exported ib_create_flow()/ib_destroy_flow()
functions using this new infrastructure.

According to the commit 400dbc9, the purpose of this infrastructure is to support passing around provider (eg. hardware) specific buffers when userspace issue commands to the kernel, so that it would be possible to extend uverbs (eg. core) buffers independently from the provider buffers.

But the new kernel command function prototypes were not modified to take advantage of this extension. This issue was exposed by Roland Dreier in a previous review[1].

So the following patch is an attempt to a revised extensible command infrastructure.

This improved extensible command infrastructure distinguish between core (eg. legacy)'s command/response buffers from provider (eg. hardware)'s
command/response buffers: each extended command implementing function is given a struct ib_udata to hold core (eg. uverbs) input and output buffers,
and another struct ib_udata to hold the hw (eg. provider) input and output
buffers.
Having those buffers identified separately make it easier to increase one buffer to support extension without having to add some code to guess the exact size of each command/response parts: This should make the extended
functions more reliable.

Additionally, instead of relying on command identifier being greater than IB_USER_VERBS_CMD_THRESHOLD, the proposed infrastructure rely on unused bits in command field: on the 32 bits provided by command field, only 6 bits are really needed to encode the identifier of commands currently supported by the kernel. (Even using only 6 bits leaves room for about 23 new commands).

So this patch makes use of some high order bits in command field to store flags, leaving enough room for more command identifiers than one will ever need (eg. 256).

The new flags are used to specify if the command should be processed as an extended one or a legacy one. While designing the new command format,
care was taken to make usage of flags itself extensible.

Using high order bits of the commands field ensure that newer libibverbs on older kernel will properly fail when trying to call extended commands. On the other hand, older libibverbs on newer kernel will never be able to issue calls to extended commands.

The extended command header includes the optional response pointer so that output buffer length and output buffer pointer are located together in the command, allowing proper parameters checking. This should make implementing functions easier and safer.

Additionally the extended header ensure 64bits alignment, while making all sizes multiple of 8 bytes, extending the maximum buffer size:

legacy extended

Maximum command buffer: 256KBytes 1024KBytes (512KBytes +
512KBytes)
Maximum response buffer: 256KBytes 1024KBytes (512KBytes +
512KBytes)

For the purpose of doing proper buffer size accounting, the headers size are no more taken in account in "in_words".

One of the odds of the current extensible infrastructure, reading twice the "legacy" command header, is fixed by removing the "legacy" command header
from the extended command header: they are processed as two different parts
of the command: memory is read once and information are not duplicated:
it's making clear that's an extended command scheme and not a different command scheme.

The proposed scheme will format input (command) and output (response) buffers this way:

- command:

legacy header +
extended header +
command data (core + hw):

     +----------------------------------------+
     | flags     |   00      00    |  command |
     |        in_words    |   out_words       |
     +----------------------------------------+
     |                 response               |
     |                 response               |
     | provider_in_words | provider_out_words |
     |                 padding                |
     +----------------------------------------+
     |                                        |
     .              <uverbs input>            .
     .              (in_words * 8)            .
     |                                        |
     +----------------------------------------+
     |                                        |
     .             <provider input>           .
     .          (provider_in_words * 8)       .
     |                                        |
     +----------------------------------------+

- response, if present:

     +----------------------------------------+
     |                                        |
     .          <uverbs output space>         .
     .             (out_words * 8)            .
     |                                        |
     +----------------------------------------+
     |                                        |
     .         <provider output space>        .
     .         (provider_out_words * 8)       .
     |                                        |
     +----------------------------------------+

The overall design is to ensure that the extensible infrastructure is itself extensible while begin more reliable with more input and bound checking.

[1]:
http://marc.info/?iÊL1RGDWxmM17W2o_era24A-TTDeKyoL6u3NRu_=t_dhV_ZA9MA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Cc Igor Ivanov <[email protected]>

Cc Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=ver.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
  drivers/infiniband/core/uverbs.h      |  18 ++++-
  drivers/infiniband/core/uverbs_cmd.c  |  56 ++++++++--------
  drivers/infiniband/core/uverbs_main.c | 121
++++++++++++++++++++++++++--------
  drivers/infiniband/hw/mlx4/main.c     |   6 +-
  include/rdma/ib_verbs.h               |   1 +
  include/uapi/rdma/ib_user_verbs.h     |  21 +++---
  6 files changed, 154 insertions(+), 69 deletions(-)

drivers/infiniband/core/uverbs.h +16 -2

index 4ae0307..bdc842e 100644
@@ -47,6 +47,14 @@
  #include <rdma/ib_umem.h>
  #include <rdma/ib_user_verbs.h>
 
+#define INIT_UDATA(udata, ibuf, obuf, ilen, olen)                      \
+       do {                                                            \
+               (udata)->inbuf  =void __user *) (ibuf);               \
+               (udata)->outbuf =void __user *) (obuf);               \
+               (udata)->inlen  =ilen);                               \
+               (udata)->outlen =olen);                               \
+       } while (0)
+
  /*
   * Our lifetime rules for these structs are the following:
   *

@@ -233,7 +241,13 @@ IB_UVERBS_DECLARE_CMD(destroy_srq);
  IB_UVERBS_DECLARE_CMD(create_xsrq);
  IB_UVERBS_DECLARE_CMD(open_xrcd);
  IB_UVERBS_DECLARE_CMD(close_xrcd);
-IB_UVERBS_DECLARE_CMD(create_flow);
-IB_UVERBS_DECLARE_CMD(destroy_flow);
+
+#define IB_UVERBS_DECLARE_EX_CMD(name)                         \
+       int ib_uverbs_ex_##name(struct ib_uverbs_file *file,    \
+                               struct ib_udata *ucore,         \
+                               struct ib_udata *uhw)
+
+IB_UVERBS_DECLARE_EX_CMD(create_flow);
+IB_UVERBS_DECLARE_EX_CMD(destroy_flow);
 
  #endif /* UVERBS_H */

drivers/infiniband/core/uverbs_cmd.c +0 -8

index 052e07c..e35877d 100644
@@ -56,14 +56,6 @@ static struct uverbs_lock_class
 srq_lock_class       = .name = "SRQ-uobj" };
  static struct uverbs_lock_class xrcd_lock_class = .name =
 "XRCD-uobj" };
  static struct uverbs_lock_class rule_lock_class = .name =
 "RULE-uobj" };
 
-#define INIT_UDATA(udata, ibuf, obuf, ilen, olen)                      \
-       do {                                                            \
-               (udata)->inbuf  =void __user *) (ibuf);               \
-               (udata)->outbuf =void __user *) (obuf);               \
-               (udata)->inlen  =ilen);                               \
-               (udata)->outlen =olen);                               \
-       } while (0)
-

/*
* The ib_uobject locking scheme is as follows:

   *

drivers/infiniband/core/uverbs_cmd.c +3 -3

@@ -2639,9 +2631,9 @@ static int kern_spec_to_ib_spec(struct
 ib_uverbs_flow_spec *kern_spec,
         return 0;
  }
 
-ssize_t ib_uverbs_create_flow(struct ib_uverbs_file *file,
-                             const char __user *buf, int in_len,
-                             int out_len)
+int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
+                            struct ib_udata *ucore,
+                            struct ib_udata *uhw)
  {
         struct ib_uverbs_create_flow      cmd;

         struct ib_uverbs_create_flow_resp resp;

drivers/infiniband/core/uverbs_cmd.c +7 -3

@@ -2655,11 +2647,15 @@ ssize_t ib_uverbs_create_flow(struct
 ib_uverbs_file *file,
         void *ib_spec;
         int i;
 
-       if (out_len < sizeof(resp))
+       if (ucore->outlen < sizeof(resp))
                 return -ENOSPC;
 
-       if (copy_from_user(&cmd, buf, sizeof(cmd)))
-               return -EFAULT;
+       err =b_copy_from_udata(&cmd, ucore, sizeof(cmd));
+       if (err)
+               return err;
+
+       ucore->inbuf +=izeof(cmd);
+       ucore->inlen -=izeof(cmd);
 
         if (cmd.comp_mask)

                 return -EINVAL;

drivers/infiniband/core/uverbs_cmd.c +1 -1

@@ -2671,7 +2667,7 @@ ssize_t ib_uverbs_create_flow(struct
 ib_uverbs_file *file,
         if (cmd.flow_attr.num_of_specs > IB_FLOW_SPEC_SUPPORT_LAYERS)
                 return -EINVAL;
 
-       if (cmd.flow_attr.size > (in_len - sizeof(cmd)) ||
+       if (cmd.flow_attr.size > ucore->inlen ||
             cmd.flow_attr.size >
             (cmd.flow_attr.num_of_specs * sizeof(struct

ib_uverbs_flow_spec)))

                 return -EINVAL;

drivers/infiniband/core/uverbs_cmd.c +3 -4

@@ -2683,11 +2679,10 @@ ssize_t ib_uverbs_create_flow(struct
 ib_uverbs_file *file,
                         return -ENOMEM;
 
                 memcpy(kern_flow_attr, &cmd.flow_attr,
 sizeof(*kern_flow_attr));
-               if (copy_from_user(kern_flow_attr + 1, buf + sizeof(cmd),
-                                  cmd.flow_attr.size)) {
-                       err =EFAULT;
+               err =b_copy_from_udata(kern_flow_attr + 1, ucore,
+                                        cmd.flow_attr.size);
+               if (err)
                         goto err_free_attr;
-               }
         } else {

                 kern_flow_attr =cmd.flow_attr;
         }

drivers/infiniband/core/uverbs_cmd.c +3 -4

@@ -2755,11 +2750,10 @@ ssize_t ib_uverbs_create_flow(struct
 ib_uverbs_file *file,
         memset(&resp, 0, sizeof(resp));
         resp.flow_handle =obj->id;
 
-       if (copy_to_user((void __user *)(unsigned long) cmd.response,
-                        &resp, sizeof(resp))) {
-               err =EFAULT;
+       err =b_copy_to_udata(ucore,
+                              &resp, sizeof(resp));
+       if (err)
                 goto err_copy;
-       }
 
         put_qp_read(qp);

         mutex_lock(&file->mutex);

drivers/infiniband/core/uverbs_cmd.c +1 -1

@@ -2772,7 +2766,7 @@ ssize_t ib_uverbs_create_flow(struct
 ib_uverbs_file *file,
         kfree(flow_attr);
         if (cmd.flow_attr.num_of_specs)
                 kfree(kern_flow_attr);
-       return in_len;
+       return 0;
  err_copy:
         idr_remove_uobj(&ib_uverbs_rule_idr, uobj);

destroy_flow:

drivers/infiniband/core/uverbs_cmd.c +8 -6

@@ -2789,16 +2783,18 @@ err_free_attr:
         return err;
  }
 
-ssize_t ib_uverbs_destroy_flow(struct ib_uverbs_file *file,
-                              const char __user *buf, int in_len,
-                              int out_len) {
+int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
+                             struct ib_udata *ucore,
+                             struct ib_udata *uhw)
+{
         struct ib_uverbs_destroy_flow   cmd;
         struct ib_flow                  *flow_id;
         struct ib_uobject               *uobj;
         int                             ret;
 
-       if (copy_from_user(&cmd, buf, sizeof(cmd)))
-               return -EFAULT;
+       ret =b_copy_from_udata(&cmd, ucore, sizeof(cmd));
+       if (ret)
+               return ret;
 
         uobj =dr_write_uobj(&ib_uverbs_rule_idr, cmd.flow_handle,
                               file->ucontext);

@@ -2820,7 +2816,7 @@ ssize_t ib_uverbs_destroy_flow(struct
 ib_uverbs_file *file,
 
         put_uobj(uobj);
 
-       return ret ? ret : in_len;
+       return ret ? ret : 0;
  }
 

static int __uverbs_create_xsrq(struct ib_uverbs_file *file,

drivers/infiniband/core/uverbs_main.c +7 -2

index 75ad86c..4f00654 100644
@@ -115,8 +115,13 @@ static ssize_t (*uverbs_cmd_table[])(struct
 ib_uverbs_file *file,
         [IB_USER_VERBS_CMD_CLOSE_XRCD]          =b_uverbs_close_xrcd,
         [IB_USER_VERBS_CMD_CREATE_XSRQ]         =b_uverbs_create_xsrq,
         [IB_USER_VERBS_CMD_OPEN_QP]             =b_uverbs_open_qp,
-       [IB_USER_VERBS_CMD_CREATE_FLOW]         =b_uverbs_create_flow,
-       [IB_USER_VERBS_CMD_DESTROY_FLOW]        =b_uverbs_destroy_flow
+};
+
+static int (*uverbs_ex_cmd_table[])(struct ib_uverbs_file *file,
+                                   struct ib_udata *ucore,
+                                   struct ib_udata *uhw) =
+       [IB_USER_VERBS_EX_CMD_CREATE_FLOW]      =b_uverbs_ex_create_flow,
+       [IB_USER_VERBS_EX_CMD_DESTROY_FLOW]     =b_uverbs_ex_destroy_flow
  };
 

  static void ib_uverbs_add_one(struct ib_device *device);

drivers/infiniband/core/uverbs_main.c +1 -0

@@ -587,6 +592,7 @@ static ssize_t ib_uverbs_write(struct file *filp,
 const char __user *buf,
  {
         struct ib_uverbs_file *file =ilp->private_data;
         struct ib_uverbs_cmd_hdr hdr;
+       __u32 flags;
 
         if (count < sizeof hdr)

                 return -EINVAL;

drivers/infiniband/core/uverbs_main.c +31 -24

@@ -594,41 +600,104 @@ static ssize_t ib_uverbs_write(struct file
 *filp, const char __user *buf,
         if (copy_from_user(&hdr, buf, sizeof hdr))
                 return -EFAULT;
 
-       if (hdr.command >=RRAY_SIZE(uverbs_cmd_table) ||
-           !uverbs_cmd_table[hdr.command])
-               return -EINVAL;
+       flags =hdr.command &
+                IB_USER_VERBS_CMD_FLAGS_MASK) >>
 IB_USER_VERBS_CMD_FLAGS_SHIFT;
 
-       if (!file->ucontext &&
-           hdr.command !=B_USER_VERBS_CMD_GET_CONTEXT)
-               return -EINVAL;
+       if (!flags) {
+               __u32 command;
 
-       if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull <<
 hdr.command)))
-               return -ENOSYS;
+               if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
+
 IB_USER_VERBS_CMD_COMMAND_MASK))
+                       return -EINVAL;
 
-       if (hdr.command >=B_USER_VERBS_CMD_THRESHOLD) {
-               struct ib_uverbs_cmd_hdr_ex hdr_ex;
+               command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
 
-               if (copy_from_user(&hdr_ex, buf, sizeof(hdr_ex)))
-                       return -EFAULT;
+               if (command >=RRAY_SIZE(uverbs_cmd_table) ||
+                   !uverbs_cmd_table[command])
+                       return -EINVAL;
 
-               if (((hdr_ex.in_words + hdr_ex.provider_in_words) * 4)
 !=ount)
+               if (!file->ucontext &&
+                   command !=B_USER_VERBS_CMD_GET_CONTEXT)
                         return -EINVAL;
 
-               return uverbs_cmd_table[hdr.command](file,
-                                                    buf +
 sizeof(hdr_ex),
-                                                    (hdr_ex.in_words +
-
 hdr_ex.provider_in_words) * 4,
-                                                    (hdr_ex.out_words +
-
 hdr_ex.provider_out_words) * 4);
-       } else {
+               if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull <<
 command)))
+                       return -ENOSYS;
+
                 if (hdr.in_words * 4 !=ount)
                         return -EINVAL;
 
-               return uverbs_cmd_table[hdr.command](file,
-                                                    buf + sizeof(hdr),
-                                                    hdr.in_words * 4,
-                                                    hdr.out_words * 4);
+               return uverbs_cmd_table[command](file,
+                                                buf + sizeof(hdr),
+                                                hdr.in_words * 4,
+                                                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;
+               int err;
+
+               if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
+

IB_USER_VERBS_CMD_COMMAND_MASK))

drivers/infiniband/core/uverbs_main.c +12 -0

+                       return -EINVAL;
+
+               command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
+
+               if (command >=RRAY_SIZE(uverbs_ex_cmd_table) ||
+                   !uverbs_ex_cmd_table[command])
+                       return -ENOSYS;
+
+               if (!file->ucontext)
+                       return -EINVAL;
+
+               if (!(file->device->ib_dev->uverbs_ex_cmd_mask & (1ull

<< command)))

drivers/infiniband/core/uverbs_main.c +2 -0

+                       return -ENOSYS;
+

drivers/infiniband/core/uverbs_main.c static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,

Quoted 8 lines

drivers/infiniband/core/uverbs_main.c +4 -0

+               if (count < (sizeof(hdr) + sizeof(ex_hdr)))
+                       return -EINVAL;
+
+               if (copy_from_user(&ex_hdr, buf + sizeof(hdr),

sizeof(ex_hdr)))

drivers/infiniband/core/uverbs_main.c +3 -0

+                       return -EFAULT;
+
+               count -=izeof(hdr) + sizeof(ex_hdr);

Quoted 27 lines

drivers/infiniband/core/uverbs_main.c +21 -0

+               buf +=izeof(hdr) + sizeof(ex_hdr);
+
+               if ((hdr.in_words + ex_hdr.provider_in_words) * 8 !=ount)
+                       return -EINVAL;
+
+               if (ex_hdr.response) {
+                       if (!hdr.out_words && !ex_hdr.provider_out_words)
+                               return -EINVAL;
+               } else {
+                       if (hdr.out_words || ex_hdr.provider_out_words)
+                               return -EINVAL;
+               }
+
+               INIT_UDATA(&ucore,
+                          (hdr.in_words) ? buf : 0,
+                          (unsigned long)ex_hdr.response,
+                          hdr.in_words * 8,
+                          hdr.out_words * 8);
+
+               INIT_UDATA(&uhw,
+                          (ex_hdr.provider_in_words) ? buf +

ucore.inlen : 0,
+ (ex_hdr.provider_out_words) ? (unsigned
long)ex_hdr.response + ucore.outlen : 0,

drivers/infiniband/core/uverbs_main.c +3 -0

+                          ex_hdr.provider_in_words * 8,
+                          ex_hdr.provider_out_words * 8);
+

drivers/infiniband/core/uverbs_main.c static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf, · line 697

Quoted 8 lines

drivers/infiniband/core/uverbs_main.c +8 -0

+               err =verbs_ex_cmd_table[command](file,
+                                                  &ucore,
+                                                  &uhw);
+
+               if (err)
+                       return err;
+
+               return count;

Quoted 44 lines

         }

drivers/infiniband/core/uverbs_main.c +2 -0

+
+       return -ENOSYS;
  }
 
  static int ib_uverbs_mmap(struct file *filp, struct vm_area_struct

*vma)

drivers/infiniband/hw/mlx4/main.c +0 -1

index d6c5a73..1aad9b3 100644
@@ -1691,9 +1691,9 @@ static void *mlx4_ib_add(struct mlx4_dev *dev)
                 ibdev->ib_dev.create_flow       =lx4_ib_create_flow;
                 ibdev->ib_dev.destroy_flow      =lx4_ib_destroy_flow;
 
-               ibdev->ib_dev.uverbs_cmd_mask

|- (1ull << IB_USER_VERBS_CMD_CREATE_FLOW) |

drivers/infiniband/hw/mlx4/main.c +1 -1

-                       (1ull << IB_USER_VERBS_CMD_DESTROY_FLOW);
+               ibdev->ib_dev.uverbs_ex_cmd_mask

|+ (1ull << IB_USER_VERBS_EX_CMD_CREATE_FLOW) |

drivers/infiniband/hw/mlx4/main.c +1 -0

+                       (1ull << IB_USER_VERBS_EX_CMD_DESTROY_FLOW);
         }
 
         mlx4_ib_alloc_eqs(dev, ibdev);

include/rdma/ib_verbs.h +1 -0

index e393171..a06fc12 100644
@@ -1436,6 +1436,7 @@ struct ib_device {
 
         int                          uverbs_abi_ver;
         u64                          uverbs_cmd_mask;
+       u64                          uverbs_ex_cmd_mask;
 
         char                         node_desc[64];
         __be64                       node_guid;

include/uapi/rdma/ib_user_verbs.h +0 -0

index 93577fc..cbfdd4c 100644
@@ -87,8 +87,11 @@ enum {
         IB_USER_VERBS_CMD_CLOSE_XRCD,

include/uapi/rdma/ib_user_verbs.h enum { · line 95

Quoted 10 lines

include/uapi/rdma/ib_user_verbs.h +5 -2

         IB_USER_VERBS_CMD_CREATE_XSRQ,
         IB_USER_VERBS_CMD_OPEN_QP,
-       IB_USER_VERBS_CMD_CREATE_FLOW =B_USER_VERBS_CMD_THRESHOLD,
-       IB_USER_VERBS_CMD_DESTROY_FLOW
+};
+
+enum {
+       IB_USER_VERBS_EX_CMD_CREATE_FLOW =B_USER_VERBS_CMD_THRESHOLD,
+       IB_USER_VERBS_EX_CMD_DESTROY_FLOW
  };

I think B_USER_VERBS_CMD_THRESHOLD could be removed.

Quoted 37 lines

include/uapi/rdma/ib_user_verbs.h +8 -7

  /*

@@ -120,16 +123,20 @@ struct ib_uverbs_comp_event_desc {
   * the rest of the command struct based on these value.
   */
 
+#define IB_USER_VERBS_CMD_COMMAND_MASK 0xff
+#define IB_USER_VERBS_CMD_FLAGS_MASK 0xff000000u
+#define IB_USER_VERBS_CMD_FLAGS_SHIFT 24
+
+#define IB_USER_VERBS_CMD_FLAG_EXTENDED 0x80
+
  struct ib_uverbs_cmd_hdr {
         __u32 command;
         __u16 in_words;
         __u16 out_words;
  };
 
-struct ib_uverbs_cmd_hdr_ex {
-       __u32 command;
-       __u16 in_words;
-       __u16 out_words;
+struct ib_uverbs_ex_cmd_hdr {
+       __u64 response;
         __u16 provider_in_words;
         __u16 provider_out_words;
         __u32 cmd_hdr_reserved;

@@ -777,8 +784,6 @@ struct ib_uverbs_flow_attr {
 
  struct ib_uverbs_create_flow  {
         __u32 comp_mask;
-       __u32 reserved;
-       __u64 response;
         __u32 qp_handle;
         struct ib_uverbs_flow_attr flow_attr;
  };
--

Quoted 2 lines

1.8.3.1

This infrastructure (as well as the original one) doesn't deal with the case we discussed about in the previous patches. When a command contains a structure that can be extended, we don't have a clean nice way of doing that. Though, because it's a limitation of the original design and this patch cleans up the functionality of the original infrastructure, I'm in favor of merging it.

Matan

Quoted 122 lines · Yann Droneaud

Quoted 121 lines

Commit 400dbc9 added an infrastructure for extensible uverbs commands while later commit 436f2ad exported ib_create_flow()/ib_destroy_flow()
functions using this new infrastructure.

drivers/infiniband/core/uverbs_main.c +5 -8

index 75ad86c..4f00654 100644
@@ -587,6 +592,7 @@ static ssize_t ib_uverbs_write(struct file *filp,

@@ -594,41 +600,104 @@ static ssize_t ib_uverbs_write(struct file
 *filp, const char __user *buf,
         if (copy_from_user(&hdr, buf, sizeof hdr))
                 return -EFAULT;
 
-       if (hdr.command >=RRAY_SIZE(uverbs_cmd_table) ||
-           !uverbs_cmd_table[hdr.command])
-               return -EINVAL;
+       flags =hdr.command &
+                IB_USER_VERBS_CMD_FLAGS_MASK) >>
 IB_USER_VERBS_CMD_FLAGS_SHIFT;
 
-       if (!file->ucontext &&
-           hdr.command !=B_USER_VERBS_CMD_GET_CONTEXT)
-               return -EINVAL;
+       if (!flags) {
+               __u32 command;
 
-       if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull <<
 hdr.command)))
-               return -ENOSYS;
+               if (hdr.command & 

~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
+
IB_USER_VERBS_CMD_COMMAND_MASK))

drivers/infiniband/core/uverbs_main.c +5 -5

+                       return -EINVAL;
 
-       if (hdr.command >=B_USER_VERBS_CMD_THRESHOLD) {
-               struct ib_uverbs_cmd_hdr_ex hdr_ex;
+               command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
 
-               if (copy_from_user(&hdr_ex, buf, sizeof(hdr_ex)))
-                       return -EFAULT;
+               if (command >=RRAY_SIZE(uverbs_cmd_table) ||
+                   !uverbs_cmd_table[command])
+                       return -EINVAL;
 
-               if (((hdr_ex.in_words + hdr_ex.provider_in_words) * 

4)
!=ount)

drivers/infiniband/core/uverbs_main.c +2 -2

+               if (!file->ucontext &&
+                   command !=B_USER_VERBS_CMD_GET_CONTEXT)
                         return -EINVAL;
 
-               return uverbs_cmd_table[hdr.command](file,
-                                                    buf +

sizeof(hdr_ex),

drivers/infiniband/core/uverbs_main.c +1 -2

-                                                    (hdr_ex.in_words 
+
-

hdr_ex.provider_in_words) * 4,
-
(hdr_ex.out_words +
-
hdr_ex.provider_out_words) * 4);

drivers/infiniband/core/uverbs_main.c +1 -1

-       } else {
+               if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull 

<<
command)))

drivers/infiniband/core/uverbs_main.c +2 -2

+                       return -ENOSYS;
+
                 if (hdr.in_words * 4 !=ount)
                         return -EINVAL;
 
-               return uverbs_cmd_table[hdr.command](file,
-                                                    buf + 

sizeof(hdr),
- hdr.in_words *
4,
- hdr.out_words *
4);

drivers/infiniband/core/uverbs_main.c +14 -0

+               return uverbs_cmd_table[command](file,
+                                                buf + sizeof(hdr),
+                                                hdr.in_words * 4,
+                                                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;
+               int err;
+
+               if (hdr.command & 

~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK |
+
IB_USER_VERBS_CMD_COMMAND_MASK))

drivers/infiniband/core/uverbs_main.c +12 -0

+                       return -EINVAL;
+
+               command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
+
+               if (command >=RRAY_SIZE(uverbs_ex_cmd_table) ||
+                   !uverbs_ex_cmd_table[command])
+                       return -ENOSYS;
+
+               if (!file->ucontext)
+                       return -EINVAL;
+
+               if (!(file->device->ib_dev->uverbs_ex_cmd_mask & 

(1ull
<< command)))

drivers/infiniband/core/uverbs_main.c +6 -0

+                       return -ENOSYS;
+
+               if (count < (sizeof(hdr) + sizeof(ex_hdr)))
+                       return -EINVAL;
+
+               if (copy_from_user(&ex_hdr, buf + sizeof(hdr),

sizeof(ex_hdr)))

Quoted 3 lines

drivers/infiniband/core/uverbs_main.c static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,

Quoted 3 lines

drivers/infiniband/core/uverbs_main.c +3 -0

+                       return -EFAULT;
+
+               count -=izeof(hdr) + sizeof(ex_hdr);

A small issue: count is reduced here and the write function returns the reduced size.
I think we should return the amount of data the user wrote and not to subtract the headers from the returned size.

Quoted 2 lines

drivers/infiniband/core/uverbs_main.c

Quoted 2 lines

drivers/infiniband/core/uverbs_main.c +2 -0

+
+               return count;

====================^

Quoted 21 lines

Quoted 8 lines

include/uapi/rdma/ib_user_verbs.h +0 -0

index 93577fc..cbfdd4c 100644
@@ -87,8 +87,11 @@ enum {
         IB_USER_VERBS_CMD_CLOSE_XRCD,
         IB_USER_VERBS_CMD_CREATE_XSRQ,

include/uapi/rdma/ib_user_verbs.h enum { · line 96

Quoted 10 lines

include/uapi/rdma/ib_user_verbs.h +5 -2

         IB_USER_VERBS_CMD_OPEN_QP,
-       IB_USER_VERBS_CMD_CREATE_FLOW = IB_USER_VERBS_CMD_THRESHOLD,
-       IB_USER_VERBS_CMD_DESTROY_FLOW
+};
+
+enum {
+       IB_USER_VERBS_EX_CMD_CREATE_FLOW = 
 IB_USER_VERBS_CMD_THRESHOLD,
+       IB_USER_VERBS_EX_CMD_DESTROY_FLOW
  };

I think B_USER_VERBS_CMD_THRESHOLD could be removed.

Hi,

Quoted 118 lines · Matan Barak

Quoted 117 lines · Matan Barak

Quoted 115 lines · Yann Droneaud

Commit 400dbc9 added an infrastructure for extensible uverbs commands while later commit 436f2ad exported ib_create_flow()/ib_destroy_flow()
functions using this new infrastructure.

drivers/infiniband/core/uverbs_main.c +5 -8

index 75ad86c..4f00654 100644
@@ -587,6 +592,7 @@ static ssize_t ib_uverbs_write(struct file *filp,

@@ -594,41 +600,104 @@ static ssize_t ib_uverbs_write(struct file
 *filp, const char __user *buf,
         if (copy_from_user(&hdr, buf, sizeof hdr))
                 return -EFAULT;
 
-       if (hdr.command >=RRAY_SIZE(uverbs_cmd_table) ||
-           !uverbs_cmd_table[hdr.command])
-               return -EINVAL;
+       flags =hdr.command &
+                IB_USER_VERBS_CMD_FLAGS_MASK) >>
 IB_USER_VERBS_CMD_FLAGS_SHIFT;
 
-       if (!file->ucontext &&
-           hdr.command !=B_USER_VERBS_CMD_GET_CONTEXT)
-               return -EINVAL;
+       if (!flags) {
+               __u32 command;
 
-       if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull <<
 hdr.command)))
-               return -ENOSYS;
+               if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK

|
+
IB_USER_VERBS_CMD_COMMAND_MASK))

drivers/infiniband/core/uverbs_main.c +5 -5

+                       return -EINVAL;
 
-       if (hdr.command >=B_USER_VERBS_CMD_THRESHOLD) {
-               struct ib_uverbs_cmd_hdr_ex hdr_ex;
+               command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
 
-               if (copy_from_user(&hdr_ex, buf, sizeof(hdr_ex)))
-                       return -EFAULT;
+               if (command >=RRAY_SIZE(uverbs_cmd_table) ||
+                   !uverbs_cmd_table[command])
+                       return -EINVAL;
 
-               if (((hdr_ex.in_words + hdr_ex.provider_in_words) * 4)

!=ount)

drivers/infiniband/core/uverbs_main.c +2 -2

+               if (!file->ucontext &&
+                   command !=B_USER_VERBS_CMD_GET_CONTEXT)
                         return -EINVAL;
 
-               return uverbs_cmd_table[hdr.command](file,
-                                                    buf +

sizeof(hdr_ex),

drivers/infiniband/core/uverbs_main.c +0 -2

-                                                    (hdr_ex.in_words +
-

hdr_ex.provider_in_words) * 4,

drivers/infiniband/core/uverbs_main.c +0 -2

-                                                    (hdr_ex.out_words +
-

hdr_ex.provider_out_words) * 4);

drivers/infiniband/core/uverbs_main.c +1 -1

-       } else {
+               if (!(file->device->ib_dev->uverbs_cmd_mask & (1ull <<

command)))

drivers/infiniband/core/uverbs_main.c +16 -4

+                       return -ENOSYS;
+
                 if (hdr.in_words * 4 !=ount)
                         return -EINVAL;
 
-               return uverbs_cmd_table[hdr.command](file,
-                                                    buf + sizeof(hdr),
-                                                    hdr.in_words * 4,
-                                                    hdr.out_words * 4);
+               return uverbs_cmd_table[command](file,
+                                                buf + sizeof(hdr),
+                                                hdr.in_words * 4,
+                                                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;
+               int err;
+
+               if (hdr.command & ~(__u32)(IB_USER_VERBS_CMD_FLAGS_MASK

|
+
IB_USER_VERBS_CMD_COMMAND_MASK))

drivers/infiniband/core/uverbs_main.c +12 -0

+                       return -EINVAL;
+
+               command =dr.command & IB_USER_VERBS_CMD_COMMAND_MASK;
+
+               if (command >=RRAY_SIZE(uverbs_ex_cmd_table) ||
+                   !uverbs_ex_cmd_table[command])
+                       return -ENOSYS;
+
+               if (!file->ucontext)
+                       return -EINVAL;
+
+               if (!(file->device->ib_dev->uverbs_ex_cmd_mask & (1ull

<< command)))

drivers/infiniband/core/uverbs_main.c +6 -0

+                       return -ENOSYS;
+
+               if (count < (sizeof(hdr) + sizeof(ex_hdr)))
+                       return -EINVAL;
+
+               if (copy_from_user(&ex_hdr, buf + sizeof(hdr),

sizeof(ex_hdr)))
+ return -EFAULT;

Quoted 9 lines

Quoted 2 lines

drivers/infiniband/core/uverbs_main.c

Quoted 2 lines

drivers/infiniband/core/uverbs_main.c +2 -0

+
+               count -=izeof(hdr) + sizeof(ex_hdr);

A small issue: count is reduced here and the write function returns the reduced size.
I think we should return the amount of data the user wrote and not to subtract the headers from the returned size.

Good catch.

Quoted 32 lines

Quoted 4 lines

drivers/infiniband/core/uverbs_main.c

Quoted 4 lines

drivers/infiniband/core/uverbs_main.c +2 -0

+
+               return count;

====================^

Quoted 22 lines

Quoted 8 lines

include/uapi/rdma/ib_user_verbs.h +0 -0

index 93577fc..cbfdd4c 100644
@@ -87,8 +87,11 @@ enum {
         IB_USER_VERBS_CMD_CLOSE_XRCD,
         IB_USER_VERBS_CMD_CREATE_XSRQ,

include/uapi/rdma/ib_user_verbs.h enum { · line 96

Quoted 10 lines

include/uapi/rdma/ib_user_verbs.h +5 -2

         IB_USER_VERBS_CMD_OPEN_QP,
-       IB_USER_VERBS_CMD_CREATE_FLOW = IB_USER_VERBS_CMD_THRESHOLD,
 
-       IB_USER_VERBS_CMD_DESTROY_FLOW
+};
+
+enum {
+       IB_USER_VERBS_EX_CMD_CREATE_FLOW = IB_USER_VERBS_CMD_THRESHOLD,
+       IB_USER_VERBS_EX_CMD_DESTROY_FLOW
  };

I think B_USER_VERBS_CMD_THRESHOLD could be removed.

If it's removed, CREATE_FLOW will be the first command.

I thought it could be interesting to keep room for the "legacy" commands if they're going to be supported as extended commands in the future.

Regards.

--
Yann Droneaud
OPTEYA

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

The unused field in the extended header is a perfect candidate to hold the command "comp_mask" (eg. bit field used to handle compatibility). This was suggested by Roland Dreier in a previous review[1].

So this patch move comp_mask from create_flow/destroy_flow commands to the extended command header. Then comp_mask is passed as part of function parameters.

[1]:
http://marc.info/?iÊL1RGDXJtrc849M6_XNZT5xO1+ybKtLWGq6yg6LhoSsKpsmkYA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

Cc Igor Ivanov <[email protected]>

Cc Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/[email protected]>

Signed-off-by Yann Droneaud <[email protected]>

Link http://marc.info/?i=ver.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link http://mid.gmane.org/cover.1381510045.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

---
  drivers/infiniband/core/uverbs.h      |  3 ++-
  drivers/infiniband/core/uverbs_cmd.c  | 15 ++++++++++-----
  drivers/infiniband/core/uverbs_main.c |  6 ++++--
  include/uapi/rdma/ib_user_verbs.h     |  6 +++---
  4 files changed, 19 insertions(+), 11 deletions(-)

drivers/infiniband/core/uverbs.h +2 -1

index bdc842e..a12b516 100644
@@ -245,7 +245,8 @@ IB_UVERBS_DECLARE_CMD(close_xrcd);
  #define IB_UVERBS_DECLARE_EX_CMD(name)                         \
         int ib_uverbs_ex_##name(struct ib_uverbs_file *file,    \
                                 struct ib_udata *ucore,         \
-                               struct ib_udata *uhw)
+                               struct ib_udata *uhw,           \
+                               __u32 comp_mask)
 
  IB_UVERBS_DECLARE_EX_CMD(create_flow);
  IB_UVERBS_DECLARE_EX_CMD(destroy_flow);

drivers/infiniband/core/uverbs_cmd.c +10 -5

index e35877d..a8ecb3a 100644
@@ -2633,7 +2633,8 @@ static int kern_spec_to_ib_spec(struct ib_uverbs_flow_spec *kern_spec,
 
  int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
                              struct ib_udata *ucore,
-                            struct ib_udata *uhw)
+                            struct ib_udata *uhw,
+                            __u32 comp_mask)
  {
         struct ib_uverbs_create_flow      cmd;
         struct ib_uverbs_create_flow_resp resp;

@@ -2647,6 +2648,9 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
         void *ib_spec;
         int i;
 
+       if (comp_mask)
+               return -EINVAL;
+
         if (ucore->outlen < sizeof(resp))
                 return -ENOSPC;
 

@@ -2657,9 +2661,6 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
         ucore->inbuf +=izeof(cmd);
         ucore->inlen -=izeof(cmd);
 
-       if (cmd.comp_mask)
-               return -EINVAL;
-
         if ((cmd.flow_attr.type =IB_FLOW_ATTR_SNIFFER &&
              !capable(CAP_NET_ADMIN)) || !capable(CAP_NET_RAW))
                 return -EPERM;

@@ -2785,13 +2786,17 @@ err_free_attr:
 
  int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
                               struct ib_udata *ucore,
-                             struct ib_udata *uhw)
+                             struct ib_udata *uhw,
+                             __u32 comp_mask)
  {
         struct ib_uverbs_destroy_flow   cmd;
         struct ib_flow                  *flow_id;
         struct ib_uobject               *uobj;
         int                             ret;
 
+       if (comp_mask)
+               return -EINVAL;
+
         ret =b_copy_from_udata(&cmd, ucore, sizeof(cmd));
         if (ret)
                 return ret;

drivers/infiniband/core/uverbs_main.c +4 -2

index 4f00654..a9687dd 100644
@@ -119,7 +119,8 @@ static ssize_t (*uverbs_cmd_table[])(struct ib_uverbs_file *file,
 
  static int (*uverbs_ex_cmd_table[])(struct ib_uverbs_file *file,
                                     struct ib_udata *ucore,
-                                   struct ib_udata *uhw) =
+                                   struct ib_udata *uhw,
+                                   __u32 comp_mask) =
         [IB_USER_VERBS_EX_CMD_CREATE_FLOW]      =b_uverbs_ex_create_flow,
         [IB_USER_VERBS_EX_CMD_DESTROY_FLOW]     =b_uverbs_ex_destroy_flow
  };

@@ -689,7 +690,8 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 
                 err =verbs_ex_cmd_table[command](file,
                                                    &ucore,
-                                                  &uhw);
+                                                  &uhw,
+                                                  ex_hdr.comp_mask);
 
                 if (err)
                         return err;

include/uapi/rdma/ib_user_verbs.h +3 -4

index cbfdd4c..095a2bd 100644
@@ -139,7 +139,7 @@ struct ib_uverbs_ex_cmd_hdr {
         __u64 response;
         __u16 provider_in_words;
         __u16 provider_out_words;
-       __u32 cmd_hdr_reserved;
+       __u32 comp_mask;
  };
 
  struct ib_uverbs_get_context {

@@ -783,8 +783,8 @@ struct ib_uverbs_flow_attr {
  };
 
  struct ib_uverbs_create_flow  {
-       __u32 comp_mask;
         __u32 qp_handle;
+       __u32 reserved;
         struct ib_uverbs_flow_attr flow_attr;
  };
 

@@ -794,8 +794,8 @@ struct ib_uverbs_create_flow_resp {
  };
 
  struct ib_uverbs_destroy_flow  {
-       __u32 comp_mask;
         __u32 flow_handle;
+       __u32 reserved;
  };
 
  struct ib_uverbs_create_srq {
--

1.8.3.1

As requested by Or Gerlitz, I've rebased my previous fixes [1] against Matan Barak patch [2]. I've added two patches to fix issues left or introduced by this patch.
Regarding the previous patchset [1], I've kept the public data structure improving and renaming patches, but, in order to minimize the overall changes, I've removed the changes which were modifying the functions and variables names in the code.
I've removed "code beautifying" and left "hardening" / cleanup changes for a latter release.

On top of this, I've added the two acknowledged and important patches from my extended command infrastructure [3]. I've made cleanups in the changelog regarding the 'email' citations.