Re: [PATCH net v10 3/3] rxrpc: bound DATA receive queues

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] rxrpc_input_data_one() (net/rxrpc/input.c:556) now charges every
  accepted DATA skb, from every call on a local endpoint, to one…
- [High] Out-of-sequence DATA can deadlock a call.
- [Medium] When a call completes, the DATA skbs on call->rx_oos_queue
  keep their charge against the shared per-local budget until…

--

rxrpc: bound DATA receive queues

This patch charges accepted DATA skbs, including split jumbo subpackets,
to the per-local asynchronous receive budget (local->rx_async_queue_bytes).
The charge is released when recvmsg consumes the packets or when they are
purged. The per-call receive window limit is now based on rx_consumed
instead of ackr_window.

Link: https://lore.kernel.org/all/cover.1791994057.git.zihanx@nebusec.ai/

> diff --git a/net/rxrpc/call_event.c b/net/rxrpc/call_event.c
> index 21be9c86d7a74..9360852604645 100644
> --- a/net/rxrpc/call_event.c
> +++ b/net/rxrpc/call_event.c
> @@ -473,6 +473,7 @@ bool rxrpc_input_call_event(struct rxrpc_call *call)
>  out:
>  	if (__rxrpc_call_is_complete(call)) {
>  		timer_delete_sync(&call->timer);
> +		rxrpc_purge_queue(&call->rx_queue);

[Severity: Medium]

Should this purge rx_oos_queue instead of rx_queue?

Nothing on call->rx_queue is charged, and rxrpc_purge_queue() only
uncharges RXRPC_SKB_MARK_SERVICE_CONN_SECURED skbs.

The DATA skbs on call->rx_oos_queue were charged in rxrpc_input_data_one().
Once the call is complete they can never be delivered, because
rxrpc_input_data() returns early for completed calls.

Those charges are only released in
rxrpc_destroy_call()->rxrpc_cleanup_rx_buffers():

	rxrpc_purge_data_queue(call, &call->rx_oos_queue);

Suppose a peer fills rx_oos_queue and then aborts the call. Does the charge
stay held against the shared local budget until the application reads the
termination and releases the call, or until a kernel user drops its last
reference?

Only the I/O thread and the destroy path touch rx_oos_queue, so it looks
safe to purge it here with rxrpc_purge_data_queue().

>  		if (!test_bit(RXRPC_CALL_DISCONNECTED, &call->flags))
>  			rxrpc_disconnect_call(call);

[ ... ]

> diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
> index 73cafe6bfa9f5..6f0ba50097390 100644
> --- a/net/rxrpc/input.c
> +++ b/net/rxrpc/input.c

[ ... ]

> @@ -511,17 +531,33 @@ static void rxrpc_input_data_one(struct rxrpc_call *call, struct sk_buff *skb,

[ ... ]

> +	sp->data.charge = NULL;
> +	if (sp->offset != sizeof(struct rxrpc_wire_header) ||
> +	    (sp->hdr.flags & RXRPC_JUMBO_PACKET)) {
> +		if (!*_charge)
> +			*_charge = rxrpc_alloc_async_queue_charge(skb);
> +		sp->data.charge = *_charge;
> +	}
> +	if (!rxrpc_charge_async_queue(call->local, skb)) {
> +		sp->data.charge = NULL;
> +		if (set_last)
> +			clear_bit(RXRPC_CALL_RX_LAST, &call->flags);
> +		ack_reason = RXRPC_ACK_NOSPACE;
>  		goto send_ack;
>  	}

[Severity: High]

Can one call use up the receive budget for the whole local endpoint here?

Every accepted DATA skb from every call on the rxrpc_local is now charged
to local->rx_async_queue_bytes. rxrpc_charge_async_queue_bytes() caps that
counter at the UDP socket's sk_rcvbuf:

net/rxrpc/skbuff.c:rxrpc_charge_async_queue_bytes() {
    ...
		if (queued &&
		    (queued >= rcvbuf || size >= rcvbuf - queued)) {
			sk_drops_inc(sk);
			return false;
		}
    ...
}

rxrpc_adjust_rcvbuf() sizes sk_rcvbuf for about one call's window and
clamps it to sysctl_rmem_max (212992 by default). That is room for about
90-100 normal DATA skbs across the whole endpoint.

Meanwhile rxrpc_fill_out_ack() still advertises up to rx_winsize (255)
packets to each peer:

net/rxrpc/output.c:rxrpc_fill_out_ack() {
    ...
	qsize = (window - 1) - call->rx_consumed;
	rsize = max_t(int, call->rx_winsize - qsize, 0);
    ...
}

The charge is released only when the application reads the data, in
rxrpc_rotate_rx_window(), or when the call is destroyed.

A remote peer could open a call to a listening service, for example kafs on
AFS_CM_PORT using its shared socket. It sends seq 1, then seq 3..N, and
never seq 2. Each of those packets is charged and queued on rx_oos_queue,
all within wlimit.

No authentication seems to be needed, since DATA is queued even while the
connection is still challenging. Each packet also refreshes expect_req_by,
so the call stays alive. A slow reader on a single call could have the same
effect.

Once the budget is full, would every other call on the endpoint get
RXRPC_ACK_NOSPACE drops?

The same counter also gates CHALLENGE/RESPONSE posting in
rxrpc_post_packet_to_conn() and OOB CHALLENGE delivery. Would connection
security setup for unrelated connections fail as well?

Before this patch DATA wasn't charged, so other calls couldn't be starved
this way. Does this need a per-call or per-peer split of the budget, or a
reserved portion? The commit message also doesn't mention that calls now
share this limit.

[Severity: High]

Can ordinary packet loss livelock a call through this check?

The charge is applied before the seq == window test. The packet that would
fill the hole therefore goes through the same budget check as
out-of-sequence packets, with no exemption or reserved space.

With the defaults, rxrpc_rx_window_size is 255 and the budget is clamped to
rmem_max, roughly 90-100 skbs at a typical truesize. A Linux sender can
have up to RXRPC_TX_MAX_WINDOW (128) packets in flight.

Suppose the packet at seq == window is lost early in a large flight. The
packets after it are charged and queued on rx_oos_queue until the budget is
nearly full.

Every retransmission of the missing packet would then fail here and be
dropped with RXRPC_ACK_NOSPACE. The OOS packets move to recvmsg_queue only
after the hole is filled:

	while ((oos = skb_peek(&call->rx_oos_queue))) {

rxrpc_recvmsg_data() only reads recvmsg_queue, so the application can't
release anything either.

Does anything break this cycle other than call destruction? The
retransmissions keep refreshing the call timers. The call could then stall
indefinitely while holding the shared budget, which also starves the other
calls on the same rxrpc_local.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1792168462.git.zihanx%40nebusec.ai