Re: [PATCH net v4 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/ca1d010bc963319d4167b8e985b5afac5912f6da.1788878590.git.zihanx@xxxxxxxxxx?part=1
---
rxrpc: fix encap_rcv skb accounting exhaustion

This charges encapsulated packets against the UDP tunnel socket's receive
buffer in rxrpc_encap_rcv() and drops them once sk_rcvbuf is reached, then
orphans PACKET skbs when the I/O thread dequeues them.  It also moves the
sk_user_data clearing under RCU and defers sock_release() until after the
local queues are purged.

Link: https://lore.kernel.org/all/cover.1788539302.git.zihanx@xxxxxxxxxx/
Link: https://lore.kernel.org/all/cover.1785339953.git.zihanx@xxxxxxxxxx/
Link: https://lore.kernel.org/all/cover.1784742007.git.zihanx@xxxxxxxxxx/

> diff --git a/net/rxrpc/io_thread.c b/net/rxrpc/io_thread.c
> index dc5184a2fa9d..415b05f5e2b4 100644
> --- a/net/rxrpc/io_thread.c
> +++ b/net/rxrpc/io_thread.c
> @@ -13,6 +13,22 @@ static int rxrpc_input_packet_on_conn(struct rxrpc_connection *conn,
>  				      struct sockaddr_rxrpc *peer_srx,
>  				      struct sk_buff *skb);
>  
> +/*
> + * Drop UDP rmem ownership for packets charged in encap_rcv().
> + * sk_forward_alloc is serialised by sk_receive_queue.lock.
> + */
> +static void rxrpc_skb_orphan_udp(struct sk_buff *skb)
> +{
> +	struct sock *sk = skb->sk;
> +
> +	if (!sk)
> +		return;
> +
> +	spin_lock_bh(&sk->sk_receive_queue.lock);
> +	skb_orphan(skb);
> +	spin_unlock_bh(&sk->sk_receive_queue.lock);
> +}
> +
>  /*
>   * handle data received on the local endpoint
>   * - may be called in interrupt context

[ ... ]

> @@ -52,6 +66,22 @@ int rxrpc_encap_rcv(struct sock *udp_sk, struct sk_buff *skb)
>  	}
>  #endif
>  
> +	spin_lock(&udp_sk->sk_receive_queue.lock);
> +	if (atomic_read(&udp_sk->sk_rmem_alloc) >= READ_ONCE(udp_sk->sk_rcvbuf) ||
> +	    !sk_rmem_schedule(udp_sk, skb, skb->truesize)) {

[Severity: High]

Which sk_rcvbuf value does this end up comparing against?  Nothing in
net/rxrpc/ sizes the tunnel socket's receive buffer.  rxrpc_open_socket()
calls udp_sock_create() and setup_udp_tunnel_sock(), neither of which touches
it, so it stays at what sock_init_data() assigned:

net/core/sock.c:sock_init_data() {
	...
	sk->sk_rcvbuf		=	READ_ONCE(sysctl_rmem_default);
	...
}

That is roughly 208 KiB of truesize for the entire local endpoint, and since
this is a kernel socket with no fd, the only way to change it is the global
net.core.rmem_default.

Meanwhile rxrpc keeps advertising a 255 packet receive window per call:

net/rxrpc/misc.c:
unsigned int rxrpc_rx_window_size = 255;

net/rxrpc/conn_event.c:rxrpc_conn_retransmit_call() {
	...
	trailer.rwind		= htonl(rxrpc_rx_window_size);
	...
}

with rxrpc_rx_mtu at RXRPC_JUMBO(46).  Both are sysctl-tunable; the new cap
is not.

The test is a hard >= with no headroom, and the charge is only released one
skb at a time when the MIN_NICE krxrpcio thread dequeues.  At 2-4 KiB truesize
per MTU-sized skb, 50-90 queued skbs exhaust the budget, which a single
softirq round can deliver before the I/O thread runs.

Can a compliant AFS peer that simply fills the window rxrpc advertised to it
get dropped here, with no EXCEEDS_WINDOW ACK and recovery only via RTO?  The
resulting threshold, how it was derived, and the disagreement with the
advertised window aren't mentioned in the commit message or the v2/v3/v4
changelog.

> +		spin_unlock(&udp_sk->sk_receive_queue.lock);
> +		sk_drops_inc(udp_sk);
> +		kfree_skb(skb);
> +		return 0;
> +	}

[Severity: Low]

The drop returns 0, which UDP counts as a delivered datagram:

net/ipv4/udp.c:udp_queue_rcv_one_skb() {
	...
		ret = encap_rcv(sk, skb);
		if (ret <= 0) {
			__UDP_INC_STATS(net, UDP_MIB_INDATAGRAMS);
			return -ret;
		}
	...
}

So each dropped datagram bumps UDP_MIB_INDATAGRAMS and neither
UDP_MIB_RCVBUFERRORS nor UDP_MIB_INERRORS is incremented, unlike UDP's own
rcvbuf-overflow path for the same socket type
(__udp_enqueue_schedule_skb() -> -ENOMEM -> RCVBUFERRORS + INERRORS).
Under the flood this patch bounds, netstat -su shows InDatagrams climbing and
zero receive-buffer errors while sk_drops rises.

The hook return value can't express this (a negative return makes
udp_queue_rcv_one_skb() resubmit to the protocol and still counts
INDATAGRAMS), so would an explicit __UDP_INC_STATS(net, UDP_MIB_RCVBUFERRORS)
plus UDP_MIB_INERRORS here be appropriate, along with a specific
skb_drop_reason instead of the bare kfree_skb()?

> +
> +	skb->dev = NULL;
> +	skb_set_owner_r(skb, udp_sk);

[Severity: Medium]

This consumes the same counter and the same limit that the socket error queue
is gated on:

net/core/skbuff.c:sock_queue_err_skb() {
	if (atomic_read(&sk->sk_rmem_alloc) + skb->truesize >=
	    (unsigned int)READ_ONCE(sk->sk_rcvbuf))
		return -ENOMEM;
	...
}

rxrpc's only ICMP ingress is rxrpc_encap_err_rcv() ->
ip_icmp_error()/ipv6_icmp_error() -> sock_queue_err_skb().  On -ENOMEM the
notification is kfree_skb()'d, sk_error_report() is never called, so
rxrpc_error_report() never queues an RXRPC_SKB_MARK_ERROR skb and
rxrpc_input_error() never runs.

Does that silently drop port-unreachable (peer-dead) detection and
ICMP_FRAG_NEEDED path-MTU input for the whole endpoint for as long as the rx
queue is near full, i.e. exactly the state this patch holds during a flood?

Before this change the encap path never touched sk_rmem_alloc, so the budget
was effectively reserved for error skbs.  Error skbs also stay charged after
rxrpc_error_report() moves them onto local->rx_queue, since the patch
deliberately does not orphan them, which widens the window further.

> +	spin_unlock(&udp_sk->sk_receive_queue.lock);
> +	skb_dst_force(skb);

[Severity: Low]

What consumes the forced dst reference, and why is skb->dev cleared above?

A git grep for skb_dst and skb->dev in net/rxrpc/ matches only these two new
lines, and the only dst dereference in rxrpc (dst->dev->gso_max_segs in
peer_object.c) works on an output route lookup.

Before this change the queued skb carried a SKB_DST_NOREF dst, which
refdst_drop() skips on free; skb_dst_force() promotes it to a refcounted dst
held until the skb is finally freed.  UDP's own queueing path drops the dst
instead:

net/ipv4/udp.c:udp_queue_rcv_one_skb() {
	...
	ipv4_pktinfo_prepare(sk, skb, true);
	return __udp_queue_rcv_skb(sk, skb);
}

Would skb_dst_drop() be what is wanted here?  rxrpc_skb_orphan_udp() releases
the rmem charge at dequeue but leaves the dst in place, so DATA packets handed
to a call's recvmsg queue keep the reference for a
remote-sender-plus-slow-reader controlled interval that is not covered by the
new sk_rcvbuf cap, because dst size is not part of skb->truesize.

Neither skb->dev = NULL nor skb_dst_force() is described in the commit message
or the v2/v3/v4 changelog.

> +
> +	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;
> @@ -471,6 +501,9 @@ int rxrpc_io_thread(void *data)
>  		/* Distribute packets and errors. */
>  		while ((skb = __skb_dequeue(&rx_queue))) {
>  			struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
> +
> +			if (skb->mark == RXRPC_SKB_MARK_PACKET)
> +				rxrpc_skb_orphan_udp(skb);
>  			switch (skb->mark) {
>  			case RXRPC_SKB_MARK_PACKET:
>  				skb->priority = 0;

[ ... ]