* [PATCH net] tcp: reset tp->sacked_out when sack is enabled
@ 2022-10-26 15:15 Lu Wei
2022-10-26 14:30 ` Eric Dumazet
0 siblings, 1 reply; 6+ messages in thread
From: Lu Wei @ 2022-10-26 15:15 UTC (permalink / raw)
To: edumazet, davem, yoshfuji, dsahern, kuba, pabeni, xemul, netdev,
linux-kernel
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@huawei.com>
---
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH net] tcp: reset tp->sacked_out when sack is enabled 2022-10-26 15:15 [PATCH net] tcp: reset tp->sacked_out when sack is enabled Lu Wei @ 2022-10-26 14:30 ` Eric Dumazet 2022-10-26 14:33 ` Eric Dumazet 0 siblings, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2022-10-26 14:30 UTC (permalink / raw) To: Lu Wei, Pavel Emelyanov Cc: davem, yoshfuji, dsahern, kuba, pabeni, xemul, netdev, linux-kernel On Wed, Oct 26, 2022 at 7:12 AM Lu Wei <luwei32@huawei.com> 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@huawei.com> > --- > 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 ? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] tcp: reset tp->sacked_out when sack is enabled 2022-10-26 14:30 ` Eric Dumazet @ 2022-10-26 14:33 ` Eric Dumazet 2022-10-26 18:59 ` Denis V. Lunev 0 siblings, 1 reply; 6+ messages in thread From: Eric Dumazet @ 2022-10-26 14:33 UTC (permalink / raw) To: Lu Wei, Denis V. Lunev Cc: davem, yoshfuji, dsahern, kuba, pabeni, xemul, netdev, linux-kernel On Wed, Oct 26, 2022 at 7:30 AM Eric Dumazet <edumazet@google.com> wrote: > > On Wed, Oct 26, 2022 at 7:12 AM Lu Wei <luwei32@huawei.com> 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@huawei.com> > > --- > > 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@openvz.org>, because Pavel's address no longer works. Thanks ! ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] tcp: reset tp->sacked_out when sack is enabled 2022-10-26 14:33 ` Eric Dumazet @ 2022-10-26 18:59 ` Denis V. Lunev 2022-10-26 19:59 ` Eric Dumazet [not found] ` <CA+XRDbMma1e26HHoYm2pExANACH8n83bE7vB_Gwh3-RZUsOe2g@mail.gmail.com> 0 siblings, 2 replies; 6+ messages in thread From: Denis V. Lunev @ 2022-10-26 18:59 UTC (permalink / raw) To: Eric Dumazet, Lu Wei Cc: davem, yoshfuji, dsahern, kuba, pabeni, netdev, linux-kernel, Pavel Emelianov (Gmail), avagin, Pavel Tikhomirov On 10/26/22 16:33, Eric Dumazet wrote: > On Wed, Oct 26, 2022 at 7:30 AM Eric Dumazet <edumazet@google.com> wrote: >> On Wed, Oct 26, 2022 at 7:12 AM Lu Wei <luwei32@huawei.com> 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@huawei.com> >>> --- >>> 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@openvz.org>, 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". Thank you in advance, Den ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net] tcp: reset tp->sacked_out when sack is enabled 2022-10-26 18:59 ` Denis V. Lunev @ 2022-10-26 19:59 ` Eric Dumazet [not found] ` <CA+XRDbMma1e26HHoYm2pExANACH8n83bE7vB_Gwh3-RZUsOe2g@mail.gmail.com> 1 sibling, 0 replies; 6+ messages in thread From: Eric Dumazet @ 2022-10-26 19:59 UTC (permalink / raw) To: Denis V. Lunev Cc: Lu Wei, davem, yoshfuji, dsahern, kuba, pabeni, netdev, linux-kernel, Pavel Emelianov (Gmail), avagin, Pavel Tikhomirov On Wed, Oct 26, 2022 at 11:59 AM Denis V. Lunev <den@virtuozzo.com> wrote: > > On 10/26/22 16:33, Eric Dumazet wrote: > > On Wed, Oct 26, 2022 at 7:30 AM Eric Dumazet <edumazet@google.com> wrote: > >> On Wed, Oct 26, 2022 at 7:12 AM Lu Wei <luwei32@huawei.com> 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@huawei.com> > >>> --- > >>> 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@openvz.org>, 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <CA+XRDbMma1e26HHoYm2pExANACH8n83bE7vB_Gwh3-RZUsOe2g@mail.gmail.com>]
[parent not found: <8083c822-1493-3596-d292-520fb16b113f@huawei.com>]
* Re: [PATCH net] tcp: reset tp->sacked_out when sack is enabled [not found] ` <8083c822-1493-3596-d292-520fb16b113f@huawei.com> @ 2022-10-27 12:04 ` Eric Dumazet 0 siblings, 0 replies; 6+ messages in thread From: Eric Dumazet @ 2022-10-27 12:04 UTC (permalink / raw) To: luwei (O) Cc: Pavel Emelyanov, Denis V. Lunev, davem, yoshfuji, dsahern, kuba, pabeni, netdev, linux-kernel, avagin, Pavel Tikhomirov On Thu, Oct 27, 2022 at 5:00 AM luwei (O) <luwei32@huawei.com> wrote: > > > thanks, I will send next version Yeah, what about first agreeing on what the plans are ? In order to avoid confusion and lkml/netdev bloat, I think you should describe why existing checks are not enough. Since this was probably caught by a fuzzer like syzbot, do you have a repro ? ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-10-27 12:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-26 15:15 [PATCH net] tcp: reset tp->sacked_out when sack is enabled Lu Wei
2022-10-26 14:30 ` Eric Dumazet
2022-10-26 14:33 ` Eric Dumazet
2022-10-26 18:59 ` Denis V. Lunev
2022-10-26 19:59 ` Eric Dumazet
[not found] ` <CA+XRDbMma1e26HHoYm2pExANACH8n83bE7vB_Gwh3-RZUsOe2g@mail.gmail.com>
[not found] ` <8083c822-1493-3596-d292-520fb16b113f@huawei.com>
2022-10-27 12:04 ` Eric Dumazet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®