Re: [PATCH net] tcp: reset tp->sacked_out when sack is enabled

[Eric Dumazet]

On Wed, Oct 26, 2022 at 11:59 AM Denis V. Lunev <den@xxxxxxxxxxxxx> wrote:
>
> On 10/26/22 16:33, Eric Dumazet wrote:
> > On Wed, Oct 26, 2022 at 7:30 AM Eric Dumazet <edumazet@xxxxxxxxxx> wrote:
> >> On Wed, Oct 26, 2022 at 7:12 AM Lu Wei <luwei32@xxxxxxxxxx> wrote:
> >>> The meaning of tp->sacked_out depends on whether sack is enabled
> >>> or not. If setsockopt is called to enable sack_ok via
> >>> tcp_repair_options_est(), tp->sacked_out should be cleared, or it
> >>> will trigger warning in tcp_verify_left_out as follows:
> >>>
> >>> ============================================
> >>> WARNING: CPU: 8 PID: 0 at net/ipv4/tcp_input.c:2132
> >>> tcp_timeout_mark_lost+0x154/0x160
> >>> tcp_enter_loss+0x2b/0x290
> >>> tcp_retransmit_timer+0x50b/0x640
> >>> tcp_write_timer_handler+0x1c8/0x340
> >>> tcp_write_timer+0xe5/0x140
> >>> call_timer_fn+0x3a/0x1b0
> >>> __run_timers.part.0+0x1bf/0x2d0
> >>> run_timer_softirq+0x43/0xb0
> >>> __do_softirq+0xfd/0x373
> >>> __irq_exit_rcu+0xf6/0x140
> >>>
> >>> This warning occurs in several steps:
> >>> Step1. If sack is not enabled, when server receives dup-ack,
> >>>         it calls tcp_add_reno_sack() to increase tp->sacked_out.
> >>>
> >>> Step2. Setsockopt() is called to enable sack
> >>>
> >>> Step3. The retransmit timer expires, it calls tcp_timeout_mark_lost()
> >>>         to increase tp->lost_out but not clear tp->sacked_out because
> >>>         sack is enabled and tcp_is_reno() is false.
> >>>
> >>> So tp->left_out is increased repeatly in Step1 and Step3 and it is
> >>> greater than tp->packets_out and trigger the warning. In function
> >>> tcp_timeout_mark_lost(), tp->sacked_out will be cleared if Step2 not
> >>> happen and the warning will not be triggered. So this patch clears
> >>> tp->sacked_out in tcp_repair_options_est().
> >>>
> >>> Fixes: b139ba4e90dc ("tcp: Repair connection-time negotiated parameters")
> >>> Signed-off-by: Lu Wei <luwei32@xxxxxxxxxx>
> >>> ---
> >>>   net/ipv4/tcp.c | 3 +++
> >>>   1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> >>> index ef14efa1fb70..188d5c0e440f 100644
> >>> --- a/net/ipv4/tcp.c
> >>> +++ b/net/ipv4/tcp.c
> >>> @@ -3282,6 +3282,9 @@ static int tcp_repair_options_est(struct sock *sk, sockptr_t optbuf,
> >>>                          if (opt.opt_val != 0)
> >>>                                  return -EINVAL;
> >>>
> >>> +                       if (tcp_is_reno(tp))
> >>> +                               tp->sacked_out = 0;
> >>> +
> >>>                          tp->rx_opt.sack_ok |= TCP_SACK_SEEN;
> >>>                          break;
> >>>                  case TCPOPT_TIMESTAMP:
> >>> --
> >>> 2.31.1
> >>>
> >> Hmm, I am not sure this is the right fix.
> >>
> >> Probably TCP_REPAIR_OPTIONS should not be allowed if data has already been sent.
> >>
> >> Pavel, what do you think ?
> > Routing to Denis V. Lunev <den@xxxxxxxxxx>, because Pavel's address no
> > longer works.
> >
> > Thanks !
> Hi, guys!
>
> This code is used in CRIU. I have added CRIU maintainers
> Andrey Vagin, Pavel Tikhomirov and new address of Pavel
> Emelyanov to CC list.
>
> Here is the quote from Pavel Tikhomirov on the topic.
> "We do setsockopt with TCP_REPAIR_OPTIONS in CRIU just
> after calling connect to the socket here
> https://github.com/checkpoint-restore/criu/blob/18c6426eaeebc5fe7d0f9ca0acb592a3ec828b0c/soccr/soccr.c#L566
>
> and before libsoccr_restore_queue.
>
> So it seems there should be no data sent in this socket at
> the moment, so I believe it is safe to prohibit
> TCP_REPAIR_OPTIONS if data was already sent.
>
> Though I'd recomend running some CRIU tests after this
> change just to be sure that we don't break it.
> E.g.: "zdtm/static/socket-tcp*" or just
> "./test/zdtm.py run -a --keep-going --ignore-taint".

Thanks for confirming my suspicion.

Please Lu submit a different patch.

Your patch is only addressing the immediate issue (a WARNNG), but
should really be preventing
future bug reports because fuzzers will hit other points in the stack,
not expecting fundamental
TCP options being flipped in the middle of a connection.

Thanks

>
> Thank you in advance,
>      Den