lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <CADVnQyks4eWus9k5cZnZhVFS17r45RS8V776UgOkFhUF=HTS=A@mail.gmail.com> Date: Sat, 21 Oct 2023 19:57:02 -0400 From: Neal Cardwell <ncardwell@...gle.com> To: Fred Chen <fred.chenchen03@...il.com> Cc: edumazet@...gle.com, davem@...emloft.net, netdev@...r.kernel.org, yangpc@...gsu.com, ycheng@...gle.com Subject: Re: [PATCH v1] tcp: fix wrong RTO timeout when received SACK reneging On Fri, Oct 20, 2023 at 8:20 PM Fred Chen <fred.chenchen03@...il.com> wrote: > > This commit fix wrong RTO timeout when received SACK reneging. > > When an ACK arrived pointing to a SACK reneging, tcp_check_sack_reneging() > will rearm the RTO timer for min(1/2*srtt, 10ms) into to the future. > > But since the commit 62d9f1a6945b ("tcp: fix TLP timer not set when > CA_STATE changes from DISORDER to OPEN") merged, the tcp_set_xmit_timer() > is moved after tcp_fastretrans_alert()(which do the SACK reneging check), > so the RTO timeout will be overwrited by tcp_set_xmit_timer() with > icsk_rto instead of 1/2*srtt. > > Here is a packetdrill script to check this bug: > 0 socket(..., SOCK_STREAM, IPPROTO_TCP) = 3 > +0 bind(3, ..., ...) = 0 > +0 listen(3, 1) = 0 > > // simulate srtt to 100ms > +0 < S 0:0(0) win 32792 <mss 1000, sackOK,nop,nop,nop,wscale 7> > +0 > S. 0:0(0) ack 1 <mss 1460,nop,nop,sackOK,nop,wscale 7> > +.1 < . 1:1(0) ack 1 win 1024 > > +0 accept(3, ..., ...) = 4 > > +0 write(4, ..., 10000) = 10000 > +0 > P. 1:10001(10000) ack 1 > > // inject sack > +.1 < . 1:1(0) ack 1 win 257 <sack 1001:10001,nop,nop> > +0 > . 1:1001(1000) ack 1 > > // inject sack reneging > +.1 < . 1:1(0) ack 1001 win 257 <sack 9001:10001,nop,nop> > > // we expect rto fired in 1/2*srtt (50ms) > +.05 > . 1001:2001(1000) ack 1 > > This fix remove the FLAG_SET_XMIT_TIMER from ack_flag when > tcp_check_sack_reneging() set RTO timer with 1/2*srtt to avoid > being overwrited later. > > Fixes: 62d9f1a6945b ("tcp: fix TLP timer not set when CA_STATE changes from DISORDER to OPEN") > Signed-off-by: Fred Chen <fred.chenchen03@...il.com> > --- > net/ipv4/tcp_input.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index ab87f02..eee4e95 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -2222,16 +2222,17 @@ void tcp_enter_loss(struct sock *sk) > * restore sanity to the SACK scoreboard. If the apparent reneging > * persists until this RTO then we'll clear the SACK scoreboard. > */ > -static bool tcp_check_sack_reneging(struct sock *sk, int flag) > +static bool tcp_check_sack_reneging(struct sock *sk, int *ack_flag) > { > - if (flag & FLAG_SACK_RENEGING && > - flag & FLAG_SND_UNA_ADVANCED) { > + if (*ack_flag & FLAG_SACK_RENEGING && > + *ack_flag & FLAG_SND_UNA_ADVANCED) { > struct tcp_sock *tp = tcp_sk(sk); > unsigned long delay = max(usecs_to_jiffies(tp->srtt_us >> 4), > msecs_to_jiffies(10)); > > inet_csk_reset_xmit_timer(sk, ICSK_TIME_RETRANS, > delay, TCP_RTO_MAX); > + *ack_flag &= ~FLAG_SET_XMIT_TIMER; > return true; > } > return false; > @@ -3009,7 +3010,7 @@ static void tcp_fastretrans_alert(struct sock *sk, const u32 prior_snd_una, > tp->prior_ssthresh = 0; > > /* B. In all the states check for reneging SACKs. */ > - if (tcp_check_sack_reneging(sk, flag)) > + if (tcp_check_sack_reneging(sk, ack_flag)) > return; > > /* C. Check consistency of the current state. */ > -- Thanks a lot for the fix! The code looks good to me, and I ran it through our internal packetdrill test suite, and, with a few expected tweaks to reflect the fix, the tests all pass. Reviewed-by: Neal Cardwell <ncardwell@...gle.com> Tested-by: Neal Cardwell <ncardwell@...gle.com> thanks, neal
Powered by blists - more mailing lists