tcp: fix tcp_mtu_probe() vs highest_sack

[ Upstream commit 2b7cda9c35d3b940eb9ce74b30bbd5eb30db493d ]

Based on SNMP values provided by Roman, Yuchung made the observation
that some crashes in tcp_sacktag_walk() might be caused by MTU probing.

Looking at tcp_mtu_probe(), I found that when a new skb was placed
in front of the write queue, we were not updating tcp highest sack.

If one skb is freed because all its content was copied to the new skb
(for MTU probing), then tp->highest_sack could point to a now freed skb.

Bad things would then happen, including infinite loops.

This patch renames tcp_highest_sack_combine() and uses it
from tcp_mtu_probe() to fix the bug.

Note that I also removed one test against tp->sacked_out,
since we want to replace tp->highest_sack regardless of whatever
condition, since keeping a stale pointer to freed skb is a recipe
for disaster.

Fixes: a47e5a988a57 ("[TCP]: Convert highest_sack to sk_buff to allow direct access")
Signed-off-by: Eric Dumazet <edumazet@google.com>
Reported-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Reported-by: Roman Gushchin <guro@fb.com>
Reported-by: Oleksandr Natalenko <oleksandr@natalenko.name>
Acked-by: Alexei Starovoitov <ast@kernel.org>
Acked-by: Neal Cardwell <ncardwell@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Sasha Levin <alexander.levin@verizon.com>

diff --git a/include/net/tcp.h b/include/net/tcp.h
index 3d3a365233f056176cd3cd588ec17b51fa6f5a3d..966d229d448284d796685e2143ec3421d31dfcb7 100644 (file)
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1535,12 +1535,12 @@ static inline void tcp_highest_sack_reset(struct sock *sk)
        tcp_sk(sk)->highest_sack = tcp_write_queue_head(sk);
 }
 
-/* Called when old skb is about to be deleted (to be combined with new skb) */
-static inline void tcp_highest_sack_combine(struct sock *sk,
+/* Called when old skb is about to be deleted and replaced by new skb */
+static inline void tcp_highest_sack_replace(struct sock *sk,
                                            struct sk_buff *old,
                                            struct sk_buff *new)
 {
-       if (tcp_sk(sk)->sacked_out && (old == tcp_sk(sk)->highest_sack))
+       if (old == tcp_highest_sack(sk))
                tcp_sk(sk)->highest_sack = new;
 }
 

diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index f23590a85ce4249d680968e1684c2f4f4130a107..70fb380da35868e2a030f8858f51a0e739d5c6a2 100644 (file)
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -1962,6 +1962,7 @@ static int tcp_mtu_probe(struct sock *sk)
        nskb->ip_summed = skb->ip_summed;
 
        tcp_insert_write_queue_before(nskb, skb, sk);
+       tcp_highest_sack_replace(sk, skb, nskb);
 
        len = 0;
        tcp_for_write_queue_from_safe(skb, next, sk) {
@@ -2465,7 +2466,7 @@ static void tcp_collapse_retrans(struct sock *sk, struct sk_buff *skb)
 
        BUG_ON(tcp_skb_pcount(skb) != 1 || tcp_skb_pcount(next_skb) != 1);
 
-       tcp_highest_sack_combine(sk, next_skb, skb);
+       tcp_highest_sack_replace(sk, next_skb, skb);
 
        tcp_unlink_write_queue(next_skb, sk);