diff mbox series

[net-next] tcp: tcp_fragment() should not assume rtx skbs

Message ID 1509671403.2849.38.camel@edumazet-glaptop3.roam.corp.google.com
State Accepted, archived
Delegated to: David Miller
Headers show
Series [net-next] tcp: tcp_fragment() should not assume rtx skbs | expand

Commit Message

Eric Dumazet Nov. 3, 2017, 1:10 a.m. UTC 
From: Eric Dumazet <edumazet@google.com>

While stress testing MTU probing, we had crashes in list_del() that we root-caused
to the fact that tcp_fragment() is unconditionally inserting the freshly allocated
skb into tsorted_sent_queue list.

But this list is supposed to contain skbs that were sent.
This was mostly harmless until MTU probing was enabled.

Fortunately we can use the tcp_queue enum added later (but in same linux version)
for rtx-rb-tree to fix the bug. 

Fixes: e2080072ed2d ("tcp: new list for sent but unacked skbs for RACK recovery")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Neal Cardwell <ncardwell@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Priyaranjan Jha <priyarjha@google.com>
---
 net/ipv4/tcp_output.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Neal Cardwell Nov. 3, 2017, 1:16 a.m. UTC | #1 
On Thu, Nov 2, 2017 at 9:10 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> While stress testing MTU probing, we had crashes in list_del() that we root-caused
> to the fact that tcp_fragment() is unconditionally inserting the freshly allocated
> skb into tsorted_sent_queue list.
>
> But this list is supposed to contain skbs that were sent.
> This was mostly harmless until MTU probing was enabled.
>
> Fortunately we can use the tcp_queue enum added later (but in same linux version)
> for rtx-rb-tree to fix the bug.
>
> Fixes: e2080072ed2d ("tcp: new list for sent but unacked skbs for RACK recovery")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Acked-by: Neal Cardwell <ncardwell@google.com>

Thanks, Eric!

neal
Soheil Hassas Yeganeh Nov. 3, 2017, 2:33 a.m. UTC | #2 
On Thu, Nov 2, 2017 at 9:16 PM, Neal Cardwell <ncardwell@google.com> wrote:
> On Thu, Nov 2, 2017 at 9:10 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>> From: Eric Dumazet <edumazet@google.com>
>>
>> While stress testing MTU probing, we had crashes in list_del() that we root-caused
>> to the fact that tcp_fragment() is unconditionally inserting the freshly allocated
>> skb into tsorted_sent_queue list.
>>
>> But this list is supposed to contain skbs that were sent.
>> This was mostly harmless until MTU probing was enabled.
>>
>> Fortunately we can use the tcp_queue enum added later (but in same linux version)
>> for rtx-rb-tree to fix the bug.
>>
>> Fixes: e2080072ed2d ("tcp: new list for sent but unacked skbs for RACK recovery")
>> Signed-off-by: Eric Dumazet <edumazet@google.com>
>
> Acked-by: Neal Cardwell <ncardwell@google.com>

Acked-by: Soheil Hassas Yeganeh <soheil@google.com>

Nice! Thank you, Eric!
David Miller Nov. 3, 2017, 7:03 a.m. UTC | #3 
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 02 Nov 2017 18:10:03 -0700

> From: Eric Dumazet <edumazet@google.com>
> 
> While stress testing MTU probing, we had crashes in list_del() that we root-caused
> to the fact that tcp_fragment() is unconditionally inserting the freshly allocated
> skb into tsorted_sent_queue list.
> 
> But this list is supposed to contain skbs that were sent.
> This was mostly harmless until MTU probing was enabled.
> 
> Fortunately we can use the tcp_queue enum added later (but in same linux version)
> for rtx-rb-tree to fix the bug. 
> 
> Fixes: e2080072ed2d ("tcp: new list for sent but unacked skbs for RACK recovery")
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied, thanks Eric.
diff mbox series

Patch

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index a85e8a282d173983e35a2a1e3135ca2a63f1699e..36a3e7c909caacd981b84d1d8820a33d922f5a4e 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1395,7 +1395,8 @@  int tcp_fragment(struct sock *sk, enum tcp_queue tcp_queue,
 	/* Link BUFF into the send queue. */
 	__skb_header_release(buff);
 	tcp_insert_write_queue_after(skb, buff, sk, tcp_queue);
-	list_add(&buff->tcp_tsorted_anchor, &skb->tcp_tsorted_anchor);
+	if (tcp_queue == TCP_FRAG_IN_RTX_QUEUE)
+		list_add(&buff->tcp_tsorted_anchor, &skb->tcp_tsorted_anchor);
 
 	return 0;
 }