[PATCH][v4] tcp: fix ICMP-RTO war

[PATCH][v4] tcp: fix ICMP-RTO war

[Damian Lukowski]

Make sure, that TCP has a nonzero RTT estimation after three-way
handshake. Currently, a listening TCP has a value of 0 for srtt,
rttvar and rto right after the three-way handshake is completed
with TCP timestamps disabled.
This will lead to corrupt RTO recalculation and retransmission
flood when RTO is recalculated on backoff reversion as introduced
in "Revert RTO on ICMP destination unreachable"
(f1ecd5d9e7366609d640ff4040304ea197fbc618).
This behaviour can be provoked by connecting to a server which
"responds first" (like SMTP) and rejecting every packet after
the handshake with dest-unreachable, which will lead to softirq
load on the server (up to 30% per socket in some tests).

Thanks to Ilpo Jarvinen for providing debug patches and to
Denys Fedoryshchenko for reporting and testing.

Changes since v3: Removed bad characters in patchfile.

Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>
---
 net/ipv4/tcp_input.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 28e0296..da7173b 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -5783,12 +5783,10 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
 
 				/* tcp_ack considers this ACK as duplicate
 				 * and does not calculate rtt.
-				 * Fix it at least with timestamps.
+				 * Force it here.
 				 */
-				if (tp->rx_opt.saw_tstamp &&
-				    tp->rx_opt.rcv_tsecr && !tp->srtt)
-					tcp_ack_saw_tstamp(sk, 0);
-
+				tcp_ack_update_rtt(sk, 0, 0);
+
 				if (tp->rx_opt.tstamp_ok)
 					tp->advmss -= TCPOLEN_TSTAMP_ALIGNED;
 
-- 1.6.4.4 

Re: [PATCH][v4] tcp: fix ICMP-RTO war

[David Miller]

From: Damian Lukowski <damian@tvk.rwth-aachen.de>
Date: Wed, 10 Feb 2010 20:06:42 +0100

> Make sure, that TCP has a nonzero RTT estimation after three-way
> handshake. Currently, a listening TCP has a value of 0 for srtt,
> rttvar and rto right after the three-way handshake is completed
> with TCP timestamps disabled.
> This will lead to corrupt RTO recalculation and retransmission
> flood when RTO is recalculated on backoff reversion as introduced
> in "Revert RTO on ICMP destination unreachable"
> (f1ecd5d9e7366609d640ff4040304ea197fbc618).
> This behaviour can be provoked by connecting to a server which
> "responds first" (like SMTP) and rejecting every packet after
> the handshake with dest-unreachable, which will lead to softirq
> load on the server (up to 30% per socket in some tests).
> 
> Thanks to Ilpo Jarvinen for providing debug patches and to
> Denys Fedoryshchenko for reporting and testing.
> 
> Changes since v3: Removed bad characters in patchfile.
> 
> Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
> Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>

Thanks for doing this work, I'll study these issues
and review this patch today.

Re: [PATCH][v4] tcp: fix ICMP-RTO war

[David Miller]

From: Damian Lukowski <damian@tvk.rwth-aachen.de>
Date: Wed, 10 Feb 2010 20:06:42 +0100

> Make sure, that TCP has a nonzero RTT estimation after three-way
> handshake. Currently, a listening TCP has a value of 0 for srtt,
> rttvar and rto right after the three-way handshake is completed
> with TCP timestamps disabled.
> This will lead to corrupt RTO recalculation and retransmission
> flood when RTO is recalculated on backoff reversion as introduced
> in "Revert RTO on ICMP destination unreachable"
> (f1ecd5d9e7366609d640ff4040304ea197fbc618).
> This behaviour can be provoked by connecting to a server which
> "responds first" (like SMTP) and rejecting every packet after
> the handshake with dest-unreachable, which will lead to softirq
> load on the server (up to 30% per socket in some tests).
> 
> Thanks to Ilpo Jarvinen for providing debug patches and to
> Denys Fedoryshchenko for reporting and testing.
> 
> Changes since v3: Removed bad characters in patchfile.
> 
> Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
> Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>

Applied, thank you very much for fixing this bug.

