Re: [PATCH 8/9] IB/core: extended command: an improved infrastructure for uverbs commands

[Yann Droneaud]

Hi,

Le 17.10.2013 16:41, Matan Barak a écrit :

On 14/10/2013 6:38 PM, Matan Barak wrote:

On 11/10/2013 8:57 PM, Yann Droneaud wrote:

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.

diff --git a/drivers/infiniband/core/uverbs_main.c
b/drivers/infiniband/core/uverbs_main.c
index 75ad86c..4f00654 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -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))
+                       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);

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.

+
+               return count;

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

diff --git a/include/uapi/rdma/ib_user_verbs.h
b/include/uapi/rdma/ib_user_verbs.h
index 93577fc..cbfdd4c 100644
--- a/include/uapi/rdma/ib_user_verbs.h
+++ b/include/uapi/rdma/ib_user_verbs.h
@@ -87,8 +87,11 @@ enum {
         IB_USER_VERBS_CMD_CLOSE_XRCD,
         IB_USER_VERBS_CMD_CREATE_XSRQ,
         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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html