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: <4eb02755-8061-6cf7-3fea-5b645e371caa@kernel.org>
Date: Tue, 15 Oct 2024 22:49:33 +0300 (EEST)
From: Ilpo Järvinen <ij@...nel.org>
To: Chia-Yu Chang <chia-yu.chang@...ia-bell-labs.com>
cc: netdev@...r.kernel.org, ncardwell@...gle.com, 
    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 <olivier.tilmans@...ia.com>
Subject: Re: [PATCH net-next 17/44] tcp: accecn: AccECN negotiation

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.