When we receive a D-SACK, where the sequence number satisfies:
undo_marker <= start_seq < end_seq <= prior_snd_una
we consider this is a valid D-SACK and tcp_is_sackblock_valid()
returns true, then this D-SACK is discarded as "old stuff",
but the variable first_sack_index is not marked as negative
in tcp_sacktag_write_queue().
If this D-SACK also carries a SACK that needs to be processed
(for example, the previous SACK segment was lost), this SACK
will be treated as a D-SACK in the following processing of
tcp_sacktag_write_queue(), which will eventually lead to
incorrect updates of undo_retrans and reordering.
Fixes: fd6dad616d4f ("[TCP]: Earlier SACK block verification & simplify access to them")
Signed-off-by: Pengcheng Yang <[email protected]>
---
net/ipv4/tcp_input.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 88b987c..0238b55 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1727,8 +1727,11 @@ static int tcp_sack_cache_ok(const struct tcp_sock *tp, const struct tcp_sack_bl
}
/* Ignore very old stuff early */
- if (!after(sp[used_sacks].end_seq, prior_snd_una))
+ if (!after(sp[used_sacks].end_seq, prior_snd_una)) {
+ if (i == 0)
+ first_sack_index = -1;
continue;
+ }
used_sacks++;
}
--
1.8.0
On Mon, Dec 30, 2019 at 1:55 AM Pengcheng Yang <[email protected]> wrote:
>
> When we receive a D-SACK, where the sequence number satisfies:
> undo_marker <= start_seq < end_seq <= prior_snd_una
> we consider this is a valid D-SACK and tcp_is_sackblock_valid()
> returns true, then this D-SACK is discarded as "old stuff",
> but the variable first_sack_index is not marked as negative
> in tcp_sacktag_write_queue().
>
> If this D-SACK also carries a SACK that needs to be processed
> (for example, the previous SACK segment was lost),
What do you mean by ' previous sack segment was lost' ?
this SACK
> will be treated as a D-SACK in the following processing of
> tcp_sacktag_write_queue(), which will eventually lead to
> incorrect updates of undo_retrans and reordering.
>
> Fixes: fd6dad616d4f ("[TCP]: Earlier SACK block verification & simplify access to them")
> Signed-off-by: Pengcheng Yang <[email protected]>
> ---
> net/ipv4/tcp_input.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 88b987c..0238b55 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -1727,8 +1727,11 @@ static int tcp_sack_cache_ok(const struct tcp_sock *tp, const struct tcp_sack_bl
> }
>
> /* Ignore very old stuff early */
> - if (!after(sp[used_sacks].end_seq, prior_snd_una))
> + if (!after(sp[used_sacks].end_seq, prior_snd_una)) {
> + if (i == 0)
> + first_sack_index = -1;
> continue;
> + }
>
> used_sacks++;
> }
Hi Pengcheng Yang
This corner case deserves a packetdrill test so that we understand the
issue, can you provide one ?
Thanks.
Hi Eric Dumazet,
Thanks for discussing this issue.
'previous sack segment was lost' means that the SACK segment carried by D-SACK
will be processed by tcp_sacktag_one () due to the previous SACK loss,
but this is not necessary.
Here is the packetdrill test, this example shows that the reordering was modified
because the SACK segment was treated as D-SACK.
//dsack-old-stuff-bug.pkt
// Verify the "old stuff" D-SACK causing SACK to be treated as D-SACK
--tolerance_usecs=10000
// enable RACK and TLP
0 `sysctl -q net.ipv4.tcp_recovery=1; sysctl -q net.ipv4.tcp_early_retrans=3`
// Establish a connection, rtt = 10ms
+0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0 bind(3, ..., ...) = 0
+0 listen(3, 1) = 0
+.1 < S 0:0(0) win 32792 <mss 1000,sackOK,nop,nop,nop,wscale 7>
+0 > S. 0:0(0) ack 1 <...>
+.01 < . 1:1(0) ack 1 win 320
+0 accept(3, ..., ...) = 4
// send 10 data segments
+0 write(4, ..., 10000) = 10000
+0 > P. 1:10001(10000) ack 1
// send TLP
+.02 > P. 9001:10001(1000) ack 1
// enter recovery and retransmit 1:1001, now undo_marker = 1
+.015 < . 1:1(0) ack 1 win 320 <sack 9001:10001, nop, nop>
+0 > . 1:1001(1000) ack 1
// ack 1:1001 and retransmit 1001:3001
+.01 < . 1:1001(1000) ack 1001 win 320 <sack 9001:10001, nop, nop>
+0 > . 1001:3001(2000) ack 1
// sack 2001:3001, now 2001:3001 has R|S
+.01 < . 1001:1001(0) ack 1001 win 320 <sack 2001:3001 9001:10001, nop, nop>
+0 %{ assert tcpi_reordering == 3, tcpi_reordering }%
// d-sack 1:1001, satisfies: undo_marker(1) <= start_seq < end_seq <= prior_snd_una(1001)
// BUG: 2001:3001 is treated as D-SACK then reordering is modified in tcp_sacktag_one()
+0 < . 1001:1001(0) ack 1001 win 320 <sack 1:1001 2001:3001 9001:10001, nop, nop>
// reordering was modified to 8
+0 %{ assert tcpi_reordering == 3, tcpi_reordering }%
-----邮件原件-----
发件人: Eric Dumazet <[email protected]>
发送时间: 2019年12月30日 21:41
收件人: Pengcheng Yang <[email protected]>
抄送: David Miller <[email protected]>; Alexey Kuznetsov <[email protected]>; Hideaki YOSHIFUJI <[email protected]>; Alexei Starovoitov <[email protected]>; Daniel Borkmann <[email protected]>; Martin KaFai Lau <[email protected]>; Song Liu <[email protected]>; Yonghong Song <[email protected]>; [email protected]; netdev <[email protected]>; LKML <[email protected]>
主题: Re: [PATCH] tcp: fix "old stuff" D-SACK causing SACK to be treated as D-SACK
On Mon, Dec 30, 2019 at 1:55 AM Pengcheng Yang <[email protected]> wrote:
>
> When we receive a D-SACK, where the sequence number satisfies:
> undo_marker <= start_seq < end_seq <= prior_snd_una
> we consider this is a valid D-SACK and tcp_is_sackblock_valid()
> returns true, then this D-SACK is discarded as "old stuff",
> but the variable first_sack_index is not marked as negative
> in tcp_sacktag_write_queue().
>
> If this D-SACK also carries a SACK that needs to be processed
> (for example, the previous SACK segment was lost),
What do you mean by ' previous sack segment was lost' ?
this SACK
> will be treated as a D-SACK in the following processing of
> tcp_sacktag_write_queue(), which will eventually lead to
> incorrect updates of undo_retrans and reordering.
>
> Fixes: fd6dad616d4f ("[TCP]: Earlier SACK block verification & simplify access to them")
> Signed-off-by: Pengcheng Yang <[email protected]>
> ---
> net/ipv4/tcp_input.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 88b987c..0238b55 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -1727,8 +1727,11 @@ static int tcp_sack_cache_ok(const struct tcp_sock *tp, const struct tcp_sack_bl
> }
>
> /* Ignore very old stuff early */
> - if (!after(sp[used_sacks].end_seq, prior_snd_una))
> + if (!after(sp[used_sacks].end_seq, prior_snd_una)) {
> + if (i == 0)
> + first_sack_index = -1;
> continue;
> + }
>
> used_sacks++;
> }
Hi Pengcheng Yang
This corner case deserves a packetdrill test so that we understand the
issue, can you provide one ?
Thanks.
Hi Eric Dumazet,
I'm sorry there was a slight error in the packetdrill test case of the previous email reply,
the ACK segment should not carry data, although this does not affect the description of the situation.
I fixed the packetdrill test and resent it as follows:
packetdrill test case:
// Verify the "old stuff" D-SACK causing SACK to be treated as D-SACK
--tolerance_usecs=10000
// enable RACK and TLP
0 `sysctl -q net.ipv4.tcp_recovery=1; sysctl -q net.ipv4.tcp_early_retrans=3`
// Establish a connection, rtt = 10ms
+0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0 bind(3, ..., ...) = 0
+0 listen(3, 1) = 0
+.1 < S 0:0(0) win 32792 <mss 1000,sackOK,nop,nop,nop,wscale 7>
+0 > S. 0:0(0) ack 1 <...>
+.01 < . 1:1(0) ack 1 win 320
+0 accept(3, ..., ...) = 4
// send 10 data segments
+0 write(4, ..., 10000) = 10000
+0 > P. 1:10001(10000) ack 1
// send TLP
+.02 > P. 9001:10001(1000) ack 1
// enter recovery and retransmit 1:1001, now undo_marker = 1
+.015 < . 1:1(0) ack 1 win 320 <sack 9001:10001, nop, nop>
+0 > . 1:1001(1000) ack 1
// ack 1:1001 and retransmit 1001:3001
+.01 < . 1:1(0) ack 1001 win 320 <sack 9001:10001, nop, nop>
+0 > . 1001:3001(2000) ack 1
// sack 2001:3001, now 2001:3001 has R|S
+.01 < . 1:1(0) ack 1001 win 320 <sack 2001:3001 9001:10001, nop, nop>
+0 %{ assert tcpi_reordering == 3, tcpi_reordering }%
// d-sack 1:1001, satisfies: undo_marker(1) <= start_seq < end_seq <= prior_snd_una(1001)
// BUG: 2001:3001 is treated as D-SACK then reordering is modified in tcp_sacktag_one()
+0 < . 1:1(0) ack 1001 win 320 <sack 1:1001 2001:3001 9001:10001, nop, nop>
// reordering was modified to 8
+0 %{ assert tcpi_reordering == 3, tcpi_reordering }%
-----邮件原件-----
发件人: 杨鹏程 <[email protected]>
发送时间: 2020年1月1日 19:47
收件人: 'Eric Dumazet' <[email protected]>
抄送: 'David Miller' <[email protected]>; 'Alexey Kuznetsov' <[email protected]>; 'Hideaki YOSHIFUJI' <[email protected]>; 'Alexei Starovoitov' <[email protected]>; 'Daniel Borkmann' <[email protected]>; 'Martin KaFai Lau' <[email protected]>; 'Song Liu' <[email protected]>; 'Yonghong Song' <[email protected]>; '[email protected]' <[email protected]>; 'netdev' <[email protected]>; 'LKML' <[email protected]>
主题: Re: [PATCH] tcp: fix "old stuff" D-SACK causing SACK to be treated as D-SACK
Hi Eric Dumazet,
Thanks for discussing this issue.
'previous sack segment was lost' means that the SACK segment carried by D-SACK will be processed by tcp_sacktag_one () due to the previous SACK loss, but this is not necessary.
Here is the packetdrill test, this example shows that the reordering was modified because the SACK segment was treated as D-SACK.
//dsack-old-stuff-bug.pkt
// Verify the "old stuff" D-SACK causing SACK to be treated as D-SACK
--tolerance_usecs=10000
// enable RACK and TLP
0 `sysctl -q net.ipv4.tcp_recovery=1; sysctl -q net.ipv4.tcp_early_retrans=3`
// Establish a connection, rtt = 10ms
+0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
+0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
+0 bind(3, ..., ...) = 0
+0 listen(3, 1) = 0
+.1 < S 0:0(0) win 32792 <mss 1000,sackOK,nop,nop,nop,wscale 7>
+0 > S. 0:0(0) ack 1 <...>
+.01 < . 1:1(0) ack 1 win 320
+0 accept(3, ..., ...) = 4
// send 10 data segments
+0 write(4, ..., 10000) = 10000
+0 > P. 1:10001(10000) ack 1
// send TLP
+.02 > P. 9001:10001(1000) ack 1
// enter recovery and retransmit 1:1001, now undo_marker = 1
+.015 < . 1:1(0) ack 1 win 320 <sack 9001:10001, nop, nop>
+0 > . 1:1001(1000) ack 1
// ack 1:1001 and retransmit 1001:3001
+.01 < . 1:1001(1000) ack 1001 win 320 <sack 9001:10001, nop, nop>
+0 > . 1001:3001(2000) ack 1
// sack 2001:3001, now 2001:3001 has R|S
+.01 < . 1001:1001(0) ack 1001 win 320 <sack 2001:3001 9001:10001, nop, nop>
+0 %{ assert tcpi_reordering == 3, tcpi_reordering }%
// d-sack 1:1001, satisfies: undo_marker(1) <= start_seq < end_seq <= prior_snd_una(1001) // BUG: 2001:3001 is treated as D-SACK then reordering is modified in tcp_sacktag_one()
+0 < . 1001:1001(0) ack 1001 win 320 <sack 1:1001 2001:3001 9001:10001, nop, nop>
// reordering was modified to 8
+0 %{ assert tcpi_reordering == 3, tcpi_reordering }%
-----邮件原件-----
发件人: Eric Dumazet <[email protected]>
发送时间: 2019年12月30日 21:41
收件人: Pengcheng Yang <[email protected]>
抄送: David Miller <[email protected]>; Alexey Kuznetsov <[email protected]>; Hideaki YOSHIFUJI <[email protected]>; Alexei Starovoitov <[email protected]>; Daniel Borkmann <[email protected]>; Martin KaFai Lau <[email protected]>; Song Liu <[email protected]>; Yonghong Song <[email protected]>; [email protected]; netdev <[email protected]>; LKML <[email protected]>
主题: Re: [PATCH] tcp: fix "old stuff" D-SACK causing SACK to be treated as D-SACK
On Mon, Dec 30, 2019 at 1:55 AM Pengcheng Yang <[email protected]> wrote:
>
> When we receive a D-SACK, where the sequence number satisfies:
> undo_marker <= start_seq < end_seq <= prior_snd_una we
> consider this is a valid D-SACK and tcp_is_sackblock_valid() returns
> true, then this D-SACK is discarded as "old stuff", but the variable
> first_sack_index is not marked as negative in
> tcp_sacktag_write_queue().
>
> If this D-SACK also carries a SACK that needs to be processed (for
> example, the previous SACK segment was lost),
What do you mean by ' previous sack segment was lost' ?
this SACK
> will be treated as a D-SACK in the following processing of
> tcp_sacktag_write_queue(), which will eventually lead to incorrect
> updates of undo_retrans and reordering.
>
> Fixes: fd6dad616d4f ("[TCP]: Earlier SACK block verification &
> simplify access to them")
> Signed-off-by: Pengcheng Yang <[email protected]>
> ---
> net/ipv4/tcp_input.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index
> 88b987c..0238b55 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -1727,8 +1727,11 @@ static int tcp_sack_cache_ok(const struct tcp_sock *tp, const struct tcp_sack_bl
> }
>
> /* Ignore very old stuff early */
> - if (!after(sp[used_sacks].end_seq, prior_snd_una))
> + if (!after(sp[used_sacks].end_seq, prior_snd_una)) {
> + if (i == 0)
> + first_sack_index = -1;
> continue;
> + }
>
> used_sacks++;
> }
Hi Pengcheng Yang
This corner case deserves a packetdrill test so that we understand the issue, can you provide one ?
Thanks.
On 1/1/20 11:48 PM, 杨鹏程 wrote:
> Hi Eric Dumazet,
>
> I'm sorry there was a slight error in the packetdrill test case of the previous email reply,
> the ACK segment should not carry data, although this does not affect the description of the situation.
> I fixed the packetdrill test and resent it as follows:
>
> packetdrill test case:
> // Verify the "old stuff" D-SACK causing SACK to be treated as D-SACK
> --tolerance_usecs=10000
>
> // enable RACK and TLP
> 0 `sysctl -q net.ipv4.tcp_recovery=1; sysctl -q net.ipv4.tcp_early_retrans=3`
>
> // Establish a connection, rtt = 10ms
> +0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
> +0 bind(3, ..., ...) = 0
> +0 listen(3, 1) = 0
>
> +.1 < S 0:0(0) win 32792 <mss 1000,sackOK,nop,nop,nop,wscale 7>
> +0 > S. 0:0(0) ack 1 <...>
> +.01 < . 1:1(0) ack 1 win 320
> +0 accept(3, ..., ...) = 4
>
> // send 10 data segments
> +0 write(4, ..., 10000) = 10000
> +0 > P. 1:10001(10000) ack 1
>
> // send TLP
> +.02 > P. 9001:10001(1000) ack 1
>
> // enter recovery and retransmit 1:1001, now undo_marker = 1
> +.015 < . 1:1(0) ack 1 win 320 <sack 9001:10001, nop, nop>
> +0 > . 1:1001(1000) ack 1
>
> // ack 1:1001 and retransmit 1001:3001
> +.01 < . 1:1(0) ack 1001 win 320 <sack 9001:10001, nop, nop>
> +0 > . 1001:3001(2000) ack 1
>
> // sack 2001:3001, now 2001:3001 has R|S
> +.01 < . 1:1(0) ack 1001 win 320 <sack 2001:3001 9001:10001, nop, nop>
>
> +0 %{ assert tcpi_reordering == 3, tcpi_reordering }%
>
> // d-sack 1:1001, satisfies: undo_marker(1) <= start_seq < end_seq <= prior_snd_una(1001)
> // BUG: 2001:3001 is treated as D-SACK then reordering is modified in tcp_sacktag_one()
> +0 < . 1:1(0) ack 1001 win 320 <sack 1:1001 2001:3001 9001:10001, nop, nop>
>
> // reordering was modified to 8
> +0 %{ assert tcpi_reordering == 3, tcpi_reordering }%
>
>
Very nice, thanks a lot for this test !
>
> -----邮件原件-----
> 发件人: 杨鹏程 <[email protected]>
> 发送时间: 2020年1月1日 19:47
> 收件人: 'Eric Dumazet' <[email protected]>
> 抄送: 'David Miller' <[email protected]>; 'Alexey Kuznetsov' <[email protected]>; 'Hideaki YOSHIFUJI' <[email protected]>; 'Alexei Starovoitov' <[email protected]>; 'Daniel Borkmann' <[email protected]>; 'Martin KaFai Lau' <[email protected]>; 'Song Liu' <[email protected]>; 'Yonghong Song' <[email protected]>; '[email protected]' <[email protected]>; 'netdev' <[email protected]>; 'LKML' <[email protected]>
> 主题: Re: [PATCH] tcp: fix "old stuff" D-SACK causing SACK to be treated as D-SACK
>
> Hi Eric Dumazet,
>
> Thanks for discussing this issue.
>
> 'previous sack segment was lost' means that the SACK segment carried by D-SACK will be processed by tcp_sacktag_one () due to the previous SACK loss, but this is not necessary.
>
> Here is the packetdrill test, this example shows that the reordering was modified because the SACK segment was treated as D-SACK.
>
> //dsack-old-stuff-bug.pkt
> // Verify the "old stuff" D-SACK causing SACK to be treated as D-SACK
> --tolerance_usecs=10000
>
> // enable RACK and TLP
> 0 `sysctl -q net.ipv4.tcp_recovery=1; sysctl -q net.ipv4.tcp_early_retrans=3`
>
> // Establish a connection, rtt = 10ms
> +0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3
> +0 setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
> +0 bind(3, ..., ...) = 0
> +0 listen(3, 1) = 0
>
> +.1 < S 0:0(0) win 32792 <mss 1000,sackOK,nop,nop,nop,wscale 7>
> +0 > S. 0:0(0) ack 1 <...>
> +.01 < . 1:1(0) ack 1 win 320
> +0 accept(3, ..., ...) = 4
>
> // send 10 data segments
> +0 write(4, ..., 10000) = 10000
> +0 > P. 1:10001(10000) ack 1
>
> // send TLP
> +.02 > P. 9001:10001(1000) ack 1
>
> // enter recovery and retransmit 1:1001, now undo_marker = 1
> +.015 < . 1:1(0) ack 1 win 320 <sack 9001:10001, nop, nop>
> +0 > . 1:1001(1000) ack 1
>
> // ack 1:1001 and retransmit 1001:3001
> +.01 < . 1:1001(1000) ack 1001 win 320 <sack 9001:10001, nop, nop>
> +0 > . 1001:3001(2000) ack 1
>
> // sack 2001:3001, now 2001:3001 has R|S
> +.01 < . 1001:1001(0) ack 1001 win 320 <sack 2001:3001 9001:10001, nop, nop>
>
> +0 %{ assert tcpi_reordering == 3, tcpi_reordering }%
>
> // d-sack 1:1001, satisfies: undo_marker(1) <= start_seq < end_seq <= prior_snd_una(1001) // BUG: 2001:3001 is treated as D-SACK then reordering is modified in tcp_sacktag_one()
> +0 < . 1001:1001(0) ack 1001 win 320 <sack 1:1001 2001:3001 9001:10001, nop, nop>
>
> // reordering was modified to 8
> +0 %{ assert tcpi_reordering == 3, tcpi_reordering }%
>
>
>
>
> -----邮件原件-----
> 发件人: Eric Dumazet <[email protected]>
> 发送时间: 2019年12月30日 21:41
> 收件人: Pengcheng Yang <[email protected]>
> 抄送: David Miller <[email protected]>; Alexey Kuznetsov <[email protected]>; Hideaki YOSHIFUJI <[email protected]>; Alexei Starovoitov <[email protected]>; Daniel Borkmann <[email protected]>; Martin KaFai Lau <[email protected]>; Song Liu <[email protected]>; Yonghong Song <[email protected]>; [email protected]; netdev <[email protected]>; LKML <[email protected]>
> 主题: Re: [PATCH] tcp: fix "old stuff" D-SACK causing SACK to be treated as D-SACK
>
> On Mon, Dec 30, 2019 at 1:55 AM Pengcheng Yang <[email protected]> wrote:
>>
>> When we receive a D-SACK, where the sequence number satisfies:
>> undo_marker <= start_seq < end_seq <= prior_snd_una we
>> consider this is a valid D-SACK and tcp_is_sackblock_valid() returns
>> true, then this D-SACK is discarded as "old stuff", but the variable
>> first_sack_index is not marked as negative in
>> tcp_sacktag_write_queue().
>>
>> If this D-SACK also carries a SACK that needs to be processed (for
>> example, the previous SACK segment was lost),
>
> What do you mean by ' previous sack segment was lost' ?
>
> this SACK
>> will be treated as a D-SACK in the following processing of
>> tcp_sacktag_write_queue(), which will eventually lead to incorrect
>> updates of undo_retrans and reordering.
>>
>> Fixes: fd6dad616d4f ("[TCP]: Earlier SACK block verification &
>> simplify access to them")
>> Signed-off-by: Pengcheng Yang <[email protected]>
>> ---
>> net/ipv4/tcp_input.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index
>> 88b987c..0238b55 100644
>> --- a/net/ipv4/tcp_input.c
>> +++ b/net/ipv4/tcp_input.c
>> @@ -1727,8 +1727,11 @@ static int tcp_sack_cache_ok(const struct tcp_sock *tp, const struct tcp_sack_bl
>> }
>>
>> /* Ignore very old stuff early */
>> - if (!after(sp[used_sacks].end_seq, prior_snd_una))
>> + if (!after(sp[used_sacks].end_seq, prior_snd_una)) {
>> + if (i == 0)
>> + first_sack_index = -1;
>> continue;
>> + }
>>
>> used_sacks++;
>> }
>
>
> Hi Pengcheng Yang
>
> This corner case deserves a packetdrill test so that we understand the issue, can you provide one ?
>
> Thanks.
>
From: Pengcheng Yang <[email protected]>
Date: Mon, 30 Dec 2019 17:54:41 +0800
> When we receive a D-SACK, where the sequence number satisfies:
> undo_marker <= start_seq < end_seq <= prior_snd_una
> we consider this is a valid D-SACK and tcp_is_sackblock_valid()
> returns true, then this D-SACK is discarded as "old stuff",
> but the variable first_sack_index is not marked as negative
> in tcp_sacktag_write_queue().
>
> If this D-SACK also carries a SACK that needs to be processed
> (for example, the previous SACK segment was lost), this SACK
> will be treated as a D-SACK in the following processing of
> tcp_sacktag_write_queue(), which will eventually lead to
> incorrect updates of undo_retrans and reordering.
>
> Fixes: fd6dad616d4f ("[TCP]: Earlier SACK block verification & simplify access to them")
> Signed-off-by: Pengcheng Yang <[email protected]>
Applied and queued up for -stable, thank you.