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

[Simon Horman]

From: 'Simon Horman' <horms@xxxxxxxxxx>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
tcp: clear sock_ops cb flags before force-closing a child socket

This patch clears the inherited bpf_sock_ops_cb_flags on a child socket
before inet_csk_prepare_forced_close() drops the socket lock, so the
subsequent tcp_done() does not invoke tcp_call_bpf() without the lock and
trip sock_owned_by_me(). The fix is applied at the put_and_exit labels in
tcp_v4_syn_recv_sock() and tcp_v6_syn_recv_sock().

> diff --git a/include/net/tcp.h b/include/net/tcp.h
> index 98848db62894..97eac5fa341c 100644
> --- a/include/net/tcp.h
> +++ b/include/net/tcp.h
> @@ -2942,6 +2942,11 @@ static inline int tcp_call_bpf_3arg(struct sock *sk, int op, u32 arg1, u32 arg2,
>  	return tcp_call_bpf(sk, op, 3, args);
>  }
>  
> +static inline void tcp_clear_sock_ops_cb_flags(struct sock *sk)
> +{
> +	tcp_sk(sk)->bpf_sock_ops_cb_flags = 0;
> +}
> +
>  #else

[ ... ]

> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index fdc81150ff6c..7748668dba82 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;

[High]
Should the same clearing also be added to chtls_recv_sock() in
drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c?

That function clones a child via tcp_create_openreq_child() ->
inet_csk_clone_lock() (which inherits the listener's
bpf_sock_ops_cb_flags), and on its error paths (dst lookup, neighbour
lookup, port mismatch, chtls_sock_create(), or l2t_entry allocation
failure) it falls through to free_sk:

drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_cm.c:chtls_recv_sock() {
    [ ... ]
free_sk:
	inet_csk_prepare_forced_close(newsk);
	tcp_done(newsk);
free_oreq:
	chtls_reqsk_free(oreq);
	return NULL;
}

That sequence looks like it would hit the same sock_owned_by_me() warning
in tcp_set_state() -> tcp_call_bpf() if BPF_SOCK_OPS_STATE_CB_FLAG was set
on the listener, which is the case the commit message describes.

Would it be more robust to clear the cb flags inside
inet_csk_prepare_forced_close() itself so the invariant holds for every
present and future caller, or alternatively to add
tcp_clear_sock_ops_cb_flags(newsk) at the chtls free_sk label as well?