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
| ||
|
Message-ID: <k2xd1c2719f1005081027v376a4ebfp300c6272f9ea91df@mail.gmail.com> Date: Sat, 8 May 2010 10:27:43 -0700 From: Jerry Chu <hkchu@...gle.com> To: Damian Lukowski <damian@....rwth-aachen.de> Cc: ilpo.jarvinen@...sinki.fi, netdev@...r.kernel.org Subject: Re: [PATCH][v4] tcp: fix ICMP-RTO war On Sat, May 8, 2010 at 1:30 AM, Damian Lukowski <damian@....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@...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
Powered by blists - more mailing lists