[PATCH for v3.13 0/7] create_flow/destroy_flow fixes for v3.13

[PATCH for v3.13 0/7] create_flow/destroy_flow fixes for v3.13

[Yann Droneaud]

Hi,

Please find a patchset against create_flow/destroy_flow and
associated extended command scheme.

These are fixes that must be applied before making the new uverbs
widely available.

This patchset gather some patches already sent independently:
- The first two patches were already sent[1] to address a warning
  reported by sparse.
- The next patch was already sent[2] to handle an uncommon type
  of extended command.

The three patches ensure that commands will be extensible:
- One patch add a missing check of comp_mask
- Two patches add checks on reserved fields
  following advice from an article read today[3].

The last patches fix an error path.

Please review and apply for v3.13.

Regards.

[1] [PATCH for-next 0/2] Fix "drivers/infiniband/core/uverbs_main.c:683:17: sparse: Using plain integer as NULL pointer" warning
    http://marc.info/?i=cover.1384869925.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

[2] [PATCH for-next] IB/uverbs: set ucore.outbuf to NULL if core response space is omitted
    http://marc.info/?i=1384872527-26154-1-git-send-email-ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

[3] "Botching up ioctls" by Daniel Vetter
    http://blog.ffwll.ch/2013/11/botching-up-ioctls.html

Yann Droneaud (7):
  IB/core: const'ify inbuf in struct ib_udata
  IB/uverbs: remove implicit cast in INIT_UDATA()
  IB/uverbs: set outbuf to NULL when no core response space is provided
  IB/uverbs: check reserved field in extended command header
  IB/uverbs: check comp_mask in destroy_flow
  IB/uverbs: check reserved fields in create_flow
  IB/uverbs: set error code when fail to consume all flow_spec items

 drivers/infiniband/core/uverbs.h      | 12 ++++++------
 drivers/infiniband/core/uverbs_cmd.c  | 31 +++++++++++++++++++++----------
 drivers/infiniband/core/uverbs_main.c | 16 +++++++++++-----
 include/rdma/ib_verbs.h               |  2 +-
 4 files changed, 39 insertions(+), 22 deletions(-)

-- 
1.8.4.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 for v3.13 1/7] IB/core: const'ify inbuf in struct ib_udata

[Yann Droneaud]

Userspace input buffer is not modified by kernel,
let it be 'const'.

It's also a prerequisite to remove the implicit cast
from INIT_UDATA().

Link: http://marc.info/?i=cover.1385501822.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs.h | 2 +-
 include/rdma/ib_verbs.h          | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index bdc842e9faef..9879568aed8c 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -49,7 +49,7 @@
 
 #define INIT_UDATA(udata, ibuf, obuf, ilen, olen)			\
 	do {								\
-		(udata)->inbuf  = (void __user *) (ibuf);		\
+		(udata)->inbuf  = (const void __user *) (ibuf);		\
 		(udata)->outbuf = (void __user *) (obuf);		\
 		(udata)->inlen  = (ilen);				\
 		(udata)->outlen = (olen);				\
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index 979874c627ee..61e1935c91b1 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -978,7 +978,7 @@ struct ib_uobject {
 };
 
 struct ib_udata {
-	void __user *inbuf;
+	const void __user *inbuf;
 	void __user *outbuf;
 	size_t       inlen;
 	size_t       outlen;
-- 
1.8.4.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 for v3.13 2/7] IB/uverbs: remove implicit cast in INIT_UDATA()

[Yann Droneaud]

Currently, INIT_UDATA() does an implicit cast to a pointer,
so that 'response' address, eg. output buffer, can be used
as is to initialize a struct ib_udata:

        do {                                                    \
                (udata)->inbuf  = (void __user *) (ibuf);       \
                (udata)->outbuf = (void __user *) (obuf);       \
                (udata)->inlen  = (ilen);                       \
                (udata)->outlen = (olen);                       \
        } while (0)

...

        INIT_UDATA(&udata, buf + sizeof cmd,
                   (unsigned long) cmd.response + sizeof resp,
                   in_len - sizeof cmd, out_len - sizeof  resp);

...

Hidding the integer to pointer conversion is prone to error
that won't be catched by compiler/static analyzer is some case.

In the other hand, sparse reports an error if literal 0 is used
to initialize inbuf or outbuf, for example in:

        INIT_UDATA(&ucore,
                   (hdr.in_words) ? buf : 0,
                   (unsigned long)ex_hdr.response,
                   hdr.in_words * 8,
                   hdr.out_words * 8);

It was reported by kbuild test robot in message[1]:

  From: kbuild test robot <fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
  Subject: "drivers/infiniband/core/uverbs_main.c:683:17:
      sparse: Using plain integer as NULL pointer",
  Message-Id: <528b3984.SVGs20ZWpcuR/Jls%fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>

This patch fixes the warnings reported by sparse and allows the compiler
to report a warning in case a plain integer get used to initialize
a udata pointer.

This patch requires struct ib_udata to be modified to have a
const void __user *inbuf field[2], otherwise compiler will report warnings
regarding const to non const conversion:

drivers/infiniband/core/uverbs_main.c: In function ‘ib_uverbs_write’:
drivers/infiniband/core/uverbs_main.c:682:24: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default]
drivers/infiniband/core/uverbs_main.c:688:22: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default]
drivers/infiniband/core/uverbs_cmd.c: In function ‘ib_uverbs_get_context’:
drivers/infiniband/core/uverbs_cmd.c:307:23: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default]
drivers/infiniband/core/uverbs_cmd.c: In function ‘ib_uverbs_alloc_pd’:
drivers/infiniband/core/uverbs_cmd.c:516:23: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default]
...

