diff mbox

[net-next,1/5] tipc: silence sparse warnings

Message ID 1380014868-2797-2-git-send-email-jon.maloy@ericsson.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Jon Maloy Sept. 24, 2013, 9:27 a.m. UTC 
From: Ying Xue <ying.xue@windriver.com>

Eliminate below sparse warnings:

net/tipc/link.c:1210:37: warning: cast removes address space of expression
net/tipc/link.c:1218:59: warning: incorrect type in argument 2 (different address spaces)
net/tipc/link.c:1218:59:    expected void const [noderef] <asn:1>*from
net/tipc/link.c:1218:59:    got unsigned char const [usertype] *[assigned] sect_crs
net/tipc/msg.c:96:61: warning: incorrect type in argument 3 (different address spaces)
net/tipc/msg.c:96:61:    expected void const *from
net/tipc/msg.c:96:61:    got void [noderef] <asn:1>*const iov_base
net/tipc/socket.c:341:49: warning: Using plain integer as NULL pointer
net/tipc/socket.c:1371:36: warning: Using plain integer as NULL pointer
net/tipc/socket.c:1694:57: warning: Using plain integer as NULL pointer

Signed-off-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Andreas Bofjäll <andreas.bofjall@ericsson.com>
Reviewed-by: Paul Gortmaker <paul.gortmaker@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
---
 net/tipc/link.c   |    5 +++--
 net/tipc/msg.c    |    4 ++--
 net/tipc/socket.c |    6 +++---
 3 files changed, 8 insertions(+), 7 deletions(-)

Comments

David Miller Sept. 27, 2013, 5:59 a.m. UTC | #1 
From: Jon Maloy <jon.maloy@ericsson.com>
Date: Tue, 24 Sep 2013 04:27:44 -0500

> From: Ying Xue <ying.xue@windriver.com>
> 
> Eliminate below sparse warnings:
> 
> net/tipc/link.c:1210:37: warning: cast removes address space of expression
> net/tipc/link.c:1218:59: warning: incorrect type in argument 2 (different address spaces)
> net/tipc/link.c:1218:59:    expected void const [noderef] <asn:1>*from
> net/tipc/link.c:1218:59:    got unsigned char const [usertype] *[assigned] sect_crs
> net/tipc/msg.c:96:61: warning: incorrect type in argument 3 (different address spaces)
> net/tipc/msg.c:96:61:    expected void const *from
> net/tipc/msg.c:96:61:    got void [noderef] <asn:1>*const iov_base
> net/tipc/socket.c:341:49: warning: Using plain integer as NULL pointer
> net/tipc/socket.c:1371:36: warning: Using plain integer as NULL pointer
> net/tipc/socket.c:1694:57: warning: Using plain integer as NULL pointer
> 
> Signed-off-by: Ying Xue <ying.xue@windriver.com>
> Signed-off-by: Andreas Bofjäll <andreas.bofjall@ericsson.com>
> Reviewed-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>

These warnings are not just for fun, and they are certainly not an
invitation to stick casts all over the place to make them go away.

They indicate real problems in the TIPC code.

There really are user pointers in the iovecs here, and that's why the
iov_base member is annotated with "__user".

These iovecs carry pointers that come from userspace via socket calls.

And you absolutely cannot pass user pointers to skb_copy_to_linear_data()
and friends as they access the source pointer using memcpy().

You have to use the proper interfaces for accessing userspace memory,
ones that have their arguments annotated with __user.

I'm not applying this series, sorry.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ying Xue Sept. 27, 2013, 8:01 a.m. UTC | #2 
On 09/27/2013 01:59 PM, David Miller wrote:
> From: Jon Maloy <jon.maloy@ericsson.com>
> Date: Tue, 24 Sep 2013 04:27:44 -0500
> 
>> From: Ying Xue <ying.xue@windriver.com>
>>
>> Eliminate below sparse warnings:
>>
>> net/tipc/link.c:1210:37: warning: cast removes address space of expression
>> net/tipc/link.c:1218:59: warning: incorrect type in argument 2 (different address spaces)
>> net/tipc/link.c:1218:59:    expected void const [noderef] <asn:1>*from
>> net/tipc/link.c:1218:59:    got unsigned char const [usertype] *[assigned] sect_crs
>> net/tipc/msg.c:96:61: warning: incorrect type in argument 3 (different address spaces)
>> net/tipc/msg.c:96:61:    expected void const *from
>> net/tipc/msg.c:96:61:    got void [noderef] <asn:1>*const iov_base
>> net/tipc/socket.c:341:49: warning: Using plain integer as NULL pointer
>> net/tipc/socket.c:1371:36: warning: Using plain integer as NULL pointer
>> net/tipc/socket.c:1694:57: warning: Using plain integer as NULL pointer
>>
>> Signed-off-by: Ying Xue <ying.xue@windriver.com>
>> Signed-off-by: Andreas Bofjäll <andreas.bofjall@ericsson.com>
>> Reviewed-by: Paul Gortmaker <paul.gortmaker@windriver.com>
>> Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
> 
> These warnings are not just for fun, and they are certainly not an
> invitation to stick casts all over the place to make them go away.
> 
> They indicate real problems in the TIPC code.
> 
> There really are user pointers in the iovecs here, and that's why the
> iov_base member is annotated with "__user".
> 
> These iovecs carry pointers that come from userspace via socket calls.
> 
> And you absolutely cannot pass user pointers to skb_copy_to_linear_data()
> and friends as they access the source pointer using memcpy().
> 

Good point!
It's better for us to use memcpy_fromiovecend() instead of
skb_copy_to_linear_data() and its friends.

We will submit another version to correct this error soon.

Regards,
Ying

