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

[Simon Horman]

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@xxxxxxxxxx?part=1
---
> 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.