Re: [PATCH net-next] tcp: add TCP_ECN and TCP_ECN_OPTION socket options

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Per-socket TCP_ECN configuration is ignored during SYN floods because syncookies validation checks the global sysctl instead of the listener socket's effective ECN mode.
- [Medium] Missing READ_ONCE() when reading `ecn_mode` and `ecn_option` introduces a data race and double-fetch vulnerability, leaking the UNSPEC placeholder (255) into TCP operational logic.
--

commit d87f9153f9de86df8e7de5798f8dffdac8f0e2fd
Author: Irlanki Sandeep <irlanki.s@samsung.com>
Subject: tcp: add TCP_ECN and TCP_ECN_OPTION socket options

This patch introduces granular, per-connection dynamic adaptation for ECN and
AccECN configurations. It adds two new socket options, TCP_ECN and
TCP_ECN_OPTION, which override the system-wide sysctl settings to prevent
connection failures or packet loss on paths with anomalous middleboxes.

> diff --git a/include/net/tcp_ecn.h b/include/net/tcp_ecn.h
> index 865d5c5a7718d..931e0877176c7 100644
> --- a/include/net/tcp_ecn.h
> +++ b/include/net/tcp_ecn.h
[ ... ]
> @@ -30,8 +31,29 @@ enum tcp_accecn_option {
>  	TCP_ACCECN_OPTION_MINIMUM = 1,
>  	TCP_ACCECN_OPTION_FULL = 2,
>  	TCP_ACCECN_OPTION_PERSIST = 3,
> +	TCP_ACCECN_OPTION_UNSPEC = 255,	/* Use sysctl default (per-socket) */
>  };
>  
> +/* Resolve the effective ECN mode: per-socket override or sysctl fallback */
> +static inline u8 tcp_ecn_mode_eff(const struct sock *sk)
> +{
> +	u8 mode = tcp_sk(sk)->ecn_mode;
> +
> +	if (mode == TCP_ECN_MODE_UNSPEC)
> +		return READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_ecn);
> +	return mode;
> +}

[Severity: Medium]
Is it possible for the compiler to double-fetch tcp_sk(sk)->ecn_mode here?

Since the ecn_mode field is updated concurrently via WRITE_ONCE() (for
example, during a setsockopt call while handling an incoming SYN packet in
softirq context), a lack of READ_ONCE() on the reader side could result in
the UNSPEC placeholder (255) leaking into the return evaluation. 

If that placeholder leaks, could it cause AccECN to be incorrectly enabled 
later in tcp_ecn_create_request(), where 255 >= 3 would evaluate to true?

> +
> +/* Resolve the effective AccECN option: per-socket override or sysctl fallback */
> +static inline u8 tcp_accecn_option_eff(const struct sock *sk)
> +{
> +	u8 opt = tcp_sk(sk)->ecn_option;
> +
> +	if (opt == TCP_ACCECN_OPTION_UNSPEC)
> +		return READ_ONCE(sock_net(sk)->ipv4.sysctl_tcp_ecn_option);
> +	return opt;
> +}

[Severity: Medium]
Does tcp_sk(sk)->ecn_option also require a READ_ONCE() here to prevent a 
similar data race and double-fetch?

[ ... ]
> diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> index 0f60a1dbf9274..b7dea787f8237 100644
> --- a/net/ipv4/tcp_input.c
> +++ b/net/ipv4/tcp_input.c
> @@ -7463,7 +7463,7 @@ static void tcp_ecn_create_request(struct request_sock *req,
>  	u32 ecn_ok_dst;
>  
>  	if (tcp_accecn_syn_requested(th) &&
> -	    (READ_ONCE(net->ipv4.sysctl_tcp_ecn) >= 3 ||
> +	    (tcp_ecn_mode_eff(listen_sk) >= 3 ||
>  	     tcp_ca_needs_accecn(listen_sk))) {
>  		inet_rsk(req)->ecn_ok = 1;
>  		tcp_rsk(req)->accecn_ok = 1;

[Severity: High]
Does the SYN cookies path also need to be updated to respect this new 
per-socket configuration?

During a SYN flood, this standard tcp_ecn_create_request() path is bypassed
in favor of syncookies. Currently, cookie_v4_check() and cookie_v6_check() 
determine ECN support by calling cookie_ecn_ok(), which only checks the
global sysctl:

static inline bool cookie_ecn_ok(const struct net *net, const struct dst_entry *dst)
{
        return READ_ONCE(net->ipv4.sysctl_tcp_ecn) ||
                dst_feature(dst, RTAX_FEATURE_ECN);
}

If the system has the global sysctl_tcp_ecn disabled but a service has
enabled it locally via TCP_ECN on its listener socket, will established
connections silently lose their ECN capability during a SYN flood?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909144557.800676-1-irlanki.s@samsung.com?part=1