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

["Ilpo Järvinen" <ij@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 4244 bytes --]

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

> -----Original Message-----
> From: Ilpo Järvinen <ij@kernel.org> 
> Sent: Tuesday, October 15, 2024 9:50 PM
> To: Chia-Yu Chang (Nokia) <chia-yu.chang@nokia-bell-labs.com>
> Cc: netdev@vger.kernel.org; ncardwell@google.com; Koen De Schepper (Nokia) <koen.de_schepper@nokia-bell-labs.com>; g.white@CableLabs.com; ingemar.s.johansson@ericsson.com; mirja.kuehlewind@ericsson.com; cheshire@apple.com; rs.ietf@gmx.at; Jason_Livingood@comcast.com; vidhi_goel@apple.com; Olivier Tilmans (Nokia) <olivier.tilmans@nokia.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@nokia-bell-labs.com wrote:
> 
> > From: Ilpo Järvinen <ij@kernel.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@nokia.com>
> > Signed-off-by: Olivier Tilmans <olivier.tilmans@nokia.com>
> > Signed-off-by: Ilpo Järvinen <ij@kernel.org>
> > Co-developed-by: Chia-Yu Chang <chia-yu.chang@nokia-bell-labs.com>
> > Signed-off-by: Chia-Yu Chang <chia-yu.chang@nokia-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.