Re: [PATCH bpf-next v1] bpf, sockmap: Fix concurrency issues between memory charge and uncharge

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

May 20, 2025 at 04:00, "John Fastabend" <john.fastabend@gmail.com> wrote:

> 
> On 2025-05-18 11:48:31, Cong Wang wrote:
> 
[...]
> > 
> >  Solution:
> > 
> >  1. Add locking to the kfree_sk_msg() process, which is only called in the
> > 
> >  user process context.
> > 
> >  2. Integrate the charge process into sk_psock_create_ingress_msg() in the
> > 
> >  backlog process and add locking.
> > 
> >  3. Reuse the existing psock->ingress_lock.
> > 
> >  
> > 
> >  Reusing the psock->ingress_lock looks weird to me, as it is intended for
> > 
> >  locking ingress queue, at least at the time it was introduced.
> > 
> >  
> > 
> >  And technically speaking, it is the sock lock which is supposed to serialize
> > 
> >  socket charging.
> > 
> >  
> > 
> >  So is there any better solution here?
> > 
> 
> Agree I would be more apt to add the sock_lock back to the backlog then
> 
> to punish fast path this way.
> 
> Holding the ref cnt on the psock stops blocks the sk_psock_destroy() in
> 
> backlog now so is this still an issue?
> 
> Thanks,
> 
> John
>

Thanks to Cong and John for their feedback.

For TCP, lock_sock(sk) works as expected. However, since we now support
multiple socket types (UNIX, UDP), the locking mechanism must be adapted
accordingly.

For UNIX sockets, we must use u->iolock instead of lock_sock(sk) in the
backlog path. This is because we already acquire lock(u->iolock) in both:
'''
unix_bpf_recvmsg() (BPF handler)
unix_stream_read_generic() (native handler)
'''

For UDP, the native handler __skb_recv_udp() locks udp_sk(sk)->reader_queue->lock,
but no locking is implemented in udp_bpf_recvmsg(). This implies that ingress_lock
effectively serves the same purpose as udp_sk(sk)->reader_queue->lock to prevent
concurrent user-space access.

Conclusion:
To avoid using ingress_lock, we need to implement a per-socket locking strategy into psock:

Default implementation: lock_sock(sk)
UNIX sockets: Use lock(u->iolock) in backlog path.
UDP sockets: Explicitly use reader_queue->lock both in udp_bpf_recvmsg() and backlog path.

As of now, I don’t have any better ideas.