Re: [PATCH][v4] tcp: fix ICMP-RTO war

[Ilpo Järvinen]

First of all I want to let you know that I've no objection to this fix 
itself (DaveM applied it already), it certainly does it's jobs in 
preventing invalid < RTO_MIN state. However, I wonder if we could do 
further improvements in this area...

On Wed, 10 Feb 2010, Damian Lukowski wrote:

> Make sure, that TCP has a nonzero RTT estimation after three-way
> handshake. Currently, a listening TCP has a value of 0 for srtt,
> rttvar and rto right after the three-way handshake is completed
> with TCP timestamps disabled.
> This will lead to corrupt RTO recalculation and retransmission
> flood when RTO is recalculated on backoff reversion as introduced
> in "Revert RTO on ICMP destination unreachable"
> (f1ecd5d9e7366609d640ff4040304ea197fbc618).
> This behaviour can be provoked by connecting to a server which
> "responds first" (like SMTP) and rejecting every packet after
> the handshake with dest-unreachable, which will lead to softirq
> load on the server (up to 30% per socket in some tests).
> 
> Thanks to Ilpo Jarvinen for providing debug patches and to
> Denys Fedoryshchenko for reporting and testing.
> 
> Changes since v3: Removed bad characters in patchfile.
> 
> Reported-by: Denys Fedoryshchenko <denys@visp.net.lb>
> Signed-off-by: Damian Lukowski <damian@tvk.rwth-aachen.de>
> ---
>  net/ipv4/tcp_input.c |    8 +++-----
>  1 files changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 28e0296..da7173b 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -5783,12 +5783,10 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
>  
>  				/* tcp_ack considers this ACK as duplicate
>  				 * and does not calculate rtt.
> -				 * Fix it at least with timestamps.
> +				 * Force it here.
>  				 */
> -				if (tp->rx_opt.saw_tstamp &&
> -				    tp->rx_opt.rcv_tsecr && !tp->srtt)
> -					tcp_ack_saw_tstamp(sk, 0);
> -
> +				tcp_ack_update_rtt(sk, 0, 0);
> +

...Here a zero seq_rtt is given to RTT estimator (it will be effective 
only in the case w/o timestamps, TS case recalculates it from the stored 
timestamps). Maybe we could use some field (timestamp related one comes to 
my mind) in request sock to get a real RTT estimate for non-timestamp case 
too. ...It seems possible to me, though tricky because the request_sock is 
no longer that easily available here so some parameter passing would be 
needed.


-- 
 i.

Re: [PATCH][v4] tcp: fix ICMP-RTO war

[David Miller]

From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
Date: Tue, 16 Feb 2010 14:45:25 +0200 (EET)

> On Wed, 10 Feb 2010, Damian Lukowski wrote:
> 
>> @@ -5783,12 +5783,10 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
>>  
>>  				/* tcp_ack considers this ACK as duplicate
>>  				 * and does not calculate rtt.
>> -				 * Fix it at least with timestamps.
>> +				 * Force it here.
>>  				 */
>> -				if (tp->rx_opt.saw_tstamp &&
>> -				    tp->rx_opt.rcv_tsecr && !tp->srtt)
>> -					tcp_ack_saw_tstamp(sk, 0);
>> -
>> +				tcp_ack_update_rtt(sk, 0, 0);
>> +
> 
> ...Here a zero seq_rtt is given to RTT estimator (it will be effective 
> only in the case w/o timestamps, TS case recalculates it from the stored 
> timestamps). Maybe we could use some field (timestamp related one comes to 
> my mind) in request sock to get a real RTT estimate for non-timestamp case 
> too. ...It seems possible to me, though tricky because the request_sock is 
> no longer that easily available here so some parameter passing would be 
> needed.

Agreed.

But even more simply I think we should make even the current
tcp_ack_update_rtt() call here conditional on at least
tp->srtt being zero.

Damian do you at least agree with that?

Re: [PATCH][v4] tcp: fix ICMP-RTO war

[Jerry Chu]

Hi,

