Re: [PATCH net] tcp: fix tcp_mtu_probe() vs highest_sack

On Mon, Oct 30, 2017 at 11:21:42PM -0700, Eric Dumazet wrote:

On Mon, 2017-10-30 at 23:17 -0700, Alexei Starovoitov wrote:

quoted

On Mon, Oct 30, 2017 at 11:08:20PM -0700, Eric Dumazet wrote:

quoted

From: Eric Dumazet <edumazet@google.com>

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 <redacted>
Reported-by: Roman Gushchin <redacted>
Reported-by: Oleksandr Natalenko <redacted>

Thanks!

Acked-by: Alexei Starovoitov <ast@kernel.org>

wow. a bug from 2007.
Any idea why it only started to bite us in 4.11 ?

It's not trivial for us to reproduce it, but we will definitely
test the patch as soon as we can.
Do you have packet drill test or something for easy repro?

I tried to cook a packetdrill test but could not trigger the issue.

When have you started to enable mtu probing ?

for some time. somehow 4.6 based kernel didn't trigger it.
May be it's a different bug still...