netdev - Re: A buggy behavior for Linux TCP Reno and HTCP

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] [day] [month] [year] [list]

Date:   Mon, 24 Jul 2017 21:19:57 -0700
From:   Stephen Hemminger <stephen@...workplumber.org>
To:     Yuchung Cheng <ycheng@...gle.com>
Cc:     Neal Cardwell <ncardwell@...gle.com>, Lisong Xu <xu@....edu>,
        Wei Sun <unlcsewsun@...il.com>, netdev <netdev@...r.kernel.org>
Subject: Re: A buggy behavior for Linux TCP Reno and HTCP

On Mon, 24 Jul 2017 16:41:12 -0700
Yuchung Cheng <ycheng@...gle.com> wrote:

> On Mon, Jul 24, 2017 at 11:29 AM, Neal Cardwell <ncardwell@...gle.com> wrote:
> > On Mon, Jul 24, 2017 at 2:17 PM, Yuchung Cheng <ycheng@...gle.com> wrote:  
> >> On Sun, Jul 23, 2017 at 7:37 PM, Neal Cardwell <ncardwell@...gle.com> wrote:  
> >>> On Sun, Jul 23, 2017 at 10:36 PM, Neal Cardwell <ncardwell@...gle.com> wrote:  
> > ...  
> >>>> What if we call the field tp->prior_cwnd? Then at least we'd have some
> >>>> nice symmetry:
> >>>>
> >>>> - tp->snd_cwnd,  which is saved in tp->prior_cwnd  (and restored upon undo)
> >>>> - tp->snd_ssthresh,  which is saved in tp-> prior_ssthresh  (and
> >>>> restored upon undo)
> >>>>
> >>>> That sounds appealing to me. WDYT?  
> >>>
> >>> And, I should add, if we go with the tp->prior_cwnd approach, then we
> >>> can have a single "default"/CUBIC-style undo function, instead of 15
> >>> separate but identical implementations...  
> >> you mean all CC modules share one ca_ops->undo_cwnd function? sounds a
> >> nice consolidation work.  
> >
> > Yes, exactly.
> >
> > Right now we have 9 modules that have identical tcp_foo_cwnd_undo functions:
> >
> > tcp_bic.c:188:  return max(tp->snd_cwnd, ca->loss_cwnd);
> > tcp_cubic.c:378:        return max(tcp_sk(sk)->snd_cwnd, ca->loss_cwnd);
> > tcp_dctcp.c:318:        return max(tcp_sk(sk)->snd_cwnd, ca->loss_cwnd);
> > tcp_highspeed.c:165:    return max(tcp_sk(sk)->snd_cwnd, ca->loss_cwnd);
> > tcp_illinois.c:309:     return max(tcp_sk(sk)->snd_cwnd, ca->loss_cwnd);
> > tcp_nv.c:190:   return max(tcp_sk(sk)->snd_cwnd, ca->loss_cwnd);
> > tcp_scalable.c:50:      return max(tcp_sk(sk)->snd_cwnd, ca->loss_cwnd);
> > tcp_veno.c:210: return max(tcp_sk(sk)->snd_cwnd, veno->loss_cwnd);
> > tcp_yeah.c:232: return max(tcp_sk(sk)->snd_cwnd, yeah->loss_cwnd);
> >
> > And if we fix this bug in tcp_reno_undo_cwnd() by referring to
> > ca->loss_cwnd then we will add another 6 like this.
> >
> > So my proposal would be
> >
> > - tp->snd_cwnd,  which is saved in tp->prior_cwnd  (and restored upon undo)
> > - tp->snd_ssthresh,  which is saved in tp-> prior_ssthresh  (and
> >    restored upon undo)
> >
> > Actually, now that I re-read the code, we already do have a
> > prior_cwnd, which is used for the PRR code, and already set upon
> > entering CA_Recovery. So if we set prior_cwnd for CA_Loss, perhaps we
> > can do something like:
> >
> > diff --git a/net/ipv4/tcp_cong.c b/net/ipv4/tcp_cong.c
> > index fde983f6376b..c2b174469645 100644
> > --- a/net/ipv4/tcp_cong.c
> > +++ b/net/ipv4/tcp_cong.c
> > @@ -456,7 +456,7 @@ u32 tcp_reno_undo_cwnd(struct sock *sk)
> >  {
> >         const struct tcp_sock *tp = tcp_sk(sk);
> >
> > -       return max(tp->snd_cwnd, tp->snd_ssthresh << 1);
> > +       return max(tp->snd_cwnd, tp->prior_cwnd);
> >  }
> >  EXPORT_SYMBOL_GPL(tcp_reno_undo_cwnd);
> >
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 2920e0cb09f8..ae790a84302d 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -1951,6 +1951,7 @@ void tcp_enter_loss(struct sock *sk)
> >             !after(tp->high_seq, tp->snd_una) ||
> >             (icsk->icsk_ca_state == TCP_CA_Loss && !icsk->icsk_retransmits)) {
> >                 tp->prior_ssthresh = tcp_current_ssthresh(sk);
> > +               tp->prior_cwnd = tp->snd_cwnd;
> >                 tp->snd_ssthresh = icsk->icsk_ca_ops->ssthresh(sk);
> >                 tcp_ca_event(sk, CA_EVENT_LOSS);
> >                 tcp_init_undo(tp);
> >
> > And then change all the CC modules but BBR to use the
> > tcp_reno_undo_cwnd() instead of their own custom undo code.
> >
> > WDYT?  
> Looks reasonable. But we might want to eventually refactor TCP undo
> code: the stats changes (prior_ssthresh, prior_cwnd, undo_marker,
> undo_retrans) are scattered in different helpers, making the code hard
> to audit.

I like having common code as much as possible,
having per CC undo means more variations and sources of errors.