I'm working on a patch that tries to measure and use the RTT for the passive
open side when the TS option is NOT enabled. My code conflicts with your
recently added "tcp_ack_update_rtt(sk, 0, 0);" Could you tell me why do you
force this call for the no-TS case when obviously "0" is not a measured RTT?
If you try to force icsk_rto to be initialized correctly, it is
already initialized to
TCP_TIMEOUT_INIT by tcp_create_openreq_child(). What am I missing?

Thanks,

Jerry

>From: David Miller <davem@davemloft.net>
>
> Date: Sun, Feb 21, 2010 at 7:10 PM
> Subject: Re: [PATCH][v4] tcp: fix ICMP-RTO war
> To: ilpo.jarvinen@helsinki.fi
> Cc: damian@tvk.rwth-aachen.de, netdev@vger.kernel.org
>
>
> From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
> Date: Tue, 16 Feb 2010 14:45:25 +0200 (EET)
>
> > On Wed, 10 Feb 2010, Damian Lukowski wrote:
> >
> >> @@ -5783,12 +5783,10 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
> >>
> >>                              /* tcp_ack considers this ACK as duplicate
> >>                               * and does not calculate rtt.
> >> -                             * Fix it at least with timestamps.
> >> +                             * Force it here.
> >>                               */
> >> -                            if (tp->rx_opt.saw_tstamp &&
> >> -                                tp->rx_opt.rcv_tsecr && !tp->srtt)
> >> -                                    tcp_ack_saw_tstamp(sk, 0);
> >> -
> >> +                            tcp_ack_update_rtt(sk, 0, 0);
> >> +
> >
> > ...Here a zero seq_rtt is given to RTT estimator (it will be effective
> > only in the case w/o timestamps, TS case recalculates it from the stored
> > timestamps). Maybe we could use some field (timestamp related one comes to
> > my mind) in request sock to get a real RTT estimate for non-timestamp case
> > too. ...It seems possible to me, though tricky because the request_sock is
> > no longer that easily available here so some parameter passing would be
> > needed.
>
> Agreed.
>
> But even more simply I think we should make even the current
> tcp_ack_update_rtt() call here conditional on at least
> tp->srtt being zero.
>
> Damian do you at least agree with that?
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][v4] tcp: fix ICMP-RTO war

[Damian Lukowski]

> I'm working on a patch that tries to measure and use the RTT for the passive
> open side when the TS option is NOT enabled. My code conflicts with your
> recently added "tcp_ack_update_rtt(sk, 0, 0);" Could you tell me why do you
> force this call for the no-TS case when obviously "0" is not a measured RTT?
> If you try to force icsk_rto to be initialized correctly, it is
> already initialized to
> TCP_TIMEOUT_INIT by tcp_create_openreq_child(). What am I missing?

Hi,
the backoff reversion code uses __tcp_set_rto() to recalculate icsk_rto,
which itself relies on tp->srtt and rttvar.
srtt is explicitly set to 0 in tcp_create_openreq_child(), so I didn't change it.
Initializing it with TCP_TIMEOUT_INIT should also fix that specific bug,
but I don't know if there are other impacts.

Regards
 Damian

> Thanks,
> 
> Jerry
> 
>> From: David Miller <davem@davemloft.net>
>>
>> Date: Sun, Feb 21, 2010 at 7:10 PM
>> Subject: Re: [PATCH][v4] tcp: fix ICMP-RTO war
>> To: ilpo.jarvinen@helsinki.fi
>> Cc: damian@tvk.rwth-aachen.de, netdev@vger.kernel.org
>>
>>
>> From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
>> Date: Tue, 16 Feb 2010 14:45:25 +0200 (EET)
>>
>>> On Wed, 10 Feb 2010, Damian Lukowski wrote:
>>>
>>>> @@ -5783,12 +5783,10 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
>>>>
>>>>                              /* tcp_ack considers this ACK as duplicate
>>>>                               * and does not calculate rtt.
>>>> -                             * Fix it at least with timestamps.
>>>> +                             * Force it here.
>>>>                               */
>>>> -                            if (tp->rx_opt.saw_tstamp &&
>>>> -                                tp->rx_opt.rcv_tsecr && !tp->srtt)
>>>> -                                    tcp_ack_saw_tstamp(sk, 0);
>>>> -
>>>> +                            tcp_ack_update_rtt(sk, 0, 0);
>>>> +
>>>
>>> ...Here a zero seq_rtt is given to RTT estimator (it will be effective
>>> only in the case w/o timestamps, TS case recalculates it from the stored
>>> timestamps). Maybe we could use some field (timestamp related one comes to
>>> my mind) in request sock to get a real RTT estimate for non-timestamp case
>>> too. ...It seems possible to me, though tricky because the request_sock is
>>> no longer that easily available here so some parameter passing would be
>>> needed.
>>
>> Agreed.
>>
>> But even more simply I think we should make even the current
>> tcp_ack_update_rtt() call here conditional on at least
>> tp->srtt being zero.
>>
>> Damian do you at least agree with that?
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH][v4] tcp: fix ICMP-RTO war

