* [PATCH net-next] tcp: avoid possible arithmetic overflows @ 2014-09-20 17:19 Eric Dumazet 2014-09-20 18:01 ` Joe Perches 0 siblings, 1 reply; 13+ messages in thread From: Eric Dumazet @ 2014-09-20 17:19 UTC (permalink / raw) To: David Miller; +Cc: netdev, Neal Cardwell, Yuchung Cheng From: Eric Dumazet <edumazet@google.com> icsk_rto is an 32bit field, and icsk_backoff can reach 15 by default, or more if some sysctl (eg tcp_retries2) are changed. Better use 64bit to perform icsk_rto << icsk_backoff operations From: Eric Dumazet <edumazet@google.com> --- net/ipv4/tcp_input.c | 7 +++++-- net/ipv4/tcp_output.c | 13 ++++++------- net/ipv4/tcp_timer.c | 5 +++-- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 02fb66d4a018..1ea3847c62fc 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -3208,9 +3208,12 @@ static void tcp_ack_probe(struct sock *sk) * This function is not for random using! */ } else { + unsigned long when; + + when = min((u64)icsk->icsk_rto << icsk->icsk_backoff, + (u64)TCP_RTO_MAX); inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, - min(icsk->icsk_rto << icsk->icsk_backoff, TCP_RTO_MAX), - TCP_RTO_MAX); + when, TCP_RTO_MAX); } } diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 7f1280dcad57..2231b400f3ce 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -3279,6 +3279,7 @@ void tcp_send_probe0(struct sock *sk) { struct inet_connection_sock *icsk = inet_csk(sk); struct tcp_sock *tp = tcp_sk(sk); + unsigned long when; int err; err = tcp_write_wakeup(sk); @@ -3294,9 +3295,8 @@ void tcp_send_probe0(struct sock *sk) if (icsk->icsk_backoff < sysctl_tcp_retries2) icsk->icsk_backoff++; icsk->icsk_probes_out++; - inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, - min(icsk->icsk_rto << icsk->icsk_backoff, TCP_RTO_MAX), - TCP_RTO_MAX); + when = min((u64)icsk->icsk_rto << icsk->icsk_backoff, + (u64)TCP_RTO_MAX); } else { /* If packet was not sent due to local congestion, * do not backoff and do not remember icsk_probes_out. @@ -3306,11 +3306,10 @@ void tcp_send_probe0(struct sock *sk) */ if (!icsk->icsk_probes_out) icsk->icsk_probes_out = 1; - inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, - min(icsk->icsk_rto << icsk->icsk_backoff, - TCP_RESOURCE_PROBE_INTERVAL), - TCP_RTO_MAX); + when = min((u64)icsk->icsk_rto << icsk->icsk_backoff, + (u64)TCP_RESOURCE_PROBE_INTERVAL); } + inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, when, TCP_RTO_MAX); } int tcp_rtx_synack(struct sock *sk, struct request_sock *req) diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c index a339e7ba05a4..05e1d0723233 100644 --- a/net/ipv4/tcp_timer.c +++ b/net/ipv4/tcp_timer.c @@ -180,7 +180,7 @@ static int tcp_write_timeout(struct sock *sk) retry_until = sysctl_tcp_retries2; if (sock_flag(sk, SOCK_DEAD)) { - const int alive = (icsk->icsk_rto < TCP_RTO_MAX); + const int alive = icsk->icsk_rto < TCP_RTO_MAX; retry_until = tcp_orphan_retries(sk, alive); do_reset = alive || @@ -294,7 +294,8 @@ static void tcp_probe_timer(struct sock *sk) max_probes = sysctl_tcp_retries2; if (sock_flag(sk, SOCK_DEAD)) { - const int alive = ((icsk->icsk_rto << icsk->icsk_backoff) < TCP_RTO_MAX); + u64 exp_rto = (u64)icsk->icsk_rto << icsk->icsk_backoff; + const int alive = exp_rto < TCP_RTO_MAX; max_probes = tcp_orphan_retries(sk, alive); ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] tcp: avoid possible arithmetic overflows 2014-09-20 17:19 [PATCH net-next] tcp: avoid possible arithmetic overflows Eric Dumazet @ 2014-09-20 18:01 ` Joe Perches 2014-09-20 19:46 ` Yuchung Cheng 0 siblings, 1 reply; 13+ messages in thread From: Joe Perches @ 2014-09-20 18:01 UTC (permalink / raw) To: Eric Dumazet; +Cc: David Miller, netdev, Neal Cardwell, Yuchung Cheng On Sat, 2014-09-20 at 10:19 -0700, Eric Dumazet wrote: > From: Eric Dumazet <edumazet@google.com> > > icsk_rto is an 32bit field, and icsk_backoff can reach 15 by default, > or more if some sysctl (eg tcp_retries2) are changed. > > Better use 64bit to perform icsk_rto << icsk_backoff operations Maybe better to use a helper function for this? something like: static inline u64 icsk_rto_backoff(const struct inet_connection_sock *icsk) { u64 when = (u64)icsk->icsk_rto; return when << icsk->icsk_backoff; } > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c [] > @@ -3208,9 +3208,12 @@ static void tcp_ack_probe(struct sock *sk) > * This function is not for random using! > */ > } else { > + unsigned long when; > + > + when = min((u64)icsk->icsk_rto << icsk->icsk_backoff, > + (u64)TCP_RTO_MAX); Maybe: u32 when = (u32)min_t(u64, icsk_rto_backoff(icsk), TCP_RTO_MAX); ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] tcp: avoid possible arithmetic overflows 2014-09-20 18:01 ` Joe Perches @ 2014-09-20 19:46 ` Yuchung Cheng 2014-09-20 19:55 ` Eric Dumazet 0 siblings, 1 reply; 13+ messages in thread From: Yuchung Cheng @ 2014-09-20 19:46 UTC (permalink / raw) To: Joe Perches; +Cc: Eric Dumazet, David Miller, netdev, Neal Cardwell On Sat, Sep 20, 2014 at 11:01 AM, Joe Perches <joe@perches.com> wrote: > On Sat, 2014-09-20 at 10:19 -0700, Eric Dumazet wrote: >> From: Eric Dumazet <edumazet@google.com> >> >> icsk_rto is an 32bit field, and icsk_backoff can reach 15 by default, >> or more if some sysctl (eg tcp_retries2) are changed. >> >> Better use 64bit to perform icsk_rto << icsk_backoff operations > > Maybe better to use a helper function for this? > > something like: > > static inline u64 icsk_rto_backoff(const struct inet_connection_sock *icsk) > { > u64 when = (u64)icsk->icsk_rto; > > return when << icsk->icsk_backoff; > } Thanks for the fix Eric. I second Joe's idea to use a helper function. > >> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > [] >> @@ -3208,9 +3208,12 @@ static void tcp_ack_probe(struct sock *sk) >> * This function is not for random using! >> */ >> } else { >> + unsigned long when; >> + >> + when = min((u64)icsk->icsk_rto << icsk->icsk_backoff, >> + (u64)TCP_RTO_MAX); > > Maybe: > u32 when = (u32)min_t(u64, icsk_rto_backoff(icsk), TCP_RTO_MAX); > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] tcp: avoid possible arithmetic overflows 2014-09-20 19:46 ` Yuchung Cheng @ 2014-09-20 19:55 ` Eric Dumazet 2014-09-20 20:19 ` Joe Perches 2014-09-22 11:13 ` [PATCH " David Laight 0 siblings, 2 replies; 13+ messages in thread From: Eric Dumazet @ 2014-09-20 19:55 UTC (permalink / raw) To: Yuchung Cheng; +Cc: Joe Perches, David Miller, netdev, Neal Cardwell On Sat, 2014-09-20 at 12:46 -0700, Yuchung Cheng wrote: > On Sat, Sep 20, 2014 at 11:01 AM, Joe Perches <joe@perches.com> wrote: > > On Sat, 2014-09-20 at 10:19 -0700, Eric Dumazet wrote: > >> From: Eric Dumazet <edumazet@google.com> > >> > >> icsk_rto is an 32bit field, and icsk_backoff can reach 15 by default, > >> or more if some sysctl (eg tcp_retries2) are changed. > >> > >> Better use 64bit to perform icsk_rto << icsk_backoff operations > > > > Maybe better to use a helper function for this? > > > > something like: > > > > static inline u64 icsk_rto_backoff(const struct inet_connection_sock *icsk) > > { > > u64 when = (u64)icsk->icsk_rto; > > > > return when << icsk->icsk_backoff; > > } > Thanks for the fix Eric. I second Joe's idea to use a helper function. > Yep. Given the timeout functions in the kernel use 'unsigned long', I prefer to keep the u64 magic private to this helper. I will probably use static inline unsigned long icsk_rto_backoff(const struct inet_connection_sock *icsk) { u64 when = (u64)icsk->icsk_rto << icsk->icsk_backoff; return min_t(u64, when, ~0UL); } On 64bit arches, the min_t() should be a nop. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH net-next] tcp: avoid possible arithmetic overflows 2014-09-20 19:55 ` Eric Dumazet @ 2014-09-20 20:19 ` Joe Perches 2014-09-21 0:29 ` [PATCH v2 " Eric Dumazet 2014-09-22 11:13 ` [PATCH " David Laight 1 sibling, 1 reply; 13+ messages in thread From: Joe Perches @ 2014-09-20 20:19 UTC (permalink / raw) To: Eric Dumazet; +Cc: Yuchung Cheng, David Miller, netdev, Neal Cardwell On Sat, 2014-09-20 at 12:55 -0700, Eric Dumazet wrote: > On Sat, 2014-09-20 at 12:46 -0700, Yuchung Cheng wrote: > > On Sat, Sep 20, 2014 at 11:01 AM, Joe Perches <joe@perches.com> wrote: > > > On Sat, 2014-09-20 at 10:19 -0700, Eric Dumazet wrote: > > >> From: Eric Dumazet <edumazet@google.com> > > >> > > >> icsk_rto is an 32bit field, and icsk_backoff can reach 15 by default, > > >> or more if some sysctl (eg tcp_retries2) are changed. > > >> > > >> Better use 64bit to perform icsk_rto << icsk_backoff operations > > > > > > Maybe better to use a helper function for this? > > > > > > something like: > > > > > > static inline u64 icsk_rto_backoff(const struct inet_connection_sock *icsk) > > > { > > > u64 when = (u64)icsk->icsk_rto; > > > > > > return when << icsk->icsk_backoff; > > > } > > Thanks for the fix Eric. I second Joe's idea to use a helper function. > > > > Yep. > > Given the timeout functions in the kernel use 'unsigned long', I prefer > to keep the u64 magic private to this helper. > > I will probably use > > static inline unsigned long icsk_rto_backoff(const struct inet_connection_sock *icsk) > { > u64 when = (u64)icsk->icsk_rto << icsk->icsk_backoff; > > return min_t(u64, when, ~0UL); OK. I think an explicit cast to unsigned long after the min_t to avoid the implicit downcast would be better return (unsigned long)min_t(etc...) so that no warning is produced if someone does make W=3 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 net-next] tcp: avoid possible arithmetic overflows 2014-09-20 20:19 ` Joe Perches @ 2014-09-21 0:29 ` Eric Dumazet 2014-09-21 17:46 ` Yuchung Cheng 2014-09-22 3:56 ` [PATCH v2 " Joe Perches 0 siblings, 2 replies; 13+ messages in thread From: Eric Dumazet @ 2014-09-21 0:29 UTC (permalink / raw) To: Joe Perches; +Cc: Yuchung Cheng, David Miller, netdev, Neal Cardwell From: Eric Dumazet <edumazet@google.com> icsk_rto is a 32bit field, and icsk_backoff can reach 15 by default, or more if some sysctl (eg tcp_retries2) are changed. Better use 64bit to perform icsk_rto << icsk_backoff operations As Joe Perches suggested, add a helper for this. From: Eric Dumazet <edumazet@google.com> --- include/net/inet_connection_sock.h | 9 +++++++++ net/ipv4/tcp_input.c | 5 +++-- net/ipv4/tcp_output.c | 13 ++++++------- net/ipv4/tcp_timer.c | 4 ++-- 4 files changed, 20 insertions(+), 11 deletions(-) diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h index 5fbe6568c3cf..7551b402d6fe 100644 --- a/include/net/inet_connection_sock.h +++ b/include/net/inet_connection_sock.h @@ -242,6 +242,15 @@ static inline void inet_csk_reset_xmit_timer(struct sock *sk, const int what, #endif } +static inline unsigned long +inet_csk_rto_backoff(const struct inet_connection_sock *icsk, + unsigned long max_when) +{ + u64 when = (u64)icsk->icsk_rto << icsk->icsk_backoff; + + return (unsigned long)min_t(u64, when, max_when); +} + struct sock *inet_csk_accept(struct sock *sk, int flags, int *err); struct request_sock *inet_csk_search_req(const struct sock *sk, diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 02fb66d4a018..13f3da4762e3 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -3208,9 +3208,10 @@ static void tcp_ack_probe(struct sock *sk) * This function is not for random using! */ } else { + unsigned long when = inet_csk_rto_backoff(icsk, TCP_RTO_MAX); + inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, - min(icsk->icsk_rto << icsk->icsk_backoff, TCP_RTO_MAX), - TCP_RTO_MAX); + when, TCP_RTO_MAX); } } diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 7f1280dcad57..8c61a7c0c889 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -3279,6 +3279,7 @@ void tcp_send_probe0(struct sock *sk) { struct inet_connection_sock *icsk = inet_csk(sk); struct tcp_sock *tp = tcp_sk(sk); + unsigned long probe_max; int err; err = tcp_write_wakeup(sk); @@ -3294,9 +3295,7 @@ void tcp_send_probe0(struct sock *sk) if (icsk->icsk_backoff < sysctl_tcp_retries2) icsk->icsk_backoff++; icsk->icsk_probes_out++; - inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, - min(icsk->icsk_rto << icsk->icsk_backoff, TCP_RTO_MAX), - TCP_RTO_MAX); + probe_max = TCP_RTO_MAX; } else { /* If packet was not sent due to local congestion, * do not backoff and do not remember icsk_probes_out. @@ -3306,11 +3305,11 @@ void tcp_send_probe0(struct sock *sk) */ if (!icsk->icsk_probes_out) icsk->icsk_probes_out = 1; - inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, - min(icsk->icsk_rto << icsk->icsk_backoff, - TCP_RESOURCE_PROBE_INTERVAL), - TCP_RTO_MAX); + probe_max = TCP_RESOURCE_PROBE_INTERVAL; } + inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, + inet_csk_rto_backoff(icsk, probe_max), + TCP_RTO_MAX); } int tcp_rtx_synack(struct sock *sk, struct request_sock *req) diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c index a339e7ba05a4..b24360f6e293 100644 --- a/net/ipv4/tcp_timer.c +++ b/net/ipv4/tcp_timer.c @@ -180,7 +180,7 @@ static int tcp_write_timeout(struct sock *sk) retry_until = sysctl_tcp_retries2; if (sock_flag(sk, SOCK_DEAD)) { - const int alive = (icsk->icsk_rto < TCP_RTO_MAX); + const int alive = icsk->icsk_rto < TCP_RTO_MAX; retry_until = tcp_orphan_retries(sk, alive); do_reset = alive || @@ -294,7 +294,7 @@ static void tcp_probe_timer(struct sock *sk) max_probes = sysctl_tcp_retries2; if (sock_flag(sk, SOCK_DEAD)) { - const int alive = ((icsk->icsk_rto << icsk->icsk_backoff) < TCP_RTO_MAX); + const int alive = inet_csk_rto_backoff(icsk, TCP_RTO_MAX) < TCP_RTO_MAX; max_probes = tcp_orphan_retries(sk, alive); ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net-next] tcp: avoid possible arithmetic overflows 2014-09-21 0:29 ` [PATCH v2 " Eric Dumazet @ 2014-09-21 17:46 ` Yuchung Cheng 2014-09-22 12:42 ` Eric Dumazet 2014-09-22 3:56 ` [PATCH v2 " Joe Perches 1 sibling, 1 reply; 13+ messages in thread From: Yuchung Cheng @ 2014-09-21 17:46 UTC (permalink / raw) To: Eric Dumazet; +Cc: Joe Perches, David Miller, netdev, Neal Cardwell On Sat, Sep 20, 2014 at 5:29 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > From: Eric Dumazet <edumazet@google.com> > > icsk_rto is a 32bit field, and icsk_backoff can reach 15 by default, > or more if some sysctl (eg tcp_retries2) are changed. > > Better use 64bit to perform icsk_rto << icsk_backoff operations > > As Joe Perches suggested, add a helper for this. > > From: Eric Dumazet <edumazet@google.com> Acked-by: Yuchung Cheng <ycheng@google.com> Maybe also use the new helper for the backoff in tcp_v4_err()? > --- > include/net/inet_connection_sock.h | 9 +++++++++ > net/ipv4/tcp_input.c | 5 +++-- > net/ipv4/tcp_output.c | 13 ++++++------- > net/ipv4/tcp_timer.c | 4 ++-- > 4 files changed, 20 insertions(+), 11 deletions(-) > > diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h > index 5fbe6568c3cf..7551b402d6fe 100644 > --- a/include/net/inet_connection_sock.h > +++ b/include/net/inet_connection_sock.h > @@ -242,6 +242,15 @@ static inline void inet_csk_reset_xmit_timer(struct sock *sk, const int what, > #endif > } > > +static inline unsigned long > +inet_csk_rto_backoff(const struct inet_connection_sock *icsk, > + unsigned long max_when) > +{ > + u64 when = (u64)icsk->icsk_rto << icsk->icsk_backoff; > + > + return (unsigned long)min_t(u64, when, max_when); > +} > + > struct sock *inet_csk_accept(struct sock *sk, int flags, int *err); > > struct request_sock *inet_csk_search_req(const struct sock *sk, > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c > index 02fb66d4a018..13f3da4762e3 100644 > --- a/net/ipv4/tcp_input.c > +++ b/net/ipv4/tcp_input.c > @@ -3208,9 +3208,10 @@ static void tcp_ack_probe(struct sock *sk) > * This function is not for random using! > */ > } else { > + unsigned long when = inet_csk_rto_backoff(icsk, TCP_RTO_MAX); > + > inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, > - min(icsk->icsk_rto << icsk->icsk_backoff, TCP_RTO_MAX), > - TCP_RTO_MAX); > + when, TCP_RTO_MAX); > } > } > > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c > index 7f1280dcad57..8c61a7c0c889 100644 > --- a/net/ipv4/tcp_output.c > +++ b/net/ipv4/tcp_output.c > @@ -3279,6 +3279,7 @@ void tcp_send_probe0(struct sock *sk) > { > struct inet_connection_sock *icsk = inet_csk(sk); > struct tcp_sock *tp = tcp_sk(sk); > + unsigned long probe_max; > int err; > > err = tcp_write_wakeup(sk); > @@ -3294,9 +3295,7 @@ void tcp_send_probe0(struct sock *sk) > if (icsk->icsk_backoff < sysctl_tcp_retries2) > icsk->icsk_backoff++; > icsk->icsk_probes_out++; > - inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, > - min(icsk->icsk_rto << icsk->icsk_backoff, TCP_RTO_MAX), > - TCP_RTO_MAX); > + probe_max = TCP_RTO_MAX; > } else { > /* If packet was not sent due to local congestion, > * do not backoff and do not remember icsk_probes_out. > @@ -3306,11 +3305,11 @@ void tcp_send_probe0(struct sock *sk) > */ > if (!icsk->icsk_probes_out) > icsk->icsk_probes_out = 1; > - inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, > - min(icsk->icsk_rto << icsk->icsk_backoff, > - TCP_RESOURCE_PROBE_INTERVAL), > - TCP_RTO_MAX); > + probe_max = TCP_RESOURCE_PROBE_INTERVAL; > } > + inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, > + inet_csk_rto_backoff(icsk, probe_max), > + TCP_RTO_MAX); > } > > int tcp_rtx_synack(struct sock *sk, struct request_sock *req) > diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c > index a339e7ba05a4..b24360f6e293 100644 > --- a/net/ipv4/tcp_timer.c > +++ b/net/ipv4/tcp_timer.c > @@ -180,7 +180,7 @@ static int tcp_write_timeout(struct sock *sk) > > retry_until = sysctl_tcp_retries2; > if (sock_flag(sk, SOCK_DEAD)) { > - const int alive = (icsk->icsk_rto < TCP_RTO_MAX); > + const int alive = icsk->icsk_rto < TCP_RTO_MAX; > > retry_until = tcp_orphan_retries(sk, alive); > do_reset = alive || > @@ -294,7 +294,7 @@ static void tcp_probe_timer(struct sock *sk) > max_probes = sysctl_tcp_retries2; > > if (sock_flag(sk, SOCK_DEAD)) { > - const int alive = ((icsk->icsk_rto << icsk->icsk_backoff) < TCP_RTO_MAX); > + const int alive = inet_csk_rto_backoff(icsk, TCP_RTO_MAX) < TCP_RTO_MAX; > > max_probes = tcp_orphan_retries(sk, alive); > > > ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net-next] tcp: avoid possible arithmetic overflows 2014-09-21 17:46 ` Yuchung Cheng @ 2014-09-22 12:42 ` Eric Dumazet 2014-09-22 20:19 ` [PATCH v3 " Eric Dumazet 0 siblings, 1 reply; 13+ messages in thread From: Eric Dumazet @ 2014-09-22 12:42 UTC (permalink / raw) To: Yuchung Cheng; +Cc: Joe Perches, David Miller, netdev, Neal Cardwell On Sun, 2014-09-21 at 10:46 -0700, Yuchung Cheng wrote: > On Sat, Sep 20, 2014 at 5:29 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > From: Eric Dumazet <edumazet@google.com> > > > > icsk_rto is a 32bit field, and icsk_backoff can reach 15 by default, > > or more if some sysctl (eg tcp_retries2) are changed. > > > > Better use 64bit to perform icsk_rto << icsk_backoff operations > > > > As Joe Perches suggested, add a helper for this. > > > > From: Eric Dumazet <edumazet@google.com> > Acked-by: Yuchung Cheng <ycheng@google.com> > > Maybe also use the new helper for the backoff in tcp_v4_err()? You mean the following bit ? If yes, I'll incorporate it in v3 Thanks diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 006b045716d8..3b2e49cb2b61 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -430,9 +430,9 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info) break; icsk->icsk_backoff--; - inet_csk(sk)->icsk_rto = (tp->srtt_us ? __tcp_set_rto(tp) : - TCP_TIMEOUT_INIT) << icsk->icsk_backoff; - tcp_bound_rto(sk); + icsk->icsk_rto = tp->srtt_us ? __tcp_set_rto(tp) : + TCP_TIMEOUT_INIT; + icsk->icsk_rto = inet_csk_rto_backoff(icsk, TCP_RTO_MAX); skb = tcp_write_queue_head(sk); BUG_ON(!skb); ^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH v3 net-next] tcp: avoid possible arithmetic overflows 2014-09-22 12:42 ` Eric Dumazet @ 2014-09-22 20:19 ` Eric Dumazet 2014-09-22 20:27 ` David Miller 0 siblings, 1 reply; 13+ messages in thread From: Eric Dumazet @ 2014-09-22 20:19 UTC (permalink / raw) To: Yuchung Cheng; +Cc: Joe Perches, David Miller, netdev, Neal Cardwell From: Eric Dumazet <edumazet@google.com> icsk_rto is a 32bit field, and icsk_backoff can reach 15 by default, or more if some sysctl (eg tcp_retries2) are changed. Better use 64bit to perform icsk_rto << icsk_backoff operations As Joe Perches suggested, add a helper for this. Yuchung spotted the tcp_v4_err() case. From: Eric Dumazet <edumazet@google.com> --- include/net/inet_connection_sock.h | 9 +++++++++ net/ipv4/tcp_input.c | 5 +++-- net/ipv4/tcp_ipv4.c | 6 +++--- net/ipv4/tcp_output.c | 13 ++++++------- net/ipv4/tcp_timer.c | 4 ++-- 5 files changed, 23 insertions(+), 14 deletions(-) diff --git a/include/net/inet_connection_sock.h b/include/net/inet_connection_sock.h index 5fbe6568c3cf..848e85cb5c61 100644 --- a/include/net/inet_connection_sock.h +++ b/include/net/inet_connection_sock.h @@ -242,6 +242,15 @@ static inline void inet_csk_reset_xmit_timer(struct sock *sk, const int what, #endif } +static inline unsigned long +inet_csk_rto_backoff(const struct inet_connection_sock *icsk, + unsigned long max_when) +{ + u64 when = (u64)icsk->icsk_rto << icsk->icsk_backoff; + + return (unsigned long)min_t(u64, when, max_when); +} + struct sock *inet_csk_accept(struct sock *sk, int flags, int *err); struct request_sock *inet_csk_search_req(const struct sock *sk, diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c index 02fb66d4a018..13f3da4762e3 100644 --- a/net/ipv4/tcp_input.c +++ b/net/ipv4/tcp_input.c @@ -3208,9 +3208,10 @@ static void tcp_ack_probe(struct sock *sk) * This function is not for random using! */ } else { + unsigned long when = inet_csk_rto_backoff(icsk, TCP_RTO_MAX); + inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, - min(icsk->icsk_rto << icsk->icsk_backoff, TCP_RTO_MAX), - TCP_RTO_MAX); + when, TCP_RTO_MAX); } } diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c index 006b045716d8..3b2e49cb2b61 100644 --- a/net/ipv4/tcp_ipv4.c +++ b/net/ipv4/tcp_ipv4.c @@ -430,9 +430,9 @@ void tcp_v4_err(struct sk_buff *icmp_skb, u32 info) break; icsk->icsk_backoff--; - inet_csk(sk)->icsk_rto = (tp->srtt_us ? __tcp_set_rto(tp) : - TCP_TIMEOUT_INIT) << icsk->icsk_backoff; - tcp_bound_rto(sk); + icsk->icsk_rto = tp->srtt_us ? __tcp_set_rto(tp) : + TCP_TIMEOUT_INIT; + icsk->icsk_rto = inet_csk_rto_backoff(icsk, TCP_RTO_MAX); skb = tcp_write_queue_head(sk); BUG_ON(!skb); diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c index 7f1280dcad57..8c61a7c0c889 100644 --- a/net/ipv4/tcp_output.c +++ b/net/ipv4/tcp_output.c @@ -3279,6 +3279,7 @@ void tcp_send_probe0(struct sock *sk) { struct inet_connection_sock *icsk = inet_csk(sk); struct tcp_sock *tp = tcp_sk(sk); + unsigned long probe_max; int err; err = tcp_write_wakeup(sk); @@ -3294,9 +3295,7 @@ void tcp_send_probe0(struct sock *sk) if (icsk->icsk_backoff < sysctl_tcp_retries2) icsk->icsk_backoff++; icsk->icsk_probes_out++; - inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, - min(icsk->icsk_rto << icsk->icsk_backoff, TCP_RTO_MAX), - TCP_RTO_MAX); + probe_max = TCP_RTO_MAX; } else { /* If packet was not sent due to local congestion, * do not backoff and do not remember icsk_probes_out. @@ -3306,11 +3305,11 @@ void tcp_send_probe0(struct sock *sk) */ if (!icsk->icsk_probes_out) icsk->icsk_probes_out = 1; - inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, - min(icsk->icsk_rto << icsk->icsk_backoff, - TCP_RESOURCE_PROBE_INTERVAL), - TCP_RTO_MAX); + probe_max = TCP_RESOURCE_PROBE_INTERVAL; } + inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, + inet_csk_rto_backoff(icsk, probe_max), + TCP_RTO_MAX); } int tcp_rtx_synack(struct sock *sk, struct request_sock *req) diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c index a339e7ba05a4..b24360f6e293 100644 --- a/net/ipv4/tcp_timer.c +++ b/net/ipv4/tcp_timer.c @@ -180,7 +180,7 @@ static int tcp_write_timeout(struct sock *sk) retry_until = sysctl_tcp_retries2; if (sock_flag(sk, SOCK_DEAD)) { - const int alive = (icsk->icsk_rto < TCP_RTO_MAX); + const int alive = icsk->icsk_rto < TCP_RTO_MAX; retry_until = tcp_orphan_retries(sk, alive); do_reset = alive || @@ -294,7 +294,7 @@ static void tcp_probe_timer(struct sock *sk) max_probes = sysctl_tcp_retries2; if (sock_flag(sk, SOCK_DEAD)) { - const int alive = ((icsk->icsk_rto << icsk->icsk_backoff) < TCP_RTO_MAX); + const int alive = inet_csk_rto_backoff(icsk, TCP_RTO_MAX) < TCP_RTO_MAX; max_probes = tcp_orphan_retries(sk, alive); ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH v3 net-next] tcp: avoid possible arithmetic overflows 2014-09-22 20:19 ` [PATCH v3 " Eric Dumazet @ 2014-09-22 20:27 ` David Miller 2014-09-22 20:28 ` Eric Dumazet 0 siblings, 1 reply; 13+ messages in thread From: David Miller @ 2014-09-22 20:27 UTC (permalink / raw) To: eric.dumazet; +Cc: ycheng, joe, netdev, ncardwell From: Eric Dumazet <eric.dumazet@gmail.com> Date: Mon, 22 Sep 2014 13:19:44 -0700 > From: Eric Dumazet <edumazet@google.com> > > icsk_rto is a 32bit field, and icsk_backoff can reach 15 by default, > or more if some sysctl (eg tcp_retries2) are changed. > > Better use 64bit to perform icsk_rto << icsk_backoff operations > > As Joe Perches suggested, add a helper for this. > > Yuchung spotted the tcp_v4_err() case. > > From: Eric Dumazet <edumazet@google.com> I assume you meant "Signed-off-by: " on that last line, not "From: " :-) Applied, ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v3 net-next] tcp: avoid possible arithmetic overflows 2014-09-22 20:27 ` David Miller @ 2014-09-22 20:28 ` Eric Dumazet 0 siblings, 0 replies; 13+ messages in thread From: Eric Dumazet @ 2014-09-22 20:28 UTC (permalink / raw) To: David Miller; +Cc: ycheng, joe, netdev, ncardwell On Mon, 2014-09-22 at 16:27 -0400, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Mon, 22 Sep 2014 13:19:44 -0700 > > > From: Eric Dumazet <edumazet@google.com> > > > > icsk_rto is a 32bit field, and icsk_backoff can reach 15 by default, > > or more if some sysctl (eg tcp_retries2) are changed. > > > > Better use 64bit to perform icsk_rto << icsk_backoff operations > > > > As Joe Perches suggested, add a helper for this. > > > > Yuchung spotted the tcp_v4_err() case. > > > > From: Eric Dumazet <edumazet@google.com> > > I assume you meant "Signed-off-by: " on that last line, not "From: " :-) > > Applied, Arg, thanks David ;) ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 net-next] tcp: avoid possible arithmetic overflows 2014-09-21 0:29 ` [PATCH v2 " Eric Dumazet 2014-09-21 17:46 ` Yuchung Cheng @ 2014-09-22 3:56 ` Joe Perches 1 sibling, 0 replies; 13+ messages in thread From: Joe Perches @ 2014-09-22 3:56 UTC (permalink / raw) To: Eric Dumazet; +Cc: Yuchung Cheng, David Miller, netdev, Neal Cardwell On Sat, 2014-09-20 at 17:29 -0700, Eric Dumazet wrote: > icsk_rto is a 32bit field, and icsk_backoff can reach 15 by default, > or more if some sysctl (eg tcp_retries2) are changed. > > Better use 64bit to perform icsk_rto << icsk_backoff operations Thanks Eric. > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c [] > @@ -3208,9 +3208,10 @@ static void tcp_ack_probe(struct sock *sk) > * This function is not for random using! > */ > } else { > + unsigned long when = inet_csk_rto_backoff(icsk, TCP_RTO_MAX); > + > inet_csk_reset_xmit_timer(sk, ICSK_TIME_PROBE0, > - min(icsk->icsk_rto << icsk->icsk_backoff, TCP_RTO_MAX), > - TCP_RTO_MAX); > + when, TCP_RTO_MAX); Pity about the possible extra compare to TCP_RTO_MAX here. I hope gcc smart enough to optimize it away. ^ permalink raw reply [flat|nested] 13+ messages in thread
* RE: [PATCH net-next] tcp: avoid possible arithmetic overflows 2014-09-20 19:55 ` Eric Dumazet 2014-09-20 20:19 ` Joe Perches @ 2014-09-22 11:13 ` David Laight 1 sibling, 0 replies; 13+ messages in thread From: David Laight @ 2014-09-22 11:13 UTC (permalink / raw) To: 'Eric Dumazet', Yuchung Cheng Cc: Joe Perches, David Miller, netdev, Neal Cardwell From: Eric Dumazet > On Sat, 2014-09-20 at 12:46 -0700, Yuchung Cheng wrote: > > On Sat, Sep 20, 2014 at 11:01 AM, Joe Perches <joe@perches.com> wrote: > > > On Sat, 2014-09-20 at 10:19 -0700, Eric Dumazet wrote: > > >> From: Eric Dumazet <edumazet@google.com> > > >> > > >> icsk_rto is an 32bit field, and icsk_backoff can reach 15 by default, > > >> or more if some sysctl (eg tcp_retries2) are changed. > > >> > > >> Better use 64bit to perform icsk_rto << icsk_backoff operations > > > > > > Maybe better to use a helper function for this? > > > > > > something like: > > > > > > static inline u64 icsk_rto_backoff(const struct inet_connection_sock *icsk) > > > { > > > u64 when = (u64)icsk->icsk_rto; > > > > > > return when << icsk->icsk_backoff; > > > } > > Thanks for the fix Eric. I second Joe's idea to use a helper function. > > > > Yep. > > Given the timeout functions in the kernel use 'unsigned long', I prefer > to keep the u64 magic private to this helper. > > I will probably use > > static inline unsigned long icsk_rto_backoff(const struct inet_connection_sock *icsk) > { > u64 when = (u64)icsk->icsk_rto << icsk->icsk_backoff; > > return min_t(u64, when, ~0UL); > } > > On 64bit arches, the min_t() should be a nop. Isn't it likely to generate a 'condition is always true/false' warning? (that may depend on whether min_t contains < or <=) David ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2014-09-22 20:28 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-09-20 17:19 [PATCH net-next] tcp: avoid possible arithmetic overflows Eric Dumazet 2014-09-20 18:01 ` Joe Perches 2014-09-20 19:46 ` Yuchung Cheng 2014-09-20 19:55 ` Eric Dumazet 2014-09-20 20:19 ` Joe Perches 2014-09-21 0:29 ` [PATCH v2 " Eric Dumazet 2014-09-21 17:46 ` Yuchung Cheng 2014-09-22 12:42 ` Eric Dumazet 2014-09-22 20:19 ` [PATCH v3 " Eric Dumazet 2014-09-22 20:27 ` David Miller 2014-09-22 20:28 ` Eric Dumazet 2014-09-22 3:56 ` [PATCH v2 " Joe Perches 2014-09-22 11:13 ` [PATCH " David Laight
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).