[PATCH net-next] tcp: avoid possible arithmetic overflows

[PATCH net-next] tcp: avoid possible arithmetic overflows

[Eric Dumazet]

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);
 

Re: [PATCH net-next] tcp: avoid possible arithmetic overflows

[Joe Perches]

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);

Re: [PATCH net-next] tcp: avoid possible arithmetic overflows

[Yuchung Cheng]

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);
>
>

Re: [PATCH net-next] tcp: avoid possible arithmetic overflows

[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.

Re: [PATCH net-next] tcp: avoid possible arithmetic overflows

[Joe Perches]

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

[PATCH v2 net-next] tcp: avoid possible arithmetic overflows

[Eric Dumazet]

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);
 

Re: [PATCH v2 net-next] tcp: avoid possible arithmetic overflows

[Yuchung Cheng]

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);
>
>
>

Re: [PATCH v2 net-next] tcp: avoid possible arithmetic overflows

[Joe Perches]

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.

RE: [PATCH net-next] tcp: avoid possible arithmetic overflows

[David Laight]

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

Re: [PATCH v2 net-next] tcp: avoid possible arithmetic overflows

[Eric Dumazet]

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);

[PATCH v3 net-next] tcp: avoid possible arithmetic overflows

[Eric Dumazet]

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);
 

Re: [PATCH v3 net-next] tcp: avoid possible arithmetic overflows

[David Miller]

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,

Re: [PATCH v3 net-next] tcp: avoid possible arithmetic overflows

[Eric Dumazet]

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 ;)