Re: kernel warning in tcp_fragment

[Neal Cardwell <ncardwell@google.com>]

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

On Mon, Aug 10, 2015 at 2:10 PM, Jovi Zhangwei <jovi@cloudflare.com> wrote:
>
> Ping?
>
> We saw a lot of this warnings in our production system. It would be
> great appreciate if someone can give us the fix on this warnings. :)

What is your net.ipv4.tcp_mtu_probing setting? If 1, have you tried
setting it to 0? Previous reports (
https://patchwork.ozlabs.org/patch/480882/ ) have shown that this gets
rid of at least one source of the warning. So that would provide a
useful data point.

Separately, you could also try the attached patch. This is against
3.14.39. It tries to attack a different possible source of this
warning. Please let us know if that patch helps.

Thanks!

neal

[-- Attachment #2: 0001-RFC-for-tests-on-v3.14.39-tcp-resegment-skbs-that-we.patch --]
[-- Type: application/octet-stream, Size: 4569 bytes --]

From 28f179e004adcaa2397c77882836fd7111ef61aa Mon Sep 17 00:00:00 2001
From: Neal Cardwell <ncardwell@google.com>
Date: Fri, 29 May 2015 20:05:23 -0400
Subject: [PATCH] [RFC for tests on v3.14.39] tcp: resegment skbs that we mark
 un-SACKed due to reneging

[This patch is for Linux v3.14.39 and is for testing a proposed fix
for the issue reported in the netdev thread "Recurring trace from
tcp_fragment()" from May 29, 2015. A slightly different patch would be
needed for more recent kernels.]

If we are removing a SACK mark due to reneging then we should check to
see if the pcount needs to be sanitized, since tcp_shifted_skb()
can join together SACKed skbs in a way that makes their pcount
unrepresentative of the length of the packet.

This is aimed at fixing scenarios like the one where
tcp_mark_head_lost() calls tcp_fragment() and we fire the following
warning:

         if (WARN_ON(len > skb->len))
                return -EINVAL;

Here is a theory as to how this could happen...

Suppose the MSS=1000, for simplicity.

(1) send packet A, 1001 bytes, pcount 2
(2) send packet B, 1001 bytes, pcount 2
(3) receive SACK for A
(4) receive SACK for A and B, shift B onto A.

When we shift B onto A, tcp_shifted_skb() just adds the pcounts of A
and B, so now A's pcount is 2+2=4. But its skb->len is 1001+1001 =
2002 bytes.  Now normally we would expect an skb with a pcount of 4 to
have somewhere between 3*MSS+1byte and 4*MSS (between 3001 and 4000
bytes). And tcp_mark_head_lost() and tcp_match_skb_to_sack()
implicitly assume this.

Suppose there is then SACK reneging, and we remove the SACKed bit from
this weird skb A with pcount 4 and skb->len 2002.  Then we get more
SACKs for packets beyond A, and the loss-marking rules say we should
be able to mark 3 packets starting at A as lost.  Then we try to chop
3MSS worth of bytes off of packet A, which only has 2.002MSS of data.
And the warning fires.

Signed-off-by: Neal Cardwell <ncardwell@google.com>
---
 include/net/tcp.h     |  2 ++
 net/ipv4/tcp_input.c  |  9 ++++++++-
 net/ipv4/tcp_output.c | 14 ++++++++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 1f0d847..4464312 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -543,6 +543,8 @@ void tcp_xmit_retransmit_queue(struct sock *);
 void tcp_simple_retransmit(struct sock *);
 int tcp_trim_head(struct sock *, struct sk_buff *, u32);
 int tcp_fragment(struct sock *, struct sk_buff *, u32, unsigned int);
+int tcp_reset_skb_tso_segs(struct sock *sk, struct sk_buff *skb,
+			   unsigned int mss_now);
 
 void tcp_send_probe0(struct sock *);
 void tcp_send_partial(struct sock *);
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 2291791..804713b 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1915,6 +1915,7 @@ void tcp_enter_loss(struct sock *sk, int how)
 	struct tcp_sock *tp = tcp_sk(sk);
 	struct sk_buff *skb;
 	bool new_recovery = false;
+	bool was_sacked;
 
 	/* Reduce ssthresh if it has not yet been made inside this window. */
 	if (icsk->icsk_ca_state <= TCP_CA_Disorder ||
@@ -1949,11 +1950,17 @@ void tcp_enter_loss(struct sock *sk, int how)
 			tp->undo_marker = 0;
 
 		TCP_SKB_CB(skb)->sacked &= (~TCPCB_TAGBITS)|TCPCB_SACKED_ACKED;
-		if (!(TCP_SKB_CB(skb)->sacked&TCPCB_SACKED_ACKED) || how) {
+		was_sacked = TCP_SKB_CB(skb)->sacked & TCPCB_SACKED_ACKED;
+		if (!was_sacked || how) {
 			TCP_SKB_CB(skb)->sacked &= ~TCPCB_SACKED_ACKED;
 			TCP_SKB_CB(skb)->sacked |= TCPCB_LOST;
 			tp->lost_out += tcp_skb_pcount(skb);
 			tp->retransmit_high = TCP_SKB_CB(skb)->end_seq;
+
+			/* Clean up weird pcounts from tcp_shifted_skb(). */
+			if (was_sacked)
+				tcp_reset_skb_tso_segs(sk, skb,
+						       tcp_current_mss(sk));
 		}
 	}
 	tcp_verify_left_out(tp);
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index 96f64e5..74c8757 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1501,6 +1501,20 @@ static int tcp_init_tso_segs(const struct sock *sk, struct sk_buff *skb,
 	return tso_segs;
 }
 
+/* Recompute the TSO segmentation for an skb that was already sent. */
+int tcp_reset_skb_tso_segs(struct sock *sk, struct sk_buff *skb,
+			   unsigned int mss_now)
+{
+	int oldpcount = tcp_skb_pcount(skb);
+
+	if (skb_unclone(skb, GFP_ATOMIC))
+		return -ENOMEM;
+
+	tcp_set_skb_tso_segs(sk, skb, mss_now);
+	tcp_adjust_pcount(sk, skb, oldpcount - tcp_skb_pcount(skb));
+
+	return 0;
+}
 
 /* Return true if the Nagle test allows this packet to be
  * sent now.
-- 
2.2.0.rc0.207.ga3a616c