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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <e568d71aa0c1f7397c755ce6f0a71540931ebc3e@linux.dev>
Date: Tue, 20 May 2025 15:10:34 +0000
From: "Jiayuan Chen" <jiayuan.chen@...ux.dev>
To: "John Fastabend" <john.fastabend@...il.com>, "Cong Wang"
 <xiyou.wangcong@...il.com>
Cc: bpf@...r.kernel.org, "Jakub Sitnicki" <jakub@...udflare.com>, "David S.
 Miller" <davem@...emloft.net>, "Eric Dumazet" <edumazet@...gle.com>,
 "Jakub Kicinski" <kuba@...nel.org>, "Paolo Abeni" <pabeni@...hat.com>,
 "Simon Horman" <horms@...nel.org>, "Cong Wang" <cong.wang@...edance.com>,
 "Alexei Starovoitov" <ast@...nel.org>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH bpf-next v1] bpf, sockmap: Fix concurrency issues between
 memory charge and uncharge

May 20, 2025 at 04:00, "John Fastabend" <john.fastabend@...il.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.