> You have to use the proper interfaces for accessing userspace memory,
> ones that have their arguments annotated with __user.
> 
> I'm not applying this series, sorry.
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andreas Bofjäll Sept. 27, 2013, 8:15 a.m. UTC | #3 
On 09/27/2013 10:01 AM, Ying Xue wrote:
> Good point!
> It's better for us to use memcpy_fromiovecend() instead of
> skb_copy_to_linear_data() and its friends.
>
> We will submit another version to correct this error soon.

I could be wrong here, but I think you should also remove the entire 
cast on line 1210 in link.c:

sect_crs = msg_sect[curr_sect].iov_base;

There should be no reason to cast there since you made sect_crs into 
const unchar* __user and msg_sect[].iov_base is void* __user.

/Andreas
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ying Xue Sept. 27, 2013, 9:48 a.m. UTC | #4 
On 09/27/2013 04:15 PM, Andreas Bofjäll wrote:
> On 09/27/2013 10:01 AM, Ying Xue wrote:
>> Good point!
>> It's better for us to use memcpy_fromiovecend() instead of
>> skb_copy_to_linear_data() and its friends.
>>
>> We will submit another version to correct this error soon.
> 
> I could be wrong here, but I think you should also remove the entire
> cast on line 1210 in link.c:
> 
> sect_crs = msg_sect[curr_sect].iov_base;
> 
> There should be no reason to cast there since you made sect_crs into
> const unchar* __user and msg_sect[].iov_base is void* __user.
> 

You are right, thank you for your nice reminder!

Regards,
Ying

> /Andreas
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jon Maloy Sept. 27, 2013, 10:25 a.m. UTC | #5 
On 09/27/2013 05:48 AM, Ying Xue wrote:
> On 09/27/2013 04:15 PM, Andreas Bofjäll wrote:
>> On 09/27/2013 10:01 AM, Ying Xue wrote:
>>> Good point!
>>> It's better for us to use memcpy_fromiovecend() instead of
>>> skb_copy_to_linear_data() and its friends.
>>>
>>> We will submit another version to correct this error soon.
Can you fix this asap, so I can re-post it? And, let's continue the thread
in tipc-discussion only until we (incl Paul) are happy with this patch.

///jon

>> I could be wrong here, but I think you should also remove the entire
>> cast on line 1210 in link.c:
>>
>> sect_crs = msg_sect[curr_sect].iov_base;
>>
>> There should be no reason to cast there since you made sect_crs into
>> const unchar* __user and msg_sect[].iov_base is void* __user.
>>
> You are right, thank you for your nice reminder!
>
> Regards,
> Ying
>
>> /Andreas
>>
>>

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/net/tipc/link.c b/net/tipc/link.c
index 0cc3d90..40521ae 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -1165,7 +1165,7 @@  static int link_send_sections_long(struct tipc_port *sender,
 	struct tipc_msg fragm_hdr;
 	struct sk_buff *buf, *buf_chain, *prev;
 	u32 fragm_crs, fragm_rest, hsz, sect_rest;
-	const unchar *sect_crs;
+	const unchar __user *sect_crs;
 	int curr_sect;
 	u32 fragm_no;
 	int res = 0;
@@ -1207,7 +1207,8 @@  again:
 
 		if (!sect_rest) {
 			sect_rest = msg_sect[++curr_sect].iov_len;
-			sect_crs = (const unchar *)msg_sect[curr_sect].iov_base;
+			sect_crs =
+			  (const unchar __user *)msg_sect[curr_sect].iov_base;
 		}
 
 		if (sect_rest < fragm_rest)
diff --git a/net/tipc/msg.c b/net/tipc/msg.c
index ced60e2..37cfb57 100644
--- a/net/tipc/msg.c
+++ b/net/tipc/msg.c
@@ -93,8 +93,8 @@  int tipc_msg_build(struct tipc_msg *hdr, struct iovec const *msg_sect,
 	skb_copy_to_linear_data(*buf, hdr, hsz);
 	for (res = 1, cnt = 0; res && (cnt < num_sect); cnt++) {
 		skb_copy_to_linear_data_offset(*buf, pos,
-					       msg_sect[cnt].iov_base,
-					       msg_sect[cnt].iov_len);
+			(const void __force *)msg_sect[cnt].iov_base,
+			msg_sect[cnt].iov_len);
 		pos += msg_sect[cnt].iov_len;
 	}
 	if (likely(res))
diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 6cc7ddd..0ff921d 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -338,7 +338,7 @@  static int release(struct socket *sock)
 		buf = __skb_dequeue(&sk->sk_receive_queue);
 		if (buf == NULL)
 			break;
-		if (TIPC_SKB_CB(buf)->handle != 0)
+		if (TIPC_SKB_CB(buf)->handle != NULL)
 			kfree_skb(buf);
 		else {
 			if ((sock->state == SS_CONNECTING) ||
@@ -1368,7 +1368,7 @@  static u32 filter_rcv(struct sock *sk, struct sk_buff *buf)
 		return TIPC_ERR_OVERLOAD;
 
 	/* Enqueue message */
-	TIPC_SKB_CB(buf)->handle = 0;
+	TIPC_SKB_CB(buf)->handle = NULL;
 	__skb_queue_tail(&sk->sk_receive_queue, buf);
 	skb_set_owner_r(buf, sk);
 
@@ -1691,7 +1691,7 @@  restart:
 		/* Disconnect and send a 'FIN+' or 'FIN-' message to peer */
 		buf = __skb_dequeue(&sk->sk_receive_queue);
 		if (buf) {
-			if (TIPC_SKB_CB(buf)->handle != 0) {
+			if (TIPC_SKB_CB(buf)->handle != NULL) {
 				kfree_skb(buf);
 				goto restart;
 			}