Re: drivers/infiniband/core/uverbs_main.c:683:17: sparse: Using plain integer as NULL pointer

Re: drivers/infiniband/core/uverbs_main.c:683:17: sparse: Using plain integer as NULL pointer

[Yann Droneaud]

Hi, 

Just received this warning[1] report:

Le mardi 19 novembre 2013 à 18:12 +0800, kbuild test robot a écrit :
> tree:   git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   801a76050bcf8d4e500eb8d048ff6265f37a61c8
> commit: f21519b23c1b6fa25366be4114ccf7fcf1c190f9 IB/core: extended command: an improved infrastructure for uverbs commands
> date:   2 days ago
> reproduce: make C=1 CF=-D__CHECK_ENDIAN__
> 
> 
> sparse warnings: (new ones prefixed by >>)
> 
> >> drivers/infiniband/core/uverbs_main.c:683:17: sparse: Using plain integer as NULL pointer
> >> drivers/infiniband/core/uverbs_main.c:689:17: sparse: Using plain integer as NULL pointer
> 
> vim +683 drivers/infiniband/core/uverbs_main.c
> 
>    677					return -EINVAL;
>    678			} else {
>    679				if (hdr.out_words || ex_hdr.provider_out_words)
>    680					return -EINVAL;
>    681			}
>    682	
>  > 683			INIT_UDATA(&ucore,
>    684				   (hdr.in_words) ? buf : 0,
>    685				   (unsigned long)ex_hdr.response,
>    686				   hdr.in_words * 8,
>    687				   hdr.out_words * 8);
>    688	
>  > 689			INIT_UDATA(&uhw,
>    690				   (ex_hdr.provider_in_words) ? buf + ucore.inlen : 0,
>    691				   (ex_hdr.provider_out_words) ? (unsigned long)ex_hdr.response + ucore.outlen : 0,
>    692				   ex_hdr.provider_in_words * 8,
> 

This warning is not reported by compiler (gcc) since an explicit cast is
present in INIT_UDATA(): see drivers/infiniband/core/uverbs.h[2]:

#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)


I'm going to submit a patch to fix the sparse warning by removing
implicit cast, adding explicit casts where appropriate and replace
0 by NULL in uverbs_main.c

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

[2] uverbs.h
http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/drivers/infiniband/core/uverbs.h#n50

> ---
> 0-DAY kernel build testing backend              Open Source Technology Center
> http://lists.01.org/mailman/listinfo/kbuild                 Intel Corporation

-- 
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-next 0/2] Fix "drivers/infiniband/core/uverbs_main.c:683:17: sparse: Using plain integer as NULL pointer" warning

[Yann Droneaud]

Please find two patches to fix the issue reported by kbuild test
robot[1].

The first patch was already sent[2] as part of my on going
work to improve checks on commands[3].

The second patch address the sparse warning while allowing
warnings to be reported by the compiler (and that's why first
patch is needed, so it seems to me it's quite useful to have such
warnings and code should not hide them from the developer).

[1] https://lists.01.org/pipermail/kbuild-all/2013-November/002120.html
    http://marc.info/?i=1384866781.20207.7.camel-bi+AKbBUZKY6gyzm1THtWbp2dZbC/Bob@public.gmane.org

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

[3] http://mid.gmane.org/cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
    http://marc.info/?i=cover.1376847403.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org

Yann Droneaud (2):
  IB/core: const'ify inbuf in struct ib_udata
  IB/uverbs: remove implicit cast in INIT_UDATA()

 drivers/infiniband/core/uverbs.h      | 12 ++++++------
 drivers/infiniband/core/uverbs_cmd.c  | 20 ++++++++++----------
 drivers/infiniband/core/uverbs_main.c | 13 ++++++++-----
 include/rdma/ib_verbs.h               |  2 +-
 4 files changed, 25 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-next 1/2] IB/core: const'ify inbuf in struct ib_udata

[Yann Droneaud]

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

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

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1384869925.git.ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org
Link: http://mid.gmane.org/cover.1376847403.git.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 bdc842e..9879568 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 979874c..61e1935 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-next 2/2] 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:

        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

Signed-off-by: Yann Droneaud <ydroneaud-RlY5vtjFyJ3QT0dZR+AlfA@public.gmane.org>
Link: http://marc.info/?i=cover.1384869925.git.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 9879568..0dca197 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 65f6e7d..d9d91c4 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 3438694..14d8643 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