[PATCH] tcp: fix skb vs fack_count out-of-sync condition

["Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3777 bytes --]

On Thu, 29 May 2008, Andy Furniss wrote:

> ? wrote:
> > On Sat, 3 May 2008, Andy Furniss wrote:
> > 
> > > Ilpo J?rvinen wrote:
> > >
> > > > Since I've not yet found any other path that could lead to it except the
> > > > yesterday's false-positive, getting the write queue state more
> > > > accurately
> > > > captured by the debug patch on the very first occassion might help.
> > > > Here's
> > > > less spammy version of the debug patch (I added one printout as well to
> > > > get
> > > > more usefulness to transitional state reported).
> > > >
> > > OK, increased CONFIG_LOG_BUF_SHIFT from 14 to 18 and reversed old /
> > > applied
> > > new patch.
> > 
> > Any luck so far?
> > 
> 
> Finally - looks like I got two together.
> 
> It doesn't start with WARNING - but dmesg isn't full.

Ok, here's finally the fix to those WARN_ON(!tp->sacked_out && 
tp->fackets_out) WARNINGs, both stable-2.6.25 and linus' tree affected.

-- 
 i.

--
[PATCH] tcp: fix skb vs fack_count out-of-sync condition

This bug is able to corrupt fackets_out in very rare cases.
In order for this to cause corruption:
  1) DSACK in the middle of previous SACK block must be generated.
  2) In order to take that particular branch, part or all of the
     DSACKed segment must already be SACKed so that we have that
     in cache in the first place.
  3) The new info must be top enough so that fackets_out will be
     updated on this iteration.
...then fack_count is updated while skb wasn't, then we walk again
that particular segment thus updating fack_count twice for
a single skb and finally that value is assigned to fackets_out
by tcp_sacktag_one.

It is safe to call tcp_sacktag_one just once for a segment (at
DSACK), no need to call again for plain SACK.

Potential problem of the miscount are limited to premature entry
to recovery and to inflated reordering metric (which could even
cancel each other out in the most the luckiest scenarios :-)).
Both are quite insignificant in worst case too and there exists
also code to reset them (fackets_out once sacked_out becomes zero
and reordering metric on RTO).

This has been reported by a number of people, because it occurred
quite rarely, it has been very evasive. Andy Furniss was able to
get it to occur couple of times so that a bit more info was
collected about the problem using a debug patch, though it still
required lot of checking around. Thanks also to others who have
tried to help here.

This is listed as Bugzilla #10346. The bug was introduced by
me in commit 68f8353b48 ([TCP]: Rewrite SACK block processing & 
sack_recv_cache use), I probably thought back then that there's
need to scan that entry twice or didn't dare to make it go
through it just once there. Going through twice would have
required restoring fack_count after the walk but as noted above,
I chose to drop the additional walk step altogether here.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
Cc: int 986 <int986@gmail.com>
Cc: Andy Furniss <lists@andyfurniss.entadsl.com>
Cc: Brian Vowell <brian.vowell@gmail.com>
---
 net/ipv4/tcp_input.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 5d5b08b..a00c7f8 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1392,9 +1392,9 @@ static struct sk_buff *tcp_maybe_skipping_dsack(struct sk_buff *skb,
 
 	if (before(next_dup->start_seq, skip_to_seq)) {
 		skb = tcp_sacktag_skip(skb, sk, next_dup->start_seq, fack_count);
-		tcp_sacktag_walk(skb, sk, NULL,
-				 next_dup->start_seq, next_dup->end_seq,
-				 1, fack_count, reord, flag);
+		skb = tcp_sacktag_walk(skb, sk, NULL,
+				     next_dup->start_seq, next_dup->end_seq,
+				     1, fack_count, reord, flag);
 	}
 
 	return skb;
-- 
1.5.2.2