Re: [PATCH 8/14] [TIPC] Fix socket receive queue NULL pointer dereference on SMP systems

[David Miller]

From: Per Liden [EMAIL PROTECTED]
Date: Fri, 13 Oct 2006 13:37:49 +0200

 From: P Litov [EMAIL PROTECTED]
 
 This patch corrects an SMP system-specific race condition which allowed
 TIPC to prematurely dereference the first sk_buff in a socket receive
 queue that was changing from empty to non-empty state.
 
 Signed-off-by: Allan Stephens [EMAIL PROTECTED]
 Signed-off-by: Per Liden [EMAIL PROTECTED]

If you are going to access the socket packet without some other kind
of locking that prevents changes to the queue, you must take the skb
queue lock.  You can't dance around it by checking the linked list
pointer instead the queue length.  Otherwise we'd be doing this all
over the UDP code and other datagram socket layers.  And we don't
because it simply isn't valid.

So I'm not applying this.

Also, this patch is missing a proper signed off line from the
patch author, P Litov.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 8/14] [TIPC] Fix socket receive queue NULL pointer dereference on SMP systems

[Per Liden]

From: P Litov [EMAIL PROTECTED]

This patch corrects an SMP system-specific race condition which allowed
TIPC to prematurely dereference the first sk_buff in a socket receive
queue that was changing from empty to non-empty state.

Signed-off-by: Allan Stephens [EMAIL PROTECTED]
Signed-off-by: Per Liden [EMAIL PROTECTED]
---
 net/tipc/socket.c |   40 ++--
 1 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 2a6a5a6..827f204 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -82,6 +82,18 @@ static int sockets_enabled = 0;
 static atomic_t tipc_queue_size = ATOMIC_INIT(0);
 
 
+/*
+ * Notes on receive queue locking:
+ *
+ * 1) Routines called from an application thread that examine the socket 
+ * receive queue must typically use skb_queue_empty() rather than
+ * skb_queue_len() to determine if the queue has a message in it; otherwise,
+ * a race condition can arise on SMP systems wherein skb_queue_len() indicates
+ * the presence of a message that is not yet on the queue.  (This race could
+ * be avoided by taking the queue lock before examining the queue, but this
+ * would complicate the code and impact performance.)
+ */
+
 /* 
  * sock_lock(): Lock a port/socket pair. lock_sock() can 
  * not be used here, since the same lock must protect ports 
@@ -123,7 +135,7 @@ static u32 pollmask(struct socket *sock)
 {
u32 mask;
 
-   if ((skb_queue_len(sock-sk-sk_receive_queue) != 0) ||
+   if (!skb_queue_empty(sock-sk-sk_receive_queue) ||
(sock-state == SS_UNCONNECTED) ||
(sock-state == SS_DISCONNECTING))
mask = (POLLRDNORM | POLLIN);
@@ -809,7 +821,7 @@ static int recv_msg(struct kiocb *iocb, 
struct tipc_sock *tsock = tipc_sk(sock-sk);
struct sk_buff *buf;
struct tipc_msg *msg;
-   unsigned int q_len;
+   int q_empty;
unsigned int sz;
u32 err;
int res;
@@ -828,7 +840,7 @@ static int recv_msg(struct kiocb *iocb, 
if (unlikely(sock-state == SS_UNCONNECTED))
return -ENOTCONN;
if (unlikely((sock-state == SS_DISCONNECTING)  
-(skb_queue_len(sock-sk-sk_receive_queue) == 0)))
+(skb_queue_empty(sock-sk-sk_receive_queue
return -ENOTCONN;
}
 
@@ -838,7 +850,7 @@ static int recv_msg(struct kiocb *iocb, 
return -ERESTARTSYS;
 
 restart:
-   if (unlikely((skb_queue_len(sock-sk-sk_receive_queue) == 0) 
+   if (unlikely(skb_queue_empty(sock-sk-sk_receive_queue) 
 (flags  MSG_DONTWAIT))) {
res = -EWOULDBLOCK;
goto exit;
@@ -846,7 +858,7 @@ restart:
 
if ((res = wait_event_interruptible(
*sock-sk-sk_sleep, 
-   ((q_len = skb_queue_len(sock-sk-sk_receive_queue)) ||
+   (!(q_empty = skb_queue_empty(sock-sk-sk_receive_queue)) ||
 (sock-state == SS_DISCONNECTING))) )) {
goto exit;
}
@@ -854,7 +866,7 @@ restart:
/* Catch attempt to receive on an already terminated connection */
/* [THIS CHECK MAY OVERLAP WITH AN EARLIER CHECK] */
 
-   if (!q_len) {
+   if (q_empty) {
res = -ENOTCONN;
goto exit;
}
@@ -941,7 +953,7 @@ static int recv_stream(struct kiocb *ioc
struct tipc_sock *tsock = tipc_sk(sock-sk);
struct sk_buff *buf;
struct tipc_msg *msg;
-   unsigned int q_len;
+   int q_empty;
unsigned int sz;
int sz_to_copy;
int sz_copied = 0;
@@ -962,7 +974,7 @@ static int recv_stream(struct kiocb *ioc
return -EINVAL;
 
if (unlikely(sock-state == SS_DISCONNECTING)) {
-   if (skb_queue_len(sock-sk-sk_receive_queue) == 0)
+   if (skb_queue_empty(sock-sk-sk_receive_queue))
return -ENOTCONN;
} else if (unlikely(sock-state != SS_CONNECTED))
return -ENOTCONN;
@@ -973,7 +985,7 @@ static int recv_stream(struct kiocb *ioc
return -ERESTARTSYS;
 
 restart:
-   if (unlikely((skb_queue_len(sock-sk-sk_receive_queue) == 0) 
+   if (unlikely(skb_queue_empty(sock-sk-sk_receive_queue) 
 (flags  MSG_DONTWAIT))) {
res = -EWOULDBLOCK;
goto exit;
@@ -981,7 +993,7 @@ restart:
 
if ((res = wait_event_interruptible(
*sock-sk-sk_sleep, 
-   ((q_len = skb_queue_len(sock-sk-sk_receive_queue)) ||
+   (!(q_empty = skb_queue_empty(sock-sk-sk_receive_queue)) ||
 (sock-state == SS_DISCONNECTING))) )) {
goto exit;
}
@@ -989,7 +1001,7 @@ restart:
/* Catch attempt to receive on an already terminated connection */
/* [THIS CHECK MAY OVERLAP WITH AN EARLIER CHECK] */
 
-