[Jerry Chu]

On Sat, May 8, 2010 at 1:30 AM, Damian Lukowski
<damian@tvk.rwth-aachen.de> wrote:
>
> > I'm working on a patch that tries to measure and use the RTT for the passive
> > open side when the TS option is NOT enabled. My code conflicts with your
> > recently added "tcp_ack_update_rtt(sk, 0, 0);" Could you tell me why do you
> > force this call for the no-TS case when obviously "0" is not a measured RTT?
> > If you try to force icsk_rto to be initialized correctly, it is
> > already initialized to
> > TCP_TIMEOUT_INIT by tcp_create_openreq_child(). What am I missing?
>
> Hi,
> the backoff reversion code uses __tcp_set_rto() to recalculate icsk_rto,
> which itself relies on tp->srtt and rttvar.

Guess you are talking about

inet_csk(sk)->icsk_rto = __tcp_set_rto(tp) <<
                                         icsk->icsk_backoff;

inside tcp_v4_err(), right? (I'm looking at 2.6.33 kernel.)

Yes it seems to be a bug when __tcp_set_rto() is called before
tcp_rtt_estimator()
gets a chance to initialize all the variables properly.

But I don't like your fix of adding tcp_ack_update_rtt(sk, 0, 0); to
tcp_rcv_state_process()
because that means you've got a measured RTT of 0 (really meaning < 1 tick) for
the no-TS case, which will cause tcp_rtt_estimator() to compute all
the variables as if
there has been a valid RTT measurement of 1.

A better fix IMHO is to make sure all the variables are properly
initialized when exiting
tcp_init_metrics(), e.g, if srtt remains 0, make sure
tp->mdev = tp->mdev_max = tp->rttvar = TCP_TIMEOUT_INIT;
(mdev already been initialized to TCP_TIMEOUT_INIT. I think you got
hit by rttvar == 0)

> srtt is explicitly set to 0 in tcp_create_openreq_child(), so I didn't change it.
> Initializing it with TCP_TIMEOUT_INIT should also fix that specific bug,
> but I don't know if there are other impacts.

So what do I care? Because I'm mucking with the code in this area and your fix
causes some conflict with my logic.

What do you think?

Best,

Jerry

>
> Regards
>  Damian
>
> > Thanks,
> >
> > Jerry
> >
> >> From: David Miller <davem@davemloft.net>
> >>
> >> Date: Sun, Feb 21, 2010 at 7:10 PM
> >> Subject: Re: [PATCH][v4] tcp: fix ICMP-RTO war
> >> To: ilpo.jarvinen@helsinki.fi
> >> Cc: damian@tvk.rwth-aachen.de, netdev@vger.kernel.org
> >>
> >>
> >> From: "Ilpo Järvinen" <ilpo.jarvinen@helsinki.fi>
> >> Date: Tue, 16 Feb 2010 14:45:25 +0200 (EET)
> >>
> >>> On Wed, 10 Feb 2010, Damian Lukowski wrote:
> >>>
> >>>> @@ -5783,12 +5783,10 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb,
> >>>>
> >>>>                              /* tcp_ack considers this ACK as duplicate
> >>>>                               * and does not calculate rtt.
> >>>> -                             * Fix it at least with timestamps.
> >>>> +                             * Force it here.
> >>>>                               */
> >>>> -                            if (tp->rx_opt.saw_tstamp &&
> >>>> -                                tp->rx_opt.rcv_tsecr && !tp->srtt)
> >>>> -                                    tcp_ack_saw_tstamp(sk, 0);
> >>>> -
> >>>> +                            tcp_ack_update_rtt(sk, 0, 0);
> >>>> +
> >>>
> >>> ...Here a zero seq_rtt is given to RTT estimator (it will be effective
> >>> only in the case w/o timestamps, TS case recalculates it from the stored
> >>> timestamps). Maybe we could use some field (timestamp related one comes to
> >>> my mind) in request sock to get a real RTT estimate for non-timestamp case
> >>> too. ...It seems possible to me, though tricky because the request_sock is
> >>> no longer that easily available here so some parameter passing would be
> >>> needed.
> >>
> >> Agreed.
> >>
> >> But even more simply I think we should make even the current
> >> tcp_ack_update_rtt() call here conditional on at least
> >> tp->srtt being zero.
> >>
> >> Damian do you at least agree with that?
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe netdev" in
> >> the body of a message to majordomo@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH][v4] tcp: fix ICMP-RTO war

