Re: [PATCH v2 bpf] sockmap: Fix sk_psock_drop() race vs sock_map_{unhash,close,destroy}().

[sashiko-bot@kernel.org]

Sashiko AI review found 1 potential issue(s):
- [High] The newly introduced unbounded `goto retry;` loops in `sock_map_unhash()`, `sock_map_destroy()`, and `sock_map_close()` can cause a hard lockup (deadlock) due to softirq preemption or priority inversion.
--

commit 08b24da207c576eef44ad9d88356dbd6600ab733
Author: Kuniyuki Iwashima <kuniyu@google.com>
sockmap: Fix sk_psock_drop() race vs sock_map_{unhash,close,destroy}().

This commit fixes a race condition where sk_psock_drop() might not have
finished restoring sk->sk_prot even if sk_psock_get() fails to bump the
refcount. It adds a retry loop to sock_map_unhash(), sock_map_destroy(),
and sock_map_close() when sk_psock_get() returns NULL but the protocol
hooks haven't been restored yet.

Closes: https://lore.kernel.org/bpf/69cec5ef.050a0220.2dbe29.0009.GAE@google.com/

> diff --git a/net/core/sock_map.c b/net/core/sock_map.c
> index 02a68be3002a2..99e3789492a09 100644
> --- a/net/core/sock_map.c
> +++ b/net/core/sock_map.c
[ ... ]
> @@ -1652,20 +1657,25 @@ void sock_map_destroy(struct sock *sk)
>  	void (*saved_destroy)(struct sock *sk);
>  	struct sk_psock *psock;
>  
> +retry:
>  	rcu_read_lock();
>  	psock = sk_psock_get(sk);
>  	if (unlikely(!psock)) {
>  		rcu_read_unlock();
>  		saved_destroy = READ_ONCE(sk->sk_prot)->destroy;
> +		if (unlikely(saved_destroy == sock_map_destroy))
> +			goto retry;

Can this unbounded retry loop cause a hard lockup due to softirq preemption
or priority inversion?

If sk_psock_put() is called from process context with bottom-halves enabled
(such as from sock_map_close() after release_sock()), an interrupt could
preempt the thread immediately after refcount_dec_and_test(&psock->refcnt)
drops to 0 but before sk_psock_drop() disables BH.

If a network interrupt fires on the same CPU, NET_RX_SOFTIRQ may process a
packet (such as an RST) that triggers socket destruction via:
tcp_done() -> inet_csk_destroy_sock() -> sk->sk_prot->destroy()

Since sk_psock_drop() has not yet restored the protocol, sk->sk_prot->destroy
is still sock_map_destroy().

When sock_map_destroy() calls sk_psock_get(), it returns NULL because the
refcount is exactly 0. The code then falls into the !psock branch, sees that
sk->sk_prot->destroy is still sock_map_destroy(), and jumps to retry.

Because the softirq spins infinitely in this tight loop and never yields the
CPU, the preempted process context can never execute sk_psock_drop(),
resulting in a permanent hard lockup.

A similar priority inversion deadlock could also occur on PREEMPT_RT if the
thread calling sk_psock_drop() is preempted by a higher-priority task.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260420194846.1089595-1-kuniyu@google.com?part=1