Re: [PATCH v2 bpf] sockmap: Fix sk_psock_drop() race vs sock_map_{unhash,close,destroy}().
Martin KaFai Lau <[email protected]>
| Newsgroups | dev.linux.lists.sashiko,org.kernel.vger.bpf |
|---|---|
| Message-ID | <[email protected]> |
On Tue, Apr 21, 2026 at 11:21:27AM +0800, Jiayuan Chen wrote:
> > > @@ -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.
> > 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.
The same SOCK_DEAD reasoning applies to PREEMPT_RT?
It is useful to have some explanation in the commit message for this case.
Kuniyuki, does the above make sense? I can fold it in before landing.