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:
 <PAXPR07MB7984AD495ACF09289D78AA96A3452@PAXPR07MB7984.eurprd07.prod.outlook.com>
Date: Tue, 15 Oct 2024 20:25:16 +0000
From: "Chia-Yu Chang (Nokia)" <chia-yu.chang@...ia-bell-labs.com>
To: Ilpo Järvinen <ij@...nel.org>
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

-----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

Chia-Yu