Re: [PATCH net v3 1/1] rxrpc: fix encap_rcv skb accounting exhaustion

On Tue, Sep 8, 2026 at 8:17 PM Simon Horman [off-list ref] wrote:

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/a2bd6f3ed29919a7328363b548299ae583f052b0.1788539302.git.zihanx@nebusec.ai?part=1
---

quoted

diff --git a/net/rxrpc/io_thread.c b/net/rxrpc/io_thread.c
--- a/net/rxrpc/io_thread.c
+++ b/net/rxrpc/io_thread.c

@@ -52,6 +50,19 @@ int rxrpc_encap_rcv(struct sock *udp_sk, struct sk_buff *skb)
      }
 #endif

+     if (atomic_read(&udp_sk->sk_rmem_alloc) >= READ_ONCE(udp_sk->sk_rcvbuf) ||
+         !sk_rmem_schedule(udp_sk, skb, skb->truesize)) {
+             sk_drops_inc(udp_sk);
+             kfree_skb(skb);
+             return 0;
+     }
+
+     skb->dev = NULL;
+     skb_set_owner_r(skb, udp_sk);
+     skb_dst_force(skb);
+
+     skb->mark = RXRPC_SKB_MARK_PACKET;
+     rxrpc_new_skb(skb, rxrpc_skb_new_encap_rcv);
      skb_queue_tail(rx_queue, skb);
      wake_up_process(io_thread);
      return 0;

[Severity: High]
This borrows UDP's rmem accounting without borrowing the lock that
protects it.

rxrpc_encap_rcv() is called from the UDP receive path in softirq
context with neither lock_sock(udp_sk) nor
udp_sk->sk_receive_queue.lock held, and nothing serialises two CPUs
delivering to the same local endpoint. Both new accounting calls in
this hunk perform an unsynchronised read-modify-write on
udp_sk->sk_forward_alloc: sk_rmem_schedule() reaches
__sk_mem_schedule()/sk_forward_alloc_add(), and skb_set_owner_r()
calls sk_mem_charge(), which is the same non-atomic
WRITE_ONCE(sk->sk_forward_alloc, ...) update. The newly added
skb_orphan() in rxrpc_io_thread() closes the loop from the kthread
side: sock_rfree() -> sk_mem_uncharge() does the same non-atomic
update concurrently with softirq enqueues on other CPUs. Note that
only sk_rmem_alloc is atomic_t here; sk_forward_alloc is not, so the
atomic_read() guard at the top of the hunk does not make any of this
safe.

The normal UDP path does not have this problem because
__udp_enqueue_schedule_skb() performs the forward-alloc bookkeeping
and the skb_set_owner_r() under sk->sk_receive_queue.lock, and
udp_rmem_release() takes the same lock before reclaiming. The
consequence of losing that serialisation is not confined to this
socket: the drifted sk_forward_alloc is eventually handed to
__sk_mem_reduce_allocated(), which subtracts it from the protocol-wide
udp_prot memory_allocated counter, so a flood of RxRPC-shaped packets
can skew global UDP memory accounting for every UDP socket on the
machine. That is a worse failure mode than the unbounded rx_queue this
patch is trying to fix.

How do you want to serialise this? Options I can see are enqueueing
via the existing UDP receive-queue locking rather than open-coding the
charge, or holding local->rx_queue.lock across both the charge in
rxrpc_encap_rcv() and the skb_orphan() in rxrpc_io_thread() so the
charge and uncharge sides are mutually exclusive. Please say which you
intend, and make sure the answer covers the I/O-thread uncharge path
too, not just the two enqueue-side calls.

Thanks for the review. I agree this is racy: sk_forward_alloc is not
atomic, so a BH encap_rcv() on another CPU can update it while the
I/O thread orphans the skb.

I intend to keep the skbs on the RxRPC local queue and serialise UDP
rmem with sk->sk_receive_queue.lock, the same lock UDP uses:

- rxrpc_encap_rcv() already runs in BH, so it can take spin_lock()
  around sk_rmem_schedule() and skb_set_owner_r();
- the I/O thread takes spin_lock_bh() around skb_orphan() for PACKET
  skbs charged in encap_rcv(), so a concurrent BH charge cannot run
  at the same time.

I will not enqueue these packets on the UDP receive queue.

A v4 addressing this is coming shortly.