Re: [PATCH net-next v5] tcp: add TCP_ECN and TCP_ECN_OPTION kfuncs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] bpf: out-of-bounds write in bpf_sock_ops_set_accecn_option() for non-TCP sockets
- [Medium] tcp: child sockets inadvertently inherit listener's ECN overrides
- [Low] bpf: multi-line comment style violation in selftests
--

commit c9aee50642be2579799b62e26cc528d60bbb2e33
Author: Irlanki Sandeep <irlanki.s@samsung.com>
Subject: tcp: add TCP_ECN and TCP_ECN_OPTION kfuncs

This commit introduces two BPF kfuncs to allow per-socket overrides of ECN
and AccECN configurations. It also adds helper functions to resolve the
effective values, updates the TCP stack to use them, and includes selftests.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 70dc621672f2e..b9dc1079aefc5 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
[ ... ]
> +__bpf_kfunc int bpf_sock_ops_set_accecn_option(struct bpf_sock_ops_kern *skops,
> +					       u32 opt)
> +{
> +	if (!skops->is_fullsock)
> +		return -EOPNOTSUPP;
> +
> +	if (opt != TCP_ACCECN_OPTION_UNSPEC && opt > TCP_ACCECN_OPTION_PERSIST)
> +		return -EINVAL;
> +
> +	WRITE_ONCE(tcp_sk(skops->sk)->ecn_option, opt);

[Severity: High]
Could this lead to an out-of-bounds write for non-TCP sockets?

When a UDP socket requests hardware timestamping, the transmission skb gets
SKBTX_HW_TSTAMP set, which implicitly sets SKBTX_BPF. When the timestamp
is delivered, __skb_tstamp_tx() triggers bpf_skops_tx_timestamping(),
invoking the CGROUP_SOCK_OPS BPF program for the UDP socket with
is_fullsock set to 1.

If the BPF program calls this kfunc, it bypasses the !is_fullsock check.
Since there is no verification that the socket protocol is IPPROTO_TCP,
it casts the smaller udp_sock to a tcp_sock. Writing to ecn_option would
then corrupt memory past the end of the udp_sock structure.

[ ... ]
> diff --git a/tools/testing/selftests/bpf/prog_tests/sockops_ecn_kfunc.c b/tools/testing/selftests/bpf/prog_tests/sockops_ecn_kfunc.c
> new file mode 100644
> index 0000000000000..506b48e08621f
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/sockops_ecn_kfunc.c
[ ... ]
> +	/* BPF_SOCK_OPS_TCP_LISTEN_CB -> BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB
> +	 * Listener's values are consulted for SYN negotiation, but the child
> +	 * socket is initialized with UNSPEC (not inherited).
> +	 */
> +	ASSERT_EQ(bss->listen_set_mode_ret, 0, "listen_set_mode_ret");
> +	ASSERT_EQ(bss->listen_set_opt_ret, 0, "listen_set_opt_ret");
> +	ASSERT_EQ(bss->passive_mode_read, TCP_ECN_MODE_UNSPEC,
> +		  "passive_mode_read");

[Severity: Medium]
Will this assertion fail due to child sockets inadvertently inheriting the
listener's ECN overrides?

When a passive connection is established, tcp_create_openreq_child() in
net/ipv4/tcp_minisocks.c clones the listener socket via
inet_csk_clone_lock(). This copies the entire tcp_sock structure, including
the new ecn_mode and ecn_option fields.

Since these fields are not explicitly reset to TCP_ECN_MODE_UNSPEC during
the initialization block in tcp_create_openreq_child(), the child socket
seems to retain the listener's mode, breaking the intended design and
causing this test assertion to fail.

[ ... ]
> diff --git a/tools/testing/selftests/bpf/progs/sockops_ecn_kfunc.c b/tools/testing/selftests/bpf/progs/sockops_ecn_kfunc.c
> new file mode 100644
> index 0000000000000..32ed64ac4f3d3
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/sockops_ecn_kfunc.c
[ ... ]
> +		/* Leave the socket on the sysctl defaults so the loopback
> +		 * handshake driven by the test is not altered.
> +		 */

[Severity: Low]
Does this multi-line comment violate the BPF subsystem style rules? The
required style places the opening /* on its own line.

[ ... ]
> +	case BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB:
> +		/* Listener values consulted for SYN negotiation; child starts
> +		 * with UNSPEC (not inherited).
> +		 */

[Severity: Low]
Similarly here, should the opening /* be placed on its own line to match the
BPF comment style?

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