Re: 2.6.24-rc4-mm1 - BUG in tcp_fragment

Cedric Le Goater
2007-12-14T06:52:59Z

Ilpo Järvinen wrote:
> On Thu, 13 Dec 2007, Cedric Le Goater wrote:
>
>> I got this one while compiling on NFS.
>>
>> C.
>>
>> kernel BUG at /home/legoater/linux/2.6.24-rc4-mm1/include/net/tcp.h:1480!
>
> I'm not exactly sure what patches you have applied and which patches are
> not, with rc4-mm1 there are two patches (first one was incomplete, I
> assume you had at least that one based on your other mail) to really fix
> the issues in (__|)tcp_reset_fack_counts(...).

Yes I only have the first patch you sent on lkml on top of 2.6.24-rc4-mm1.
attached below. I didn't see the second one on lkml ?

> However, there seems to be so much breakage that I have a bit trouble to
> decide where to start... The situation seems bit scary :-).

my n/w environment seems to reproduce these issues quite easily. if you
need some testing, just ping me.

Cheers,

C.

> So, I might soon prepare a revert patch for most of the questionable
> TCP parts and ask Dave to apply it (and drop them fully during next
> rebase) unless I suddently figure something out soon which explains
> all/most of the problems, then return to drawing board. ...As it seems
> that the cumulative ACK processing problem discovered later on (having
> rather cumbersome solution with skbs only) will make part of the work
> that's currently in net-2.6.25 quite useless/duplicate effort. But thanks
> anyway for reporting these.
>
>

Subject: [PATCH] [TCP]: Fix fack_count miscountings (multiple places)

1) Fack_count is set incorrectly if the highest sent skb is
already sacked (the skb->prev won't return it because it's on
the other list already). These manifest as fackets_out counting
error later on, the second-order effects are very hard to track,
so it may fix all out-standing TCP bug reports.

2) Prev == NULL check was wrong way around

3) Last skb's fack count was incorrectly skipped while() {} loop

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
---
include/net/tcp.h | 22 ++++++++++++++++------
1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 9dbed0b..11a7e3e 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1337,10 +1337,20 @@ static inline struct sk_buff *tcp_send_head(struct sock *sk)
static inline void tcp_advance_send_head(struct sock *sk, struct sk_buff *skb)
{
struct sk_buff *prev = tcp_write_queue_prev(sk, skb);
+ unsigned int fc = 0;
+
+ if (prev == (struct sk_buff *)&sk->sk_write_queue)
+ prev = NULL;
+ else if (!tcp_skb_adjacent(sk, prev, skb))
+ prev = NULL;

- if (prev != (struct sk_buff *)&sk->sk_write_queue)
- TCP_SKB_CB(skb)->fack_count = TCP_SKB_CB(prev)->fack_count +
- tcp_skb_pcount(prev);
+ if ((prev == NULL) && !__tcp_write_queue_empty(sk, TCP_WQ_SACKED))
+ prev = __tcp_write_queue_tail(sk, TCP_WQ_SACKED);
+
+ if (prev != NULL)
+ fc = TCP_SKB_CB(prev)->fack_count + tcp_skb_pcount(prev);
+
+ TCP_SKB_CB(skb)->fack_count = fc;

sk->sk_send_head = tcp_write_queue_next(sk, skb);
if (sk->sk_send_head == (struct sk_buff *)&sk->sk_write_queue)
@@ -1464,7 +1474,7 @@ static inline struct sk_buff *__tcp_reset_fack_counts(struct sock *sk,
{
unsigned int fc = 0;

- if (prev == NULL)
+ if (prev != NULL)
fc = TCP_SKB_CB(*prev)->fack_count + tcp_skb_pcount(*prev);

BUG_ON((*prev != NULL) && !tcp_skb_adjacent(sk, *prev, skb));
@@ -1521,7 +1531,7 @@ static inline void tcp_reset_fack_counts(struct sock *sk, struct sk_buff *inskb)
skb[otherq] = prev->next;
}

- while (skb[queue] != __tcp_write_queue_tail(sk, queue)) {
+ do {
/* Lazy find for the other queue */
if (skb[queue] == NULL) {
skb[queue] = tcp_write_queue_find(sk, TCP_SKB_CB(prev)->seq,
@@ -1535,7 +1545,7 @@ static inline void tcp_reset_fack_counts(struct sock *sk, struct sk_buff *inskb)
break;

queue ^= TCP_WQ_SACKED;
- }
+ } while (skb[queue] != __tcp_write_queue_tail(sk, queue));
}

static inline void __tcp_insert_write_queue_after(struct sk_buff *skb,
-- 1.5.0.6