Re: [PATCH bpf v2 06/15] bpf, sockmap: Give no progs to a TCP listener

[Jiayuan Chen <jiayuan.chen@linux.dev>]

On 11/2/26 3:21 PM, Alexei Starovoitov wrote:
> From: Chris Mason <mason@kernel.org>
>
> A psock with a verdict prog replaces sk_data_ready and sk_write_space of
> its socket. It saves the old callbacks, and the new callbacks call them.
> sock_map_link() does this to a TCP listener too. Its new sk_data_ready
> cannot read and does not call the old one. Thus it does not wake accept().
>
> sk_clone() copies the new callbacks to each child, but not the psock.
> tcp_bpf_clone() restores only sk_prot. Without a psock the child loses data
> or does not wake its reader. If it gets a psock, the psock saves the copies
> as the old callbacks. Then sk_psock_verdict_data_ready() calls itself until
> the stack overflows. sk_psock_write_space() calls itself too, without end.
>
> Give no progs to a TCP listener, as before the first commit below. A
> listener receives and sends no data. It stays in the map and keeps its own
> callbacks. Add this exception to map_sockmap.rst. A listener no longer gets
> -EBUSY from a second map with progs of the same kind.
>
> Since the second commit below, a socket keeps its psock when TCP unhashes
> it. sock_map_link() does not see the state that the socket gets later. Do
> not depend on TCP to keep such a socket out of TCP_LISTEN. Set both
> callbacks in tcp_bpf_clone() to the TCP defaults.
>
> Fixes: 0c48eefae712 ("sock_map: Lift socket state restriction for datagram sockets")
> Fixes: b8b8315e39ff ("bpf, sockmap: Remove unhash handler for BPF sockmap usage")
> Signed-off-by: Chris Mason <mason@kernel.org>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
> ---
>  Documentation/bpf/map_sockmap.rst |  4 +++-
>  net/core/sock_map.c               |  9 +++++++++
>  net/ipv4/tcp_bpf.c                | 15 +++++++++++++--
>  3 files changed, 25 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/bpf/map_sockmap.rst b/Documentation/bpf/map_sockmap.rst
> index 2d630686a00b..eb466b86df17 100644
> --- a/Documentation/bpf/map_sockmap.rst
> +++ b/Documentation/bpf/map_sockmap.rst
> @@ -33,7 +33,9 @@ of ``__SK_DROP``, ``__SK_PASS``, or ``__SK_REDIRECT``.
>  
>  When a socket is inserted into one of these maps, its socket callbacks are
>  replaced and a ``struct sk_psock`` is attached to it. Additionally, this
> -``sk_psock`` inherits the programs that are attached to the map.
> +``sk_psock`` inherits the programs that are attached to the map. A TCP listener
> +is an exception. It gets a ``struct sk_psock``, but it keeps its callbacks and
> +inherits no programs.
>  
>  A sock object may be in multiple maps, but can only inherit a single
>  parse or verdict program. If adding a sock object to a map would result
> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index efe49551caba..282c6a4b2b05 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -228,6 +228,14 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
>  	struct sk_psock *psock;
>  	int ret;
>  
> +	/*
> +	 * A listener receives no data. With the callbacks of a verdict program
> +	 * or a parser it would no longer wake accept(), and its children would
> +	 * copy them.
> +	 */
> +	if (sk_is_tcp(sk) && sk->sk_state == TCP_LISTEN)
> +		goto no_progs;
> +
>  	stream_verdict = READ_ONCE(progs->stream_verdict);
>  	if (stream_verdict) {
>  		stream_verdict = bpf_prog_inc_not_zero(stream_verdict);
> @@ -262,6 +270,7 @@ static int sock_map_link(struct bpf_map *map, struct sock *sk)
>  		}
>  	}
>  
> +no_progs:
>  	psock = sock_map_psock_get_checked(sk);
>  	if (IS_ERR(psock)) {
>  		ret = PTR_ERR(psock);
> diff --git a/net/ipv4/tcp_bpf.c b/net/ipv4/tcp_bpf.c
> index 36d1bf46353a..fd443350f9b8 100644
> --- a/net/ipv4/tcp_bpf.c
> +++ b/net/ipv4/tcp_bpf.c
> @@ -785,7 +785,18 @@ void tcp_bpf_clone(const struct sock *sk, struct sock *newsk)
>  {
>  	struct proto *prot = newsk->sk_prot;
>  
> -	if (is_insidevar(prot, tcp_bpf_prots))
> -		newsk->sk_prot = sk->sk_prot_creator;
> +	if (!is_insidevar(prot, tcp_bpf_prots))
> +		return;
> +
> +	newsk->sk_prot = sk->sk_prot_creator;
> +	/*
> +	 * sock_map_link() gives a listener no programs, but a socket which got
> +	 * them in another state and listens now has the psock's sk_data_ready
> +	 * and sk_write_space, and sock_copy() has copied them.
> +	 * sk_psock_init() refuses a socket that has sk_user_data or a ULP, so
> +	 * what the psock replaced were the defaults of a TCP socket.
> +	 */
> +	newsk->sk_data_ready = sock_def_readable;
> +	newsk->sk_write_space = sk_stream_write_space;




Reviewed-by: Jiayuan Chen <jiayuan.chen@linux.dev>


I guess the scenario is: connect(AF_UNSPEC) unhashes a connected socket
that is still in the map, then listen() turns it into a listener. Since
we no longer have an unhash handler to drop the psock, the callbacks have
to be reset in tcp_bpf_clone().