[1] https://lists.01.org/pipermail/kbuild-all/2013-November/002120.html

[2] https://patchwork.kernel.org/patch/2846202/
    http://marc.info/?i=3050a98379b4342ea59d59aeaf1ce162171df928.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Link: http://marc.info/?i=cover.1385501822.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs.h      | 12 ++++++------
 drivers/infiniband/core/uverbs_cmd.c  | 20 ++++++++++----------
 drivers/infiniband/core/uverbs_main.c | 13 ++++++++-----
 3 files changed, 24 insertions(+), 21 deletions(-)

diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
index 9879568aed8c..0dca1975d59d 100644
--- a/drivers/infiniband/core/uverbs.h
+++ b/drivers/infiniband/core/uverbs.h
@@ -47,12 +47,12 @@
 #include <rdma/ib_umem.h>
 #include <rdma/ib_user_verbs.h>
 
-#define INIT_UDATA(udata, ibuf, obuf, ilen, olen)			\
-	do {								\
-		(udata)->inbuf  = (const void __user *) (ibuf);		\
-		(udata)->outbuf = (void __user *) (obuf);		\
-		(udata)->inlen  = (ilen);				\
-		(udata)->outlen = (olen);				\
+#define INIT_UDATA(udata, ibuf, obuf, ilen, olen)	\
+	do {						\
+		(udata)->inbuf  = (ibuf);		\
+		(udata)->outbuf = (obuf);		\
+		(udata)->inlen  = (ilen);		\
+		(udata)->outlen = (olen);		\
 	} while (0)
 
 /*
diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index 65f6e7dc380c..d9d91c412628 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -305,7 +305,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file,
 	}
 
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   (void __user *)(unsigned long)cmd.response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	ucontext = ibdev->alloc_ucontext(ibdev, &udata);
@@ -514,7 +514,7 @@ ssize_t ib_uverbs_alloc_pd(struct ib_uverbs_file *file,
 		return -EFAULT;
 
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   (void __user *)(unsigned long)cmd.response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	uobj = kmalloc(sizeof *uobj, GFP_KERNEL);
@@ -711,7 +711,7 @@ ssize_t ib_uverbs_open_xrcd(struct ib_uverbs_file *file,
 		return -EFAULT;
 
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   (void __user *)(unsigned long)cmd.response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof  resp);
 
 	mutex_lock(&file->device->xrcd_tree_mutex);
@@ -923,7 +923,7 @@ ssize_t ib_uverbs_reg_mr(struct ib_uverbs_file *file,
 		return -EFAULT;
 
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   (void __user *)(unsigned long)cmd.response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	if ((cmd.start & ~PAGE_MASK) != (cmd.hca_va & ~PAGE_MASK))
@@ -1215,7 +1215,7 @@ ssize_t ib_uverbs_create_cq(struct ib_uverbs_file *file,
 		return -EFAULT;
 
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   (void __user *)(unsigned long)cmd.response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	if (cmd.comp_vector >= file->device->num_comp_vectors)
@@ -1311,7 +1311,7 @@ ssize_t ib_uverbs_resize_cq(struct ib_uverbs_file *file,
 		return -EFAULT;
 
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   (void __user *)(unsigned long)cmd.response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	cq = idr_read_cq(cmd.cq_handle, file->ucontext, 0);
@@ -1513,7 +1513,7 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
 		return -EPERM;
 
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   (void __user *)(unsigned long)cmd.response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	obj = kzalloc(sizeof *obj, GFP_KERNEL);
@@ -1700,7 +1700,7 @@ ssize_t ib_uverbs_open_qp(struct ib_uverbs_file *file,
 		return -EFAULT;
 
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   (void __user *)(unsigned long)cmd.response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	obj = kmalloc(sizeof *obj, GFP_KERNEL);
@@ -2976,7 +2976,7 @@ ssize_t ib_uverbs_create_srq(struct ib_uverbs_file *file,
 	xcmd.srq_limit	 = cmd.srq_limit;
 
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   (void __user *)(unsigned long)cmd.response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	ret = __uverbs_create_xsrq(file, &xcmd, &udata);
@@ -3001,7 +3001,7 @@ ssize_t ib_uverbs_create_xsrq(struct ib_uverbs_file *file,
 		return -EFAULT;
 
 	INIT_UDATA(&udata, buf + sizeof cmd,
-		   (unsigned long) cmd.response + sizeof resp,
+		   (void __user *)(unsigned long)cmd.response + sizeof resp,
 		   in_len - sizeof cmd, out_len - sizeof resp);
 
 	ret = __uverbs_create_xsrq(file, &cmd, &udata);
diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 34386943ebcf..14d864371050 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -635,6 +635,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		__u32 command;
 
 		struct ib_uverbs_ex_cmd_hdr ex_hdr;
+		char __user *response;
 		struct ib_udata ucore;
 		struct ib_udata uhw;
 		int err;
@@ -668,7 +669,9 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		if ((hdr.in_words + ex_hdr.provider_in_words) * 8 != count)
 			return -EINVAL;
 
-		if (ex_hdr.response) {
+		response = (char __user *)(unsigned long)ex_hdr.response;
+
+		if (response) {
 			if (!hdr.out_words && !ex_hdr.provider_out_words)
 				return -EINVAL;
 		} else {
@@ -677,14 +680,14 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		}
 
 		INIT_UDATA(&ucore,
-			   (hdr.in_words) ? buf : 0,
-			   (unsigned long)ex_hdr.response,
+			   (hdr.in_words) ? buf : NULL,
+			   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) ? buf + ucore.inlen : NULL,
+			   (ex_hdr.provider_out_words) ? response + ucore.outlen : NULL,
 			   ex_hdr.provider_in_words * 8,
 			   ex_hdr.provider_out_words * 8);
 
-- 
1.8.4.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 for v3.13 2/7] IB/uverbs: remove implicit cast in INIT_UDATA()

[Matan Barak]

On 27/11/2013 12:02 AM, Yann Droneaud wrote:
> Currently, INIT_UDATA() does an implicit cast to a pointer,
> so that 'response' address, eg. output buffer, can be used
> as is to initialize a struct ib_udata:
>
>          do {                                                    \
>                  (udata)->inbuf  = (void __user *) (ibuf);       \
>                  (udata)->outbuf = (void __user *) (obuf);       \
>                  (udata)->inlen  = (ilen);                       \
>                  (udata)->outlen = (olen);                       \
>          } while (0)
>
> ...
>
>          INIT_UDATA(&udata, buf + sizeof cmd,
>                     (unsigned long) cmd.response + sizeof resp,
>                     in_len - sizeof cmd, out_len - sizeof  resp);
>
> ...
>
> Hidding the integer to pointer conversion is prone to error
> that won't be catched by compiler/static analyzer is some case.
>
> In the other hand, sparse reports an error if literal 0 is used
> to initialize inbuf or outbuf, for example in:
>
>          INIT_UDATA(&ucore,
>                     (hdr.in_words) ? buf : 0,
>                     (unsigned long)ex_hdr.response,
>                     hdr.in_words * 8,
>                     hdr.out_words * 8);
>
> It was reported by kbuild test robot in message[1]:
>
>    From: kbuild test robot <fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>    Subject: "drivers/infiniband/core/uverbs_main.c:683:17:
>        sparse: Using plain integer as NULL pointer",
>    Message-Id: <528b3984.SVGs20ZWpcuR/Jls%fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>
> This patch fixes the warnings reported by sparse and allows the compiler
> to report a warning in case a plain integer get used to initialize
> a udata pointer.
>
> This patch requires struct ib_udata to be modified to have a
> const void __user *inbuf field[2], otherwise compiler will report warnings
> regarding const to non const conversion:
>
> drivers/infiniband/core/uverbs_main.c: In function ‘ib_uverbs_write’:
> drivers/infiniband/core/uverbs_main.c:682:24: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default]
> drivers/infiniband/core/uverbs_main.c:688:22: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default]
> drivers/infiniband/core/uverbs_cmd.c: In function ‘ib_uverbs_get_context’:
> drivers/infiniband/core/uverbs_cmd.c:307:23: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default]
> drivers/infiniband/core/uverbs_cmd.c: In function ‘ib_uverbs_alloc_pd’:
> drivers/infiniband/core/uverbs_cmd.c:516:23: attention : assignment discards ‘const’ qualifier from pointer target type [enabled by default]
> ...
>
> [1] https://lists.01.org/pipermail/kbuild-all/2013-November/002120.html
>
> [2] https://patchwork.kernel.org/patch/2846202/
>      http://marc.info/?i=3050a98379b4342ea59d59aeaf1ce162171df928.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
>
> Link: http://marc.info/?i=cover.1385501822.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
> Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
> ---
>   drivers/infiniband/core/uverbs.h      | 12 ++++++------
>   drivers/infiniband/core/uverbs_cmd.c  | 20 ++++++++++----------
>   drivers/infiniband/core/uverbs_main.c | 13 ++++++++-----
>   3 files changed, 24 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/infiniband/core/uverbs.h b/drivers/infiniband/core/uverbs.h
> index 9879568aed8c..0dca1975d59d 100644
> --- a/drivers/infiniband/core/uverbs.h
> +++ b/drivers/infiniband/core/uverbs.h
> @@ -47,12 +47,12 @@
>   #include <rdma/ib_umem.h>
>   #include <rdma/ib_user_verbs.h>
>
> -#define INIT_UDATA(udata, ibuf, obuf, ilen, olen)			\
> -	do {								\
> -		(udata)->inbuf  = (const void __user *) (ibuf);		\
> -		(udata)->outbuf = (void __user *) (obuf);		\
> -		(udata)->inlen  = (ilen);				\
> -		(udata)->outlen = (olen);				\
> +#define INIT_UDATA(udata, ibuf, obuf, ilen, olen)	\
> +	do {						\
> +		(udata)->inbuf  = (ibuf);		\
> +		(udata)->outbuf = (obuf);		\
> +		(udata)->inlen  = (ilen);		\
> +		(udata)->outlen = (olen);		\
>   	} while (0)
>
>   /*
> diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
> index 65f6e7dc380c..d9d91c412628 100644
> --- a/drivers/infiniband/core/uverbs_cmd.c
> +++ b/drivers/infiniband/core/uverbs_cmd.c
> @@ -305,7 +305,7 @@ ssize_t ib_uverbs_get_context(struct ib_uverbs_file *file,
>   	}
>
>   	INIT_UDATA(&udata, buf + sizeof cmd,
> -		   (unsigned long) cmd.response + sizeof resp,
> +		   (void __user *)(unsigned long)cmd.response + sizeof resp,

The response field is already __u64 and casting to (void __user *) 
should match the machine's pointer type size. Why do we have to cast to 
(unsigned long) and then cast to (void __user *) ?

>   		   in_len - sizeof cmd, out_len - sizeof resp);
>
>   	ucontext = ibdev->alloc_ucontext(ibdev, &udata);
> @@ -514,7 +514,7 @@ ssize_t ib_uverbs_alloc_pd(struct ib_uverbs_file *file,
>   		return -EFAULT;
>
>   	INIT_UDATA(&udata, buf + sizeof cmd,
> -		   (unsigned long) cmd.response + sizeof resp,
> +		   (void __user *)(unsigned long)cmd.response + sizeof resp,
>   		   in_len - sizeof cmd, out_len - sizeof resp);
>
>   	uobj = kmalloc(sizeof *uobj, GFP_KERNEL);
> @@ -711,7 +711,7 @@ ssize_t ib_uverbs_open_xrcd(struct ib_uverbs_file *file,
>   		return -EFAULT;
>
>   	INIT_UDATA(&udata, buf + sizeof cmd,
> -		   (unsigned long) cmd.response + sizeof resp,
> +		   (void __user *)(unsigned long)cmd.response + sizeof resp,
>   		   in_len - sizeof cmd, out_len - sizeof  resp);
>
>   	mutex_lock(&file->device->xrcd_tree_mutex);
> @@ -923,7 +923,7 @@ ssize_t ib_uverbs_reg_mr(struct ib_uverbs_file *file,
>   		return -EFAULT;
>
>   	INIT_UDATA(&udata, buf + sizeof cmd,
> -		   (unsigned long) cmd.response + sizeof resp,
> +		   (void __user *)(unsigned long)cmd.response + sizeof resp,
>   		   in_len - sizeof cmd, out_len - sizeof resp);
>
>   	if ((cmd.start & ~PAGE_MASK) != (cmd.hca_va & ~PAGE_MASK))
> @@ -1215,7 +1215,7 @@ ssize_t ib_uverbs_create_cq(struct ib_uverbs_file *file,
>   		return -EFAULT;
>
>   	INIT_UDATA(&udata, buf + sizeof cmd,
> -		   (unsigned long) cmd.response + sizeof resp,
> +		   (void __user *)(unsigned long)cmd.response + sizeof resp,
>   		   in_len - sizeof cmd, out_len - sizeof resp);
>
>   	if (cmd.comp_vector >= file->device->num_comp_vectors)
> @@ -1311,7 +1311,7 @@ ssize_t ib_uverbs_resize_cq(struct ib_uverbs_file *file,
>   		return -EFAULT;
>
>   	INIT_UDATA(&udata, buf + sizeof cmd,
> -		   (unsigned long) cmd.response + sizeof resp,
> +		   (void __user *)(unsigned long)cmd.response + sizeof resp,
>   		   in_len - sizeof cmd, out_len - sizeof resp);
>
>   	cq = idr_read_cq(cmd.cq_handle, file->ucontext, 0);
> @@ -1513,7 +1513,7 @@ ssize_t ib_uverbs_create_qp(struct ib_uverbs_file *file,
>   		return -EPERM;
>
>   	INIT_UDATA(&udata, buf + sizeof cmd,
> -		   (unsigned long) cmd.response + sizeof resp,
> +		   (void __user *)(unsigned long)cmd.response + sizeof resp,
>   		   in_len - sizeof cmd, out_len - sizeof resp);
>
>   	obj = kzalloc(sizeof *obj, GFP_KERNEL);
> @@ -1700,7 +1700,7 @@ ssize_t ib_uverbs_open_qp(struct ib_uverbs_file *file,
>   		return -EFAULT;
>
>   	INIT_UDATA(&udata, buf + sizeof cmd,
> -		   (unsigned long) cmd.response + sizeof resp,
> +		   (void __user *)(unsigned long)cmd.response + sizeof resp,
>   		   in_len - sizeof cmd, out_len - sizeof resp);
>
>   	obj = kmalloc(sizeof *obj, GFP_KERNEL);
> @@ -2976,7 +2976,7 @@ ssize_t ib_uverbs_create_srq(struct ib_uverbs_file *file,
>   	xcmd.srq_limit	 = cmd.srq_limit;
>
>   	INIT_UDATA(&udata, buf + sizeof cmd,
> -		   (unsigned long) cmd.response + sizeof resp,
> +		   (void __user *)(unsigned long)cmd.response + sizeof resp,
>   		   in_len - sizeof cmd, out_len - sizeof resp);
>
>   	ret = __uverbs_create_xsrq(file, &xcmd, &udata);
> @@ -3001,7 +3001,7 @@ ssize_t ib_uverbs_create_xsrq(struct ib_uverbs_file *file,
>   		return -EFAULT;
>
>   	INIT_UDATA(&udata, buf + sizeof cmd,
> -		   (unsigned long) cmd.response + sizeof resp,
> +		   (void __user *)(unsigned long)cmd.response + sizeof resp,
>   		   in_len - sizeof cmd, out_len - sizeof resp);
>
>   	ret = __uverbs_create_xsrq(file, &cmd, &udata);
> diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
> index 34386943ebcf..14d864371050 100644
> --- a/drivers/infiniband/core/uverbs_main.c
> +++ b/drivers/infiniband/core/uverbs_main.c
> @@ -635,6 +635,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>   		__u32 command;
>
>   		struct ib_uverbs_ex_cmd_hdr ex_hdr;
> +		char __user *response;
>   		struct ib_udata ucore;
>   		struct ib_udata uhw;
>   		int err;
> @@ -668,7 +669,9 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>   		if ((hdr.in_words + ex_hdr.provider_in_words) * 8 != count)
>   			return -EINVAL;
>
> -		if (ex_hdr.response) {
> +		response = (char __user *)(unsigned long)ex_hdr.response;
> +
> +		if (response) {
>   			if (!hdr.out_words && !ex_hdr.provider_out_words)
>   				return -EINVAL;
>   		} else {
> @@ -677,14 +680,14 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
>   		}
>
>   		INIT_UDATA(&ucore,
> -			   (hdr.in_words) ? buf : 0,
> -			   (unsigned long)ex_hdr.response,
> +			   (hdr.in_words) ? buf : NULL,
> +			   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) ? buf + ucore.inlen : NULL,
> +			   (ex_hdr.provider_out_words) ? response + ucore.outlen : NULL,
>   			   ex_hdr.provider_in_words * 8,
>   			   ex_hdr.provider_out_words * 8);
>
>

Best regards,
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 for v3.13 2/7] IB/uverbs: remove implicit cast in INIT_UDATA()

[Yann Droneaud]

Hi Matan,

Le mercredi 27 novembre 2013 à 10:21 +0200, Matan Barak a écrit :
> On 27/11/2013 12:02 AM, Yann Droneaud wrote:
...
> >   	INIT_UDATA(&udata, buf + sizeof cmd,
> > -		   (unsigned long) cmd.response + sizeof resp,
> > +		   (void __user *)(unsigned long)cmd.response + sizeof resp,
> 
> The response field is already __u64 and casting to (void __user *) 
> should match the machine's pointer type size. Why do we have to cast to 
> (unsigned long) and then cast to (void __user *) ?
> 

On 32bit ABI, u64 is not matching the size of the pointer.
Without the cast to unsigned long, GCC complains with:

  warning: cast to pointer from integer of different size
[-Wint-to-pointer-cast]

So the cast is required on 32bit platforms.

Regards.
-- 
Yann Droneaud
OPTEYA



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

[PATCH for v3.13 3/7] IB/uverbs: set outbuf to NULL when no core response space is provided

[Yann Droneaud]

In the currently uncommon case of core (eg. uverbs) response
space being omitted, but hw (eg. provider) response space being
available, outbuf get defined to "response" while it must be NULL.

This patch takes care of setting ucore->outbuf to NULL
if hdr.out_words is equal to 0.

Link: http://marc.info/?i=cover.1385501822.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 14d864371050..6c4fc6338b26 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -681,7 +681,7 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 
 		INIT_UDATA(&ucore,
 			   (hdr.in_words) ? buf : NULL,
-			   response,
+			   (hdr.out_words) ? response : NULL,
 			   hdr.in_words * 8,
 			   hdr.out_words * 8);
 
-- 
1.8.4.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 for v3.13 4/7] IB/uverbs: check reserved field in extended command header

[Yann Droneaud]

As noted by Daniel Vetter in its article "Botching up ioctls"[1]

  "Check *all* unused fields and flags and all the padding for
   whether it's 0, and reject the ioctl if that's not the case.
   Otherwise your nice plan for future extensions is going right
   down the gutters since someone *will* submit an ioctl struct
   with random stack garbage in the yet unused parts. Which then
   bakes in the ABI that those fields can never be used for
   anything else but garbage."

It's important to ensure that reserved fields are set to known
value, so that it will be possible to use them latter to extend
the ABI.

The same reasonning apply to comp_mask field present in newer
uverbs command: per commit 22878dbc9173, unsupported values in
comp_mask are rejected.

[1] http://blog.ffwll.ch/2013/11/botching-up-ioctls.html

Link: http://marc.info/?i=cover.1385501822.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/core/uverbs_main.c b/drivers/infiniband/core/uverbs_main.c
index 6c4fc6338b26..8652c13f6ea2 100644
--- a/drivers/infiniband/core/uverbs_main.c
+++ b/drivers/infiniband/core/uverbs_main.c
@@ -669,6 +669,9 @@ static ssize_t ib_uverbs_write(struct file *filp, const char __user *buf,
 		if ((hdr.in_words + ex_hdr.provider_in_words) * 8 != count)
 			return -EINVAL;
 
+		if (ex_hdr.cmd_hdr_reserved)
+			return -EINVAL;
+
 		response = (char __user *)(unsigned long)ex_hdr.response;
 
 		if (response) {
-- 
1.8.4.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 for v3.13 5/7] IB/uverbs: check comp_mask in destroy_flow

[Yann Droneaud]

Just like the check added to create_flow in 22878dbc9173,
comp_mask must be checked in destroy_flow too.

Since only empty comp_mask is currently supported,
any other value must be rejected.

Link: http://marc.info/?i=cover.1385501822.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Cc: Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs_cmd.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index d9d91c412628..f1ba441cd2ed 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2795,6 +2795,9 @@ int ib_uverbs_ex_destroy_flow(struct ib_uverbs_file *file,
 	if (ret)
 		return ret;
 
+	if (cmd.comp_mask)
+		return -EINVAL;
+
 	uobj = idr_write_uobj(&ib_uverbs_rule_idr, cmd.flow_handle,
 			      file->ucontext);
 	if (!uobj)
-- 
1.8.4.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 for v3.13 6/7] IB/uverbs: check reserved fields in create_flow

[Yann Droneaud]

As noted by Daniel Vetter in its article "Botching up ioctls"[1]

  "Check *all* unused fields and flags and all the padding for
   whether it's 0, and reject the ioctl if that's not the case.
   Otherwise your nice plan for future extensions is going right
   down the gutters since someone *will* submit an ioctl struct
   with random stack garbage in the yet unused parts. Which then
   bakes in the ABI that those fields can never be used for
   anything else but garbage."

It's important to ensure that reserved fields are set to known
value, so that it will be possible to use them latter to extend
the ABI.

The same reasonning apply to comp_mask field present in newer
uverbs command: per commit 22878dbc9173, unsupported values in
comp_mask are rejected.

[1] http://blog.ffwll.ch/2013/11/botching-up-ioctls.html

Link: http://marc.info/?i=cover.1385501822.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs_cmd.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index f1ba441cd2ed..dd1c5b6ab019 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2593,6 +2593,9 @@ out_put:
 static int kern_spec_to_ib_spec(struct ib_uverbs_flow_spec *kern_spec,
 				union ib_flow_spec *ib_spec)
 {
+	if (kern_spec->reserved)
+		return -EINVAL;
+
 	ib_spec->type = kern_spec->type;
 
 	switch (ib_spec->type) {
@@ -2671,6 +2674,10 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	    (cmd.flow_attr.num_of_specs * sizeof(struct ib_uverbs_flow_spec)))
 		return -EINVAL;
 
+	if (cmd.flow_attr.reserved[0] ||
+	    cmd.flow_attr.reserved[1])
+		return -EINVAL;
+
 	if (cmd.flow_attr.num_of_specs) {
 		kern_flow_attr = kmalloc(sizeof(*kern_flow_attr) + cmd.flow_attr.size,
 					 GFP_KERNEL);
-- 
1.8.4.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 for v3.13 7/7] IB/uverbs: set error code when fail to consume all flow_spec items

[Yann Droneaud]

If the flow_spec items parsed count does not match the number
of items declared in the flow_attr command, or if not all
bytes are used for flow_spec items (eg. trailing garbage),
a log message is reported and the function leave through
the error path. Unfortunately the error code is currently
not set.

This patch set error code to -EINVAL in such cases, so
that the error is reported to userspace instead of silently
fail.

Link: http://marc.info/?i=cover.1385501822.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/core/uverbs_cmd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/infiniband/core/uverbs_cmd.c b/drivers/infiniband/core/uverbs_cmd.c
index dd1c5b6ab019..5976d885f408 100644
--- a/drivers/infiniband/core/uverbs_cmd.c
+++ b/drivers/infiniband/core/uverbs_cmd.c
@@ -2738,6 +2738,7 @@ int ib_uverbs_ex_create_flow(struct ib_uverbs_file *file,
 	if (cmd.flow_attr.size || (i != flow_attr->num_of_specs)) {
 		pr_warn("create flow failed, flow %d: %d bytes left from uverb cmd\n",
 			i, cmd.flow_attr.size);
+		err = -EINVAL;
 		goto err_free;
 	}
 	flow_id = ib_create_flow(qp, flow_attr, IB_FLOW_DOMAIN_USER);
-- 
1.8.4.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 for v3.13 0/7] create_flow/destroy_flow fixes for v3.13

[Matan Barak]

On 27/11/2013 12:02 AM, Yann Droneaud wrote:
> Hi,
>
> Please find a patchset against create_flow/destroy_flow and
> associated extended command scheme.
>
> These are fixes that must be applied before making the new uverbs
> widely available.
>
> This patchset gather some patches already sent independently:
> - The first two patches were already sent[1] to address a warning
>    reported by sparse.
> - The next patch was already sent[2] to handle an uncommon type
>    of extended command.
>
> The three patches ensure that commands will be extensible:
> - One patch add a missing check of comp_mask
> - Two patches add checks on reserved fields
>    following advice from an article read today[3].
>
> The last patches fix an error path.
>
> Please review and apply for v3.13.
>
> Regards.
>
> [1] [PATCH for-next 0/2] Fix "drivers/infiniband/core/uverbs_main.c:683:17: sparse: Using plain integer as NULL pointer" warning
>      http://marc.info/?i=cover.1384869925.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
>
> [2] [PATCH for-next] IB/uverbs: set ucore.outbuf to NULL if core response space is omitted
>      http://marc.info/?i=1384872527-26154-1-git-send-email-ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
>
> [3] "Botching up ioctls" by Daniel Vetter
>      http://blog.ffwll.ch/2013/11/botching-up-ioctls.html
>
> Yann Droneaud (7):
>    IB/core: const'ify inbuf in struct ib_udata
>    IB/uverbs: remove implicit cast in INIT_UDATA()
>    IB/uverbs: set outbuf to NULL when no core response space is provided
>    IB/uverbs: check reserved field in extended command header
>    IB/uverbs: check comp_mask in destroy_flow
>    IB/uverbs: check reserved fields in create_flow
>    IB/uverbs: set error code when fail to consume all flow_spec items
>
>   drivers/infiniband/core/uverbs.h      | 12 ++++++------
>   drivers/infiniband/core/uverbs_cmd.c  | 31 +++++++++++++++++++++----------
>   drivers/infiniband/core/uverbs_main.c | 16 +++++++++++-----
>   include/rdma/ib_verbs.h               |  2 +-
>   4 files changed, 39 insertions(+), 22 deletions(-)
>

Hi,

Great series Yann. Thanks for your contribution.

Best regards,
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