Re: [PATCH bpf v2 1/4] bpf, sockmap: Reject unhashed UDP sockets on sockmap update

[sashiko-bot@kernel.org]

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

New issues:
- [High] Incomplete fix: Memory ordering race allows unbound UDP sockets to be inserted into sockmap, bypassing the sk_hashed() check.

Pre-existing issues:
- [High] The patch addresses a memory leak caused by UDP sockets dynamically acquiring the SOCK_RCU_FREE flag while held by a BPF program, but leaves TCP sockets vulnerable to the exact same state-transition leak.
--

commit f2013e408c7b5bf3e40a884d6c22c888b4372b0b
Author: Michal Luczaj <mhal@rbox.co>

bpf, sockmap: Reject unhashed UDP sockets on sockmap update

This commit rejects unhashed UDP sockets during sockmap updates to prevent
a memory leak. The leak occurred because unbound UDP sockets are refcounted,
but become non-refcounted (SOCK_RCU_FREE) when bound, which skips the
refcount decrement in bpf_sk_release().

> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index c60ba6d292f9a..9efbd8ca7db83 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
> @@ -542,6 +542,8 @@ static bool sock_map_sk_state_allowed(const struct sock *sk)
>  {
>  	if (sk_is_tcp(sk))
>  		return (1 << sk->sk_state) & (TCPF_ESTABLISHED | TCPF_LISTEN);

[Severity: High]
This is a pre-existing issue, but does this leave TCP sockets susceptible to
the exact same state-transition leak?

If a TCP socket is held by a BPF program in TCP_ESTABLISHED state and then
transitions to TCP_LISTEN, inet_hash() unconditionally sets SOCK_RCU_FREE:

net/ipv4/inet_hashtables.c:inet_hash() {
    ...
    sock_set_flag(sk, SOCK_RCU_FREE);
    ...
}

This dynamic state change would cause bpf_sk_release() to evaluate
sk_is_refcounted(sk) as false and skip the refcount decrement, leading to
a memory leak similar to the UDP case.

> +	if (sk_is_udp(sk))
> +		return sk_hashed(sk);

[Severity: High]
Could a memory ordering regression here allow unbound UDP sockets to still
bypass this check?

In udp_v4_get_port(), the flag and hash are set:

net/ipv4/udp.c:udp_v4_get_port() {
    ...
    sock_set_flag(sk, SOCK_RCU_FREE);
    sk_add_node_rcu(sk, &hslot->head);
    ...
}

Because these stores lack a memory barrier between them, could sk_hashed(sk)
return true on another CPU while SOCK_RCU_FREE is not yet visible, allowing
the leak to still trigger?

>  	if (sk_is_stream_unix(sk))
>  		return (1 << READ_ONCE(sk->sk_state)) & TCPF_ESTABLISHED;
>  	if (sk_is_vsock(sk) &&

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626-sockmap-lookup-udp-leak-v2-0-7e7e201c951a@rbox.co?part=1