[Ilpo Järvinen]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2678 bytes --]

On Sat, 8 May 2010, Jerry Chu wrote:

> On Sat, May 8, 2010 at 1:30 AM, Damian Lukowski
> <damian@tvk.rwth-aachen.de> wrote:
> >
> > > I'm working on a patch that tries to measure and use the RTT for the passive
> > > open side when the TS option is NOT enabled. My code conflicts with your
> > > recently added "tcp_ack_update_rtt(sk, 0, 0);" Could you tell me why do you
> > > force this call for the no-TS case when obviously "0" is not a measured RTT?
> > > If you try to force icsk_rto to be initialized correctly, it is
> > > already initialized to
> > > TCP_TIMEOUT_INIT by tcp_create_openreq_child(). What am I missing?
> >
> > Hi,
> > the backoff reversion code uses __tcp_set_rto() to recalculate icsk_rto,
> > which itself relies on tp->srtt and rttvar.
> 
> Guess you are talking about
> 
> inet_csk(sk)->icsk_rto = __tcp_set_rto(tp) <<
>                                          icsk->icsk_backoff;
> 
> inside tcp_v4_err(), right? (I'm looking at 2.6.33 kernel.)
> 
> Yes it seems to be a bug when __tcp_set_rto() is called before
> tcp_rtt_estimator()
> gets a chance to initialize all the variables properly.
>
> But I don't like your fix of adding tcp_ack_update_rtt(sk, 0, 0); to
> tcp_rcv_state_process()
> because that means you've got a measured RTT of 0 (really meaning < 1 tick) for
> the no-TS case, which will cause tcp_rtt_estimator() to compute all
> the variables as if
> there has been a valid RTT measurement of 1.
> 
> A better fix IMHO is to make sure all the variables are properly
> initialized when exiting
> tcp_init_metrics(), e.g, if srtt remains 0, make sure
> tp->mdev = tp->mdev_max = tp->rttvar = TCP_TIMEOUT_INIT;
> (mdev already been initialized to TCP_TIMEOUT_INIT. I think you got
> hit by rttvar == 0)
> 
> > srtt is explicitly set to 0 in tcp_create_openreq_child(), so I didn't change it.
> > Initializing it with TCP_TIMEOUT_INIT should also fix that specific bug,
> > but I don't know if there are other impacts.
> 
> So what do I care? Because I'm mucking with the code in this area and your fix
> causes some conflict with my logic.
> 
> What do you think?

I didn't/don't like the fix either, but it's better than ICMP-RTO war that 
it fixed.

I think that most sensible solution to this issue would not be to 
initialize with the TCP_TIMEOUT_INIT bogosity either but do the real 
measurement for rtt which should be possible already. I suggested this 
already when the fix was made but nobody has yet found the time and
energy to code the rtt measurement for the early rtt when not using TS. 
Since you're going to make that lack of feature to go away, please change 
this illogical call too then while you go :-).

-- 
 i.