Re: 2.6.24-rc4-mm1 - BUG in tcp_fragment

[Cedric Le Goater]

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@xxxxxxxxxxx>
---
 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/