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

[zihan xi]

On Sat, Sep 12, 2026 at 6:28 PM Simon Horman <horms@xxxxxxxxxx> 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/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)) {
>
Hi Simon,

Thanks for the review. All four points are valid;

> [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.

Yes. The tunnel socket is left at sysctl_rmem_default, which is
smaller than one advertised window of ordinary DATA, so a peer
filling that window can be dropped here with no EXCEEDS_WINDOW ACK.
>
> > +             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()?

Yes. return 0 is counted as a delivered datagram. A negative return
is not usable here either: it resubmits as proto -N and still
increments INDATAGRAMS. Extra RCVBUFERRORS / INERRORS and a
skb_drop_reason are appropriate.
>
> > +
> > +     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.

Yes. Charging DATA against the same rmem cap can starve
sock_queue_err_skb(), so ICMP is dropped and rxrpc_error_report()
never runs. Before this change that budget was effectively reserved
for error skbs.
>
> > +     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.

Yes. skb_dst_force() was the wrong helper. The dst is not part of
skb->truesize, so it is not covered by the rmem cap.
skb_dst_drop() is what is wanted. skb->dev = NULL is only to avoid
holding a device reference on a queued RxRPC skb.

Thanks,
Zihan Xi
>
> > +
> > +     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;
>
> [ ... ]