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

-----Original Message-----
From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
Sent: 10 September 2026 20:15
To: Irlanki Sandeep <redacted>
Cc: bpf@vger.kernel.org
Subject: Re: [PATCH net-next] tcp: add TCP_ECN and TCP_ECN_OPTION socket
options

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 [off-list ref]
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.

quoted

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

[ ... ]

quoted

@@ -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) */

quoted

 };

+/* 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?

quoted

+
+/* 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?

Yes, tcp_sk(sk)->ecn_mode and tcp_sk(sk)->ecn_option are updated
concurrently via WRITE_ONCE() in do_tcp_setsockopt(). Without READ_ONCE(),
a compiler double-fetch could leak the TCP_ECN_MODE_UNSPEC (255) sentinel
value into operational evaluations.

Both accessors (tcp_ecn_mode_eff and tcp_accecn_option_eff) have been updated
in V3 with READ_ONCE() to eliminate data races and prevent double-fetching.

[ ... ]

quoted

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,

quoted

 	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?

Agreed. Under SYN flood conditions, cookie_v4_check() and cookie_v6_check()
must honor the listener socket's effective ECN configuration rather than
falling back exclusively to the global sysctl.

Moved cookie_ecn_ok() from include/net/tcp.h to include/net/tcp_ecn.h
to avoid circular header dependencies (since tcp_ecn_mode_eff() is defined
in tcp_ecn.h and tcp.h does not include tcp_ecn.h), and updated it to take
'const struct sock *sk' so it evaluates tcp_ecn_mode_eff(sk).

Both issues have been addressed in the [PATCH net-next v3] tcp: add TCP_ECN and TCP_ECN_OPTION socket options

Thanks,
Sandeep.