netdev - Re: [PATCH v4 net-next 10/15] tcp: accecn: AccECN option send control

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: <20250418173407.GC2676982@horms.kernel.org>
Date: Fri, 18 Apr 2025 18:34:07 +0100
From: Simon Horman <horms@...nel.org>
To: chia-yu.chang@...ia-bell-labs.com
Cc: dsahern@...nel.org, kuniyu@...zon.com, bpf@...r.kernel.org,
	netdev@...r.kernel.org, dave.taht@...il.com, pabeni@...hat.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
Subject: Re: [PATCH v4 net-next 10/15] tcp: accecn: AccECN option send control

On Fri, Apr 18, 2025 at 01:00:24AM +0200, chia-yu.chang@...ia-bell-labs.com wrote:
> From: Ilpo Järvinen <ij@...nel.org>
> 
> Instead of sending the option in every ACK, limit sending to
> those ACKs where the option is necessary:
> - Handshake
> - "Change-triggered ACK" + the ACK following it. The
>   2nd ACK is necessary to unambiguously indicate which
>   of the ECN byte counters in increasing. The first
>   ACK has two counters increasing due to the ecnfield
>   edge.
> - ACKs with CE to allow CEP delta validations to take
>   advantage of the option.
> - Force option to be sent every at least once per 2^22
>   bytes. The check is done using the bit edges of the
>   byte counters (avoids need for extra variables).
> - AccECN option beacon to send a few times per RTT even if
>   nothing in the ECN state requires that. The default is 3
>   times per RTT, and its period can be set via
>   sysctl_tcp_ecn_option_beacon.
> 
> 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        |  3 +++
>  include/net/netns/ipv4.h   |  1 +
>  include/net/tcp.h          |  1 +
>  net/ipv4/sysctl_net_ipv4.c |  9 ++++++++
>  net/ipv4/tcp.c             |  5 ++++-
>  net/ipv4/tcp_input.c       | 36 +++++++++++++++++++++++++++++++-
>  net/ipv4/tcp_ipv4.c        |  1 +
>  net/ipv4/tcp_minisocks.c   |  2 ++
>  net/ipv4/tcp_output.c      | 42 ++++++++++++++++++++++++++++++--------
>  9 files changed, 90 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/tcp.h b/include/linux/tcp.h
> index 0e032d9631ac..9619524d8901 100644
> --- a/include/linux/tcp.h
> +++ b/include/linux/tcp.h
> @@ -309,7 +309,10 @@ struct tcp_sock {
>  	u8	received_ce_pending:4, /* Not yet transmit cnt of received_ce */
>  		unused2:4;
>  	u8	accecn_minlen:2,/* Minimum length of AccECN option sent */
> +		prev_ecnfield:2,/* ECN bits from the previous segment */
> +		accecn_opt_demand:2,/* Demand AccECN option for n next ACKs */
>  		est_ecnfield:2;/* ECN field for AccECN delivered estimates */
> +	u64	accecn_opt_tstamp;	/* Last AccECN option sent timestamp */
>  	u32	app_limited;	/* limited until "delivered" reaches this val */
>  	u32	rcv_wnd;	/* Current receiver window		*/
>  /*

...

> @@ -5113,7 +5116,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, 122 + 6);
> +	CACHELINE_ASSERT_GROUP_SIZE(struct tcp_sock, tcp_sock_write_txrx, 130 + 6);

Hi,

While this seems find on x86_64, x86_32 and arm64, it does not seem correct
on ARM (32-bit).

This is because the existing two byte hole after est_ecnfield grows
to 6 bytes. I assume this is because of alignment requirements.
But in any case, the result is that the overall group size increases
by 12 bytes rather than 8.

I believe that you can avoid the hole growing, and thus limit the overall
increase in size of the group to 12 bytes, by placing accecn_opt_tstamp
elsewhere, e.g. after app_limited rather than before it.

You can exercise this by cross compiling for ARM and examining
the structure layout using pahole.

Cross compilers available from [1] should be able to do that something
like this:

PATH=".../gcc-14.2.0-nolibc/arm-linux-gnueabi/bin:$PATH"
export ARCH=arm
export CROSS_COMPILE=arm-linux-gnueabi-

make allmodconfig
echo "CONFIG_DEBUG_INFO_DWARF_TOOLCHAIN_DEFAULT=y" >> .config
yes "" | make oldconfig

make net/ipv4/tcp.o

[1] https://mirrors.edge.kernel.org/pub/tools/crosstool/files/bin/x86_64/14.2.0/