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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <4BE5213C.1030300@tvk.rwth-aachen.de>
Date:	Sat, 08 May 2010 10:30:52 +0200
From:	Damian Lukowski <damian@....rwth-aachen.de>
To:	Jerry Chu <hkchu@...gle.com>
Cc:	ilpo.jarvinen@...sinki.fi, netdev@...r.kernel.org
Subject: Re: [PATCH][v4] tcp: fix ICMP-RTO war

> 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@...emloft.net>
>>
>> Date: Sun, Feb 21, 2010 at 7:10 PM
>> Subject: Re: [PATCH][v4] tcp: fix ICMP-RTO war
>> To: ilpo.jarvinen@...sinki.fi
>> Cc: damian@....rwth-aachen.de, netdev@...r.kernel.org
>>
>>
>> From: "Ilpo Järvinen" <ilpo.jarvinen@...sinki.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@...r.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html