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

["SandeepI" <irlanki.s@samsung.com>]

> -----Original Message-----
> From: sashiko-bot@kernel.org <sashiko-bot@kernel.org>
> Sent: 10 September 2026 20:15
> To: Irlanki Sandeep <irlanki.s@samsung.com>
> 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 <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?
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.


> [ ... ]
> > 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?

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.