| Message ID | 20240716015401.2365503-5-edumazet@google.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tcp: stable backports for CVE-2024-41007 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
Hi Greg, Eric, all,
I noticed this in stable-rc/queue and stable-rc/linux- for 6.1 and 6.6:
net/ipv4/tcp_timer.c:472:7: error: variable 'rtx_delta' is uninitialized when used here [-Werror,-Wuninitialized]
if (rtx_delta > user_timeout)
^~~~~~~~~
net/ipv4/tcp_timer.c:464:15: note: initialize the variable 'rtx_delta' to silence this warning
u32 rtx_delta;
^
= 0
I hope that helps!
Cheers,
Miguel
On Tue, Jul 16, 2024 at 7:10 PM Miguel Ojeda <ojeda@kernel.org> wrote: > > Hi Greg, Eric, all, > > I noticed this in stable-rc/queue and stable-rc/linux- for 6.1 and 6.6: > > net/ipv4/tcp_timer.c:472:7: error: variable 'rtx_delta' is uninitialized when used here [-Werror,-Wuninitialized] > if (rtx_delta > user_timeout) > ^~~~~~~~~ > net/ipv4/tcp_timer.c:464:15: note: initialize the variable 'rtx_delta' to silence this warning > u32 rtx_delta; > ^ > = 0 > > I hope that helps! Thanks for the report! I think it missed one small snippet of code from [1] compared to the latest kernel. We can init this part before using it, something like this: + rtx_delta = (u32)msecs_to_jiffies(tcp_time_stamp(tp) - + (tp->retrans_stamp ?: tcp_skb_timestamp(skb))); Note: fully untested. Since Eric is very busy, I decided to check and provide some useful information here. [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=614e8316aa4cafba3e204cb8ee48bd12b92f3d93 > > Cheers, > Miguel
On Tue, Jul 16, 2024 at 08:40:40PM +0800, Jason Xing wrote: > On Tue, Jul 16, 2024 at 7:10 PM Miguel Ojeda <ojeda@kernel.org> wrote: > > > > Hi Greg, Eric, all, > > > > I noticed this in stable-rc/queue and stable-rc/linux- for 6.1 and 6.6: > > > > net/ipv4/tcp_timer.c:472:7: error: variable 'rtx_delta' is uninitialized when used here [-Werror,-Wuninitialized] > > if (rtx_delta > user_timeout) > > ^~~~~~~~~ > > net/ipv4/tcp_timer.c:464:15: note: initialize the variable 'rtx_delta' to silence this warning > > u32 rtx_delta; > > ^ > > = 0 > > > > I hope that helps! > > Thanks for the report! > > I think it missed one small snippet of code from [1] compared to the > latest kernel. We can init this part before using it, something like > this: > > + rtx_delta = (u32)msecs_to_jiffies(tcp_time_stamp(tp) - > + (tp->retrans_stamp ?: tcp_skb_timestamp(skb))); > > Note: fully untested. > > Since Eric is very busy, I decided to check and provide some useful > information here. Thanks all, this was probably due to my manual backporting here, let me go check what went wrong...
On Tue, Jul 16, 2024 at 02:53:12PM +0200, Greg KH wrote: > On Tue, Jul 16, 2024 at 08:40:40PM +0800, Jason Xing wrote: > > On Tue, Jul 16, 2024 at 7:10 PM Miguel Ojeda <ojeda@kernel.org> wrote: > > > > > > Hi Greg, Eric, all, > > > > > > I noticed this in stable-rc/queue and stable-rc/linux- for 6.1 and 6.6: > > > > > > net/ipv4/tcp_timer.c:472:7: error: variable 'rtx_delta' is uninitialized when used here [-Werror,-Wuninitialized] > > > if (rtx_delta > user_timeout) > > > ^~~~~~~~~ > > > net/ipv4/tcp_timer.c:464:15: note: initialize the variable 'rtx_delta' to silence this warning > > > u32 rtx_delta; > > > ^ > > > = 0 > > > > > > I hope that helps! > > > > Thanks for the report! > > > > I think it missed one small snippet of code from [1] compared to the > > latest kernel. We can init this part before using it, something like > > this: > > > > + rtx_delta = (u32)msecs_to_jiffies(tcp_time_stamp(tp) - > > + (tp->retrans_stamp ?: tcp_skb_timestamp(skb))); > > > > Note: fully untested. > > > > Since Eric is very busy, I decided to check and provide some useful > > information here. > > Thanks all, this was probably due to my manual backporting here, let me > go check what went wrong... Yeah, this is my fault, due to 614e8316aa4c ("tcp: add support for usec resolution in TCP TS values") not being in the tree, let me go rework things... thanks, greg k-h
On Tue, Jul 16, 2024 at 02:56:28PM +0200, Greg KH wrote: > On Tue, Jul 16, 2024 at 02:53:12PM +0200, Greg KH wrote: > > On Tue, Jul 16, 2024 at 08:40:40PM +0800, Jason Xing wrote: > > > On Tue, Jul 16, 2024 at 7:10 PM Miguel Ojeda <ojeda@kernel.org> wrote: > > > > > > > > Hi Greg, Eric, all, > > > > > > > > I noticed this in stable-rc/queue and stable-rc/linux- for 6.1 and 6.6: > > > > > > > > net/ipv4/tcp_timer.c:472:7: error: variable 'rtx_delta' is uninitialized when used here [-Werror,-Wuninitialized] > > > > if (rtx_delta > user_timeout) > > > > ^~~~~~~~~ > > > > net/ipv4/tcp_timer.c:464:15: note: initialize the variable 'rtx_delta' to silence this warning > > > > u32 rtx_delta; > > > > ^ > > > > = 0 > > > > > > > > I hope that helps! > > > > > > Thanks for the report! > > > > > > I think it missed one small snippet of code from [1] compared to the > > > latest kernel. We can init this part before using it, something like > > > this: > > > > > > + rtx_delta = (u32)msecs_to_jiffies(tcp_time_stamp(tp) - > > > + (tp->retrans_stamp ?: tcp_skb_timestamp(skb))); > > > > > > Note: fully untested. > > > > > > Since Eric is very busy, I decided to check and provide some useful > > > information here. > > > > Thanks all, this was probably due to my manual backporting here, let me > > go check what went wrong... > > Yeah, this is my fault, due to 614e8316aa4c ("tcp: add support for usec > resolution in TCP TS values") not being in the tree, let me go rework > things... Ok, backporting that commit is not going to happen, that's crazy... Anyway, the diff below is what I made on top of the existing one, which should be doing the right thing. But ideally someone can test this, somehow... I'll push out -rc releases later today so that people can pound on it easier. thanks for the review! greg k-h --- a/net/ipv4/tcp_timer.c +++ b/net/ipv4/tcp_timer.c @@ -464,6 +464,9 @@ static bool tcp_rtx_probe0_timed_out(con u32 rtx_delta; s32 rcv_delta; + rtx_delta = (u32)msecs_to_jiffies(tcp_time_stamp(tp) - + (tp->retrans_stamp ?: tcp_skb_timestamp(skb))); + if (user_timeout) { /* If user application specified a TCP_USER_TIMEOUT, * it does not want win 0 packets to 'reset the timer' @@ -482,9 +485,6 @@ static bool tcp_rtx_probe0_timed_out(con if (rcv_delta <= timeout) return false; - rtx_delta = (u32)msecs_to_jiffies(tcp_time_stamp(tp) - - (tp->retrans_stamp ?: tcp_skb_timestamp(skb))); - return rtx_delta > timeout; }
On Tue, Jul 16, 2024 at 6:03 AM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Tue, Jul 16, 2024 at 02:56:28PM +0200, Greg KH wrote: > > On Tue, Jul 16, 2024 at 02:53:12PM +0200, Greg KH wrote: > > > On Tue, Jul 16, 2024 at 08:40:40PM +0800, Jason Xing wrote: > > > > On Tue, Jul 16, 2024 at 7:10 PM Miguel Ojeda <ojeda@kernel.org> wrote: > > > > > > > > > > Hi Greg, Eric, all, > > > > > > > > > > I noticed this in stable-rc/queue and stable-rc/linux- for 6.1 and 6.6: > > > > > > > > > > net/ipv4/tcp_timer.c:472:7: error: variable 'rtx_delta' is uninitialized when used here [-Werror,-Wuninitialized] > > > > > if (rtx_delta > user_timeout) > > > > > ^~~~~~~~~ > > > > > net/ipv4/tcp_timer.c:464:15: note: initialize the variable 'rtx_delta' to silence this warning > > > > > u32 rtx_delta; > > > > > ^ > > > > > = 0 > > > > > > > > > > I hope that helps! > > > > > > > > Thanks for the report! > > > > > > > > I think it missed one small snippet of code from [1] compared to the > > > > latest kernel. We can init this part before using it, something like > > > > this: > > > > > > > > + rtx_delta = (u32)msecs_to_jiffies(tcp_time_stamp(tp) - > > > > + (tp->retrans_stamp ?: tcp_skb_timestamp(skb))); > > > > > > > > Note: fully untested. > > > > > > > > Since Eric is very busy, I decided to check and provide some useful > > > > information here. > > > > > > Thanks all, this was probably due to my manual backporting here, let me > > > go check what went wrong... > > > > Yeah, this is my fault, due to 614e8316aa4c ("tcp: add support for usec > > resolution in TCP TS values") not being in the tree, let me go rework > > things... > > Ok, backporting that commit is not going to happen, that's crazy... Absolutely right, this is not stable material. > > Anyway, the diff below is what I made on top of the existing one, which > should be doing the right thing. But ideally someone can test this, > somehow... I'll push out -rc releases later today so that people can > pound on it easier. > > thanks for the review! > > greg k-h > > > --- a/net/ipv4/tcp_timer.c > +++ b/net/ipv4/tcp_timer.c > @@ -464,6 +464,9 @@ static bool tcp_rtx_probe0_timed_out(con > u32 rtx_delta; > s32 rcv_delta; > > + rtx_delta = (u32)msecs_to_jiffies(tcp_time_stamp(tp) - > + (tp->retrans_stamp ?: tcp_skb_timestamp(skb))); > + > if (user_timeout) { > /* If user application specified a TCP_USER_TIMEOUT, > * it does not want win 0 packets to 'reset the timer' > @@ -482,9 +485,6 @@ static bool tcp_rtx_probe0_timed_out(con > if (rcv_delta <= timeout) > return false; > > - rtx_delta = (u32)msecs_to_jiffies(tcp_time_stamp(tp) - > - (tp->retrans_stamp ?: tcp_skb_timestamp(skb))); > - > return rtx_delta > timeout; > } > Hi Greg, this looks great to me, thanks for taking care of this.
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c index cbd4fde47c1f8d29533bf5ce28bddf4c9a00efe7..a386e9b84984ab0be41b0c38ea015e4ae3377edf 100644 --- a/net/ipv4/tcp_timer.c +++ b/net/ipv4/tcp_timer.c @@ -437,16 +437,28 @@ static void tcp_fastopen_synack_timer(struct sock *sk, struct request_sock *req) static bool tcp_rtx_probe0_timed_out(const struct sock *sk, const struct sk_buff *skb) { + const struct inet_connection_sock *icsk = inet_csk(sk); + u32 user_timeout = READ_ONCE(icsk->icsk_user_timeout); const struct tcp_sock *tp = tcp_sk(sk); - const int timeout = TCP_RTO_MAX * 2; + int timeout = TCP_RTO_MAX * 2; u32 rtx_delta; s32 rcv_delta; + if (user_timeout) { + /* If user application specified a TCP_USER_TIMEOUT, + * it does not want win 0 packets to 'reset the timer' + * while retransmits are not making progress. + */ + if (rtx_delta > user_timeout) + return true; + timeout = min_t(u32, timeout, msecs_to_jiffies(user_timeout)); + } + /* Note: timer interrupt might have been delayed by at least one jiffy, * and tp->rcv_tstamp might very well have been written recently. * rcv_delta can thus be negative. */ - rcv_delta = inet_csk(sk)->icsk_timeout - tp->rcv_tstamp; + rcv_delta = icsk->icsk_timeout - tp->rcv_tstamp; if (rcv_delta <= timeout) return false;
commit 97a9063518f198ec0adb2ecb89789de342bb8283 upstream. If a TCP socket is using TCP_USER_TIMEOUT, and the other peer retracted its window to zero, tcp_retransmit_timer() can retransmit a packet every two jiffies (2 ms for HZ=1000), for about 4 minutes after TCP_USER_TIMEOUT has 'expired'. The fix is to make sure tcp_rtx_probe0_timed_out() takes icsk->icsk_user_timeout into account. Before blamed commit, the socket would not timeout after icsk->icsk_user_timeout, but would use standard exponential backoff for the retransmits. Also worth noting that before commit e89688e3e978 ("net: tcp: fix unexcepted socket die when snd_wnd is 0"), the issue would last 2 minutes instead of 4. Fixes: b701a99e431d ("tcp: Add tcp_clamp_rto_to_user_timeout() helper to improve accuracy") Signed-off-by: Eric Dumazet <edumazet@google.com> Cc: Neal Cardwell <ncardwell@google.com> Reviewed-by: Jason Xing <kerneljasonxing@gmail.com> Reviewed-by: Jon Maxwell <jmaxwell37@gmail.com> Reviewed-by: Kuniyuki Iwashima <kuniyu@amazon.com> Link: https://patch.msgid.link/20240710001402.2758273-1-edumazet@google.com Signed-off-by: Jakub Kicinski <kuba@kernel.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> --- net/ipv4/tcp_timer.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)