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