Re: race in skb_splice_bits?

[Octavian Purdila <opurdila@ixiacom.com>]

[-- Attachment #1: Type: text/plain, Size: 740 bytes --]

On Tuesday 27 May 2008, Evgeniy Polyakov wrote:
>
> Please try attached patch on top of vanilla tree.
> It does not use skb after socket was dropped, but instead search it
> again when socket is locked, so if socket is alive, it will find it and
> clean otherwise it will exit.
>

This fixes the crash, thanks.

One doubt though: suppose that while we drop the lock the skb gets aggregated 
with the one after it. If the original skb is fully consumed in the receive 
actor, then the we will eat the new, aggregated skb, loosing data. 

Here is a patch, based on your idea, which tries to cope with the above 
scenario. The !skb check was added for the case in which the actor does not 
consume anything in the current interration. 

tavi


[-- Attachment #2: a.diff --]
[-- Type: text/x-diff, Size: 1040 bytes --]

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 0a9002b..0a0a663 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1358,7 +1358,8 @@ done:
 
 	if (spd.nr_pages) {
 		int ret;
-
+		struct sock *sk= __skb->sk;
+ 
 		/*
 		 * Drop the socket lock, otherwise we have reverse
 		 * locking dependencies between sk_lock and i_mutex
@@ -1368,9 +1369,9 @@ done:
 		 * we call into ->sendpage() with the i_mutex lock held
 		 * and networking will grab the socket lock.
 		 */
-		release_sock(__skb->sk);
+		release_sock(sk);
 		ret = splice_to_pipe(pipe, &spd);
-		lock_sock(__skb->sk);
+		lock_sock(sk);
 		return ret;
 	}
 
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 3e91b28..34049d0 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1227,7 +1227,8 @@ int tcp_read_sock(struct sock *sk, read_descriptor_t *desc,
 				copied += used;
 				offset += used;
 			}
-			if (offset != skb->len)
+			skb = tcp_recv_skb(sk, seq-1, &offset);
+			if (!skb || (offset+1 != skb->len))
 				break;
 		}
 		if (tcp_hdr(skb)->fin) {