netdev - Re: [PATCH v5 net-next 04/15] 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: <4124e050-5614-424c-969c-9521ff02bee3@redhat.com>
Date: Tue, 29 Apr 2025 12:36:35 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: chia-yu.chang@...ia-bell-labs.com, horms@...nel.org, dsahern@...nel.org,
 kuniyu@...zon.com, bpf@...r.kernel.org, netdev@...r.kernel.org,
 dave.taht@...il.com, jhs@...atatu.com, kuba@...nel.org,
 stephen@...workplumber.org, xiyou.wangcong@...il.com, jiri@...nulli.us,
 davem@...emloft.net, edumazet@...gle.com, andrew+netdev@...n.ch,
 donald.hunter@...il.com, ast@...erby.net, liuhangbin@...il.com,
 shuah@...nel.org, linux-kselftest@...r.kernel.org, ij@...nel.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
Cc: Olivier Tilmans <olivier.tilmans@...ia.com>
Subject: Re: [PATCH v5 net-next 04/15] tcp: accecn: AccECN negotiation

On 4/22/25 5:35 PM, chia-yu.chang@...ia-bell-labs.com wrote:
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index e36018203bd0..af38fff24aa4 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -156,6 +156,10 @@ struct tcp_request_sock {
>  #if IS_ENABLED(CONFIG_MPTCP)
>  	bool				drop_req;
>  #endif
> +	u8				accecn_ok  : 1,
> +					syn_ect_snt: 2,
> +					syn_ect_rcv: 2;
> +	u8				accecn_fail_mode:4;

AFAICS this will create a 3 bytes hole. That could be bad if it will
also increase the number of cachelines used by struct tcp_request_sock.
Please include the pahole info and struct size in the commit message.

If there is no size problem I guess you are better off using a 'bool'
for 'accecn_ok'

>  	u32				txhash;
>  	u32				rcv_isn;
>  	u32				snt_isn;
> @@ -376,7 +380,10 @@ struct tcp_sock {
>  	u8	compressed_ack;
>  	u8	dup_ack_counter:2,
>  		tlp_retrans:1,	/* TLP is a retransmission */
> -		unused:5;
> +		syn_ect_snt:2,	/* AccECN ECT memory, only */
> +		syn_ect_rcv:2,	/* ... needed durign 3WHS + first seqno */
> +		wait_third_ack:1; /* Wait 3rd ACK in simultaneous open */

A good bunch of conditionals will be added to the fast path checking
this flag. Is simult open really a thing for AccECN? Can we simple
disable AccECN in such scenarios and simplify the code a bit? In my
limited experience only syzkaller reliably use it.

> +	u8	accecn_fail_mode:4;     /* AccECN failure handling */

This is outside the fastpath area, so possibly the struct size increase
is less critical, but AFAICS this will create a 6bits hole (as the next
u8 has only 6bit used). I think it's better to read the 'unused' field
to mark such hole.

>  	u8	thin_lto    : 1,/* Use linear timeouts for thin streams */
>  		fastopen_connect:1, /* FASTOPEN_CONNECT sockopt */
>  		fastopen_no_cookie:1, /* Allow send/recv SYN+data without a cookie */

[...]
> +/* See Table 2 of the AccECN draft */
> +static void tcp_ecn_rcv_synack(struct sock *sk, const struct tcphdr *th,
> +			       u8 ip_dsfield)
> +{
> +	struct tcp_sock *tp = tcp_sk(sk);
> +	u8 ace = tcp_accecn_ace(th);
> +
> +	switch (ace) {
> +	case 0x0:
> +	case 0x7:
>  		tcp_ecn_mode_set(tp, TCP_ECN_DISABLED);
> +		break;
> +	case 0x1:
> +	case 0x5:

Possibly some human readable defines could help instead of magic numbers
here.

[...]
> @@ -6171,16 +6252,27 @@ static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
>  	 * RFC 5961 4.2 : Send a challenge ack
>  	 */
>  	if (th->syn) {
> +		if (tcp_ecn_mode_accecn(tp))
> +			send_accecn_reflector = true;
>  		if (sk->sk_state == TCP_SYN_RECV && sk->sk_socket && th->ack &&
>  		    TCP_SKB_CB(skb)->seq + 1 == TCP_SKB_CB(skb)->end_seq &&
>  		    TCP_SKB_CB(skb)->seq + 1 == tp->rcv_nxt &&
> -		    TCP_SKB_CB(skb)->ack_seq == tp->snd_nxt)
> +		    TCP_SKB_CB(skb)->ack_seq == tp->snd_nxt) {
> +			if (!tcp_ecn_disabled(tp)) {
> +				u8 ect = tp->syn_ect_rcv;
> +
> +				tp->wait_third_ack = true;
> +				__tcp_send_ack(sk, tp->rcv_nxt,
> +					       !send_accecn_reflector ? 0 :
> +					       tcp_accecn_reflector_flags(ect));

The same expression is used above possibly you can create a new helper
for this statement.

...

This patch is quite huge. Any hope to break id down to a more palatable
size? i.e. moving the 3rd ack/self connect handling to a separate patch
(if that thing is really needed).

/P