netdev - Re: [RFC PATCH 24/28] tcp: try to fit AccECN option with SACK

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]

Date:   Thu, 19 Mar 2020 22:33:43 +0200 (EET)
From:   "Ilpo Järvinen" <ilpo.jarvinen@...helsinki.fi>
To:     Eric Dumazet <eric.dumazet@...il.com>
cc:     Netdev <netdev@...r.kernel.org>, Yuchung Cheng <ycheng@...gle.com>,
        Neal Cardwell <ncardwell@...gle.com>,
        Olivier Tilmans <olivier.tilmans@...ia-bell-labs.com>
Subject: Re: [RFC PATCH 24/28] tcp: try to fit AccECN option with SACK

On Wed, 18 Mar 2020, Eric Dumazet wrote:

> 
> 
> On 3/18/20 2:43 AM, Ilpo Järvinen wrote:
> > From: Ilpo Järvinen <ilpo.jarvinen@...helsinki.fi>
> > 
> > As SACK blocks tend to eat all option space when there are
> > many holes, it is useful to compromise on sending many SACK
> > blocks in every ACK and try to fit AccECN option there
> > by reduction the number of SACK blocks. But never go below
> > two SACK blocks because of AccECN option.
> > 
> > As AccECN option is often not put to every ACK, the space
> > hijack is usually only temporary.
> > 
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@...helsinki.fi>
> > ---
> >  net/ipv4/tcp_output.c | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> > index 4cc590a47f43..0aec2c57a9cc 100644
> > --- a/net/ipv4/tcp_output.c
> > +++ b/net/ipv4/tcp_output.c
> > @@ -756,6 +756,21 @@ static int tcp_options_fit_accecn(struct tcp_out_options *opts, int required,
> >  	if (opts->num_ecn_bytes < required)
> >  		return 0;
> 
> Have you tested this patch ?
> 
> (You forgot to remove the prior 2 lines)
> 
> >  
> > +	if (opts->num_ecn_bytes < required) {

Yes and no. There was no unit test for this particular condition but
I added a few now (with and w/o timestamps). I also managed to find and 
fix a byte-order related bug related to non-fullsized option while making 
those tests.

(I didn't actually forget to remove it. I managed to add the problem 
during a botched conflict merge when I reorganized some of the code.)

Thanks for taking a look.

-- 
 i.