Re: [PATCH net] tcp: clear sock_ops cb flags before force-closing a child socket

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] If a child socket setup fails before `tcp_ca_openreq_child()` is called, destroying the socket causes a congestion control module refcount underflow.
- [High] The exact same lockdep warning (`sock_owned_by_me`) remains unfixed in the `chtls_recv_sock` error path in the Chelsio TLS driver.
- [Medium] MPTCP subflow teardown fails to clear `bpf_sock_ops_cb_flags`, causing unintended BPF callbacks to execute on an unestablished child socket.
--

commit b448fd7ed4a1cfbee891642b3982415ce61f7c83
Author: Sechang Lim <rhkrqnwk98@gmail.com>

tcp: clear sock_ops cb flags before force-closing a child socket

This commit fixes a lockdep warning by clearing the inherited
bpf_sock_ops_cb_flags in the IPv4 and IPv6 syn_recv_sock error paths
before force-closing the child socket.

> The child is freed before it is ever established, so it should run no
> sock_ops callback. Clear its cb flags before the forced close.

[Severity: High]
This is a pre-existing issue, but does the Chelsio TLS driver also need
this fix?

Looking at chtls_recv_sock() in
drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c, the free_sk
error path uses the exact same sequence without clearing the flags:

free_sk:
	inet_csk_prepare_forced_close(newsk);
	tcp_done(newsk);

Since the socket lock is dropped by inet_csk_prepare_forced_close(),
wouldn't tcp_done() trigger the exact same lockdep warning if
BPF_SOCK_OPS_STATE_CB_FLAG was inherited?


[Severity: Medium]
This is also a pre-existing issue, but does MPTCP subflow teardown need
to clear these flags to match the intent that embryonic sockets should
not run BPF callbacks?

In net/mptcp/subflow.c:subflow_syn_recv_sock(), the dispose_child path
calls tcp_done() on the child socket without clearing the inherited flags:

dispose_child:
	mptcp_subflow_drop_ctx(child);
	tcp_rsk(req)->drop_req = true;
	inet_csk_prepare_for_destroy_sock(child);
	tcp_done(child);

While inet_csk_prepare_for_destroy_sock() maintains the socket lock
(avoiding the lockdep warning), it still allows tcp_done() ->
tcp_set_state() to execute BPF callbacks on the unestablished socket.

> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index fdc81150ff6cf..7748668dba829 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
[ ... ]
> @@ -1783,6 +1783,7 @@ struct sock *tcp_v4_syn_recv_sock(const struct sock *sk, struct sk_buff *skb,
>  	return NULL;
>  put_and_exit:
>  	newinet->inet_opt = NULL;
> +	tcp_clear_sock_ops_cb_flags(newsk);
>  	inet_csk_prepare_forced_close(newsk);
>  	tcp_done(newsk);
>  	goto exit;

[Severity: Critical]
This is a pre-existing issue, but could this error path cause a congestion
control module refcount underflow?

When a child socket is cloned via sk_clone_lock(), it inherits the
listener's icsk_ca_ops via a shallow copy, but the module reference count
is not incremented at that time. The refcount is incremented later during
tcp_ca_openreq_child().

If an error occurs before tcp_ca_openreq_child() is reached, such as
inet_csk_route_child_sock() failing earlier in this function:

	dst = inet_csk_route_child_sock(sk, newsk, req);
	if (!dst)
		goto put_and_exit;

We jump to put_and_exit. The teardown path (tcp_done() ->
tcp_v4_destroy_sock() -> tcp_cleanup_congestion_control()) will then
unconditionally call:

	bpf_module_put(icsk->icsk_ca_ops, icsk->icsk_ca_ops->owner);

Does this drop a module reference that was never acquired?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605094954.1374489-1-rhkrqnwk98@gmail.com?part=1