On Thu, Sep 24, 2026 at 11:25 AM Cai Xinchen <[email protected]> wrote: > > Hi, > > Thank you for the review. > > We found that sk_forward_alloc is a plain int updated by a non-atomic > RMW (sk_forward_alloc_add(), where even the read side is not > READ_ONCE), and its writers span three unrelated lock domains: > > - socket lock, process context: SO_RESERVE_MEM and TX grants > (__sk_mem_schedule(), sk_forced_mem_schedule()); > > - receive-queue lock, softirq: UDP RX charges and the > udp_rmem_release() fold; > > - no lock at all: sk_mem_charge()/sk_mem_uncharge() from > skb_set_owner_r() and skb destructors, and the sk_mem_reclaim() > fold itself. > > Any cross-domain pair loses an update. A lost charge is still > returned in full by the matching skb free, so it resurfaces as a > phantom surplus in sk_forward_alloc, and the next fold hands it back > to the memcg via __sk_mem_reduce_allocated() -> > mem_cgroup_sk_uncharge() - an uncharge with no matching charge. > > We first tried to fix it with locks, and hit three walls: > > - the socket lock cannot be used: its holders free skbs (e.g. > tcp_recvmsg()), and the destructor's sk_mem_uncharge() would > need to re-acquire it - recursion. That is why these helpers > are lockless in the first place; > > - a new per-socket spinlock serializes the RMWs but not the bug: > __sk_mem_schedule() publishes the grant before the memcg charge, > and the charge may sleep (GFP_KERNEL, memcg reclaim/OOM), so no > spinlock can cover both steps; a fold in that window can still > refund pages whose charge afterwards fails; > > - such a lock would also sit on the per-packet charge/uncharge > paths, exactly the hot path the cacheline layout around > sk_forward_alloc was tuned to keep cheap. > > Are there any good solutions to solve this problem?
Perfect, you now gave us what we need. UDP is broken, it should be easy to fix without breaking TCP. I am surprised your LLM went to a completelly broken path. sk_forward_alloc is never supposed to be updated locklessly across multiple lock domains: - For TCP, sk_forward_alloc is strictly serialized by the socket lock (lock_sock / bh_lock_sock). TCP does not use sock_rfree() as an skb destructor (sk_mem_uncharge() is called under the socket lock in tcp_eat_recv_skb() and sk_wmem_free_skb(), while TX destructors sock_wfree() / tcp_wfree() only touch sk_wmem_alloc). - For UDP, sk_forward_alloc is serialized by sk->sk_receive_queue.lock (in __udp_enqueue_schedule_skb() and udp_rmem_release()). Your reproducer and analysis point to two specific places that violate these locking rules: 1. SO_RESERVE_MEM on UDP sockets: SO_RESERVE_MEM (commit 2bb2f5fb21b0, "net: add new socket option SO_RESERVE_MEM") was designed for TCP, where sk_forward_alloc is protected by lock_sock(sk) and sk_mem_reclaim() checks sk_unused_reserved_mem(sk). However, sock_reserve_memory() only checks sk_has_account(sk), which also matches UDP. UDP does not support SO_RESERVE_MEM: udp_rmem_release() does not check sk_unused_reserved_mem(sk) (so the first recv() reclaims the reserved pages from sk_forward_alloc), and setsockopt(SO_RESERVE_MEM) only holds lock_sock(sk) instead of sk->sk_receive_queue.lock, racing with __udp_enqueue_schedule_skb() and udp_rmem_release(). We can fix this directly in sock_reserve_memory(): diff --git a/net/core/sock.c b/net/core/sock.c index 1d5927cd49a1..763c2017d9ef 100644 --- a/net/core/sock.c +++ b/net/core/sock.c @@ -1034,7 +1034,7 @@ static int sock_reserve_memory(struct sock *sk, int bytes) bool charged; int pages; - if (!mem_cgroup_sk_enabled(sk) || !sk_has_account(sk)) + if (!mem_cgroup_sk_enabled(sk) || !sk_is_tcp(sk)) return -EOPNOTSUPP; 2. BPF sockmap (net/core/skmsg.c): As you noted in the reproducer comments, sk_psock_skb_ingress() and sk_psock_skb_ingress_self() call sk_rmem_schedule() and skb_set_owner_r() (which installs sock_rfree() as skb->destructor) from sk_psock_backlog() or after dropping sk_receive_queue.lock in udp_read_skb(). Calling skb_set_owner_r() / sock_rfree() without the socket lock (or sk_receive_queue.lock for UDP) on protocols with sk_has_account(sk) is a bug in net/core/skmsg.c and should be fixed there.
