netdev - Re: [PATCH v5 net-next 03/15] tcp: AccECN core

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: <62a1b3ac-a36b-4812-a014-c04a078eed36@redhat.com>
Date: Tue, 29 Apr 2025 12:14:14 +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 03/15] tcp: AccECN core

On 4/22/25 5:35 PM, chia-yu.chang@...ia-bell-labs.com wrote:
> @@ -298,6 +298,9 @@ struct tcp_sock {
>  	u32	snd_up;		/* Urgent pointer		*/
>  	u32	delivered;	/* Total data packets delivered incl. rexmits */
>  	u32	delivered_ce;	/* Like the above but only ECE marked packets */
> +	u32	received_ce;	/* Like the above but for rcvd CE marked pkts */
> +	u8	received_ce_pending:4, /* Not yet transmit cnt of received_ce */
> +		unused2:4;

AFAICS this uses a 4 bytes hole present prior to this patch after
"rcv_wnd", leaving a 3 bytes hole after 'unused2'. Possibly should be
worth mentioning the hole presence.

@Eric: would it make sense use this hole for
'noneagle'/'rate_app_limited' and shrink the 'tcp_sock_write_txrx' group
a bit?

[...]
> @@ -5095,7 +5097,7 @@ static void __init tcp_struct_check(void)
>  	/* 32bit arches with 8byte alignment on u64 fields might need padding
>  	 * before tcp_clock_cache.
>  	 */
> -	CACHELINE_ASSERT_GROUP_SIZE(struct tcp_sock, tcp_sock_write_txrx, 92 + 4);
> +	CACHELINE_ASSERT_GROUP_SIZE(struct tcp_sock, tcp_sock_write_txrx, 97 + 7);

Really? I *think* the change here should not move the cacheline end
around, due to holes. Could you please include the relevant pahole
(trimmed) output prior to this patch and after in the commit message?

[...]
> @@ -384,17 +387,16 @@ static void tcp_data_ecn_check(struct sock *sk, const struct sk_buff *skb)
>  		if (tcp_ca_needs_ecn(sk))
>  			tcp_ca_event(sk, CA_EVENT_ECN_IS_CE);
>  
> -		if (!(tp->ecn_flags & TCP_ECN_DEMAND_CWR)) {
> +		if (!(tp->ecn_flags & TCP_ECN_DEMAND_CWR) &&
> +		    tcp_ecn_mode_rfc3168(tp)) {
>  			/* Better not delay acks, sender can have a very low cwnd */
>  			tcp_enter_quickack_mode(sk, 2);
>  			tp->ecn_flags |= TCP_ECN_DEMAND_CWR;
>  		}
> -		tp->ecn_flags |= TCP_ECN_SEEN;

At this point is not entirely clear to me why the removal of the above
line is needed/correct.

[...]
> @@ -4056,6 +4118,11 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
>  
>  	tcp_rack_update_reo_wnd(sk, &rs);
>  
> +	if (tcp_ecn_mode_accecn(tp))
> +		ecn_count = tcp_accecn_process(sk, skb,
> +					       tp->delivered - delivered,
> +					       &flag);

AFAICS the above could set FLAG_ECE in flags, menaning the previous
tcp_clean_rtx_queue() will run with such flag cleared and the later
function checking such flag will not. I wondering if this inconsistency
could cause problems?

/P