netdev - RE: [PATCH net-next 17/44] tcp: accecn: AccECN negotiation

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: <3394a181-11c2-742c-b38f-ef80f35ea418@kernel.org>
Date: Tue, 15 Oct 2024 23:31:37 +0300 (EEST)
From: Ilpo Järvinen <ij@...nel.org>
To: "Chia-Yu Chang (Nokia)" <chia-yu.chang@...ia-bell-labs.com>
cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, 
    "ncardwell@...gle.com" <ncardwell@...gle.com>, 
    "Koen De Schepper (Nokia)" <koen.de_schepper@...ia-bell-labs.com>, 
    "g.white@...leLabs.com" <g.white@...leLabs.com>, 
    "ingemar.s.johansson@...csson.com" <ingemar.s.johansson@...csson.com>, 
    "mirja.kuehlewind@...csson.com" <mirja.kuehlewind@...csson.com>, 
    "cheshire@...le.com" <cheshire@...le.com>, 
    "rs.ietf@....at" <rs.ietf@....at>, 
    "Jason_Livingood@...cast.com" <Jason_Livingood@...cast.com>, 
    "vidhi_goel@...le.com" <vidhi_goel@...le.com>, 
    "Olivier Tilmans (Nokia)" <olivier.tilmans@...ia.com>
Subject: RE: [PATCH net-next 17/44] tcp: accecn: AccECN negotiation

On Tue, 15 Oct 2024, Chia-Yu Chang (Nokia) wrote:

> -----Original Message-----
> From: Ilpo Järvinen <ij@...nel.org> 
> Sent: Tuesday, October 15, 2024 9:50 PM
> To: Chia-Yu Chang (Nokia) <chia-yu.chang@...ia-bell-labs.com>
> Cc: netdev@...r.kernel.org; ncardwell@...gle.com; Koen De Schepper (Nokia) <koen.de_schepper@...ia-bell-labs.com>; g.white@...leLabs.com; ingemar.s.johansson@...csson.com; mirja.kuehlewind@...csson.com; cheshire@...le.com; rs.ietf@....at; Jason_Livingood@...cast.com; vidhi_goel@...le.com; Olivier Tilmans (Nokia) <olivier.tilmans@...ia.com>
> Subject: Re: [PATCH net-next 17/44] tcp: accecn: AccECN negotiation
> 
> 
> CAUTION: This is an external email. Please be very careful when clicking links or opening attachments. See the URL nok.it/ext for additional information.
> 
> 
> 
> On Tue, 15 Oct 2024, chia-yu.chang@...ia-bell-labs.com wrote:
> 
> > From: Ilpo Järvinen <ij@...nel.org>
> >
> > Accurate ECN negotiation parts based on the specification:
> >   https://tools.ietf.org/id/draft-ietf-tcpm-accurate-ecn-28.txt
> >
> > Accurate ECN is negotiated using ECE, CWR and AE flags in the TCP 
> > header. TCP falls back into using RFC3168 ECN if one of the ends 
> > supports only RFC3168-style ECN.
> >
> > The AccECN negotiation includes reflecting IP ECN field value seen in 
> > SYN and SYNACK back using the same bits as negotiation to allow 
> > responding to SYN CE marks and to detect ECN field mangling. CE marks 
> > should not occur currently because SYN=1 segments are sent with 
> > Non-ECT in IP ECN field (but proposal exists to remove this 
> > restriction).
> >
> > Reflecting SYN IP ECN field in SYNACK is relatively simple.
> > Reflecting SYNACK IP ECN field in the final/third ACK of the handshake 
> > is more challenging. Linux TCP code is not well prepared for using the 
> > final/third ACK a signalling channel which makes things somewhat 
> > complicated here.
> >
> > Co-developed-by: Olivier Tilmans <olivier.tilmans@...ia.com>
> > Signed-off-by: Olivier Tilmans <olivier.tilmans@...ia.com>
> > Signed-off-by: Ilpo Järvinen <ij@...nel.org>
> > Co-developed-by: Chia-Yu Chang <chia-yu.chang@...ia-bell-labs.com>
> > Signed-off-by: Chia-Yu Chang <chia-yu.chang@...ia-bell-labs.com>
> > ---
> >  include/linux/tcp.h        |   9 ++-
> >  include/net/tcp.h          |  80 +++++++++++++++++++-
> >  net/ipv4/syncookies.c      |   3 +
> >  net/ipv4/sysctl_net_ipv4.c |   2 +-
> >  net/ipv4/tcp.c             |   2 +
> >  net/ipv4/tcp_input.c       | 149 +++++++++++++++++++++++++++++++++----
> >  net/ipv4/tcp_ipv4.c        |   3 +-
> >  net/ipv4/tcp_minisocks.c   |  51 +++++++++++--
> >  net/ipv4/tcp_output.c      |  77 +++++++++++++++----
> >  net/ipv6/syncookies.c      |   1 +
> >  net/ipv6/tcp_ipv6.c        |   1 +
> >  11 files changed, 336 insertions(+), 42 deletions(-)
> >
> 
> > @@ -6358,6 +6446,13 @@ void tcp_rcv_established(struct sock *sk, struct sk_buff *skb)
> >               return;
> >
> >  step5:
> > +     if (unlikely(tp->wait_third_ack)) {
> > +             if (!tcp_ecn_disabled(tp))
> > +                     tp->wait_third_ack = 0;
> 
> I don't think !tcp_ecn_disabled(tp) condition is necessary and is harmful (I think I tried to explain this earlier but it seems there was a misunderstanding).
> 
> A third ACK is third ACK regardless of ECN mode and this entire code block should be skipped on subsequent ACKs after the third ACK. By adding that ECN mode condition, ->wait_third_ack cannot be set to zero if ECN mode get disabled which is harmful because then this code can never be skipped.
> 
> --
>  i.
> 
> If you read the only place I set this flag as 1 is with the same 
> condition if (!tcp_ecn_disabled(tp)), the original idea is to make it 
> symmetric when setting back to 0.
> Of course it might create problem if in future we change the condition 
> when set this flag as TRUE, then we need to change also here to set this 
> flag back to FALSE. But if this confusing, I can remove this if 
> condition in the next patches

My point is that something can change ECN mode in between so the symmetry 
argument doesn't really work here. You want to make sure wait_third_ack 
won't remain set if ECN got disable before we reach this line.

-- 
 i.