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

On 4/21/26 9:13 AM, [email protected] wrote:
> 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 <[email protected]>
> 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/[email protected]/
>
>> 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.


sock_map_close(sk)
|___ sk_psock_put(sk, psock)   <- refcnt-hits-0 window lives here
|___ saved_close == tcp_close
       |__tcp_close
             |____ sock_orphan   <- SOCK_DEAD set here
             |____(later) inet_csk_destroy_sock

At the exact instant the refcnt can be observed at 0 with
sk_prot not yet restored, SOCK_DEAD is guaranteed not to be set.

So I think AI is not correct.

> 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.
>