Re: [PATCH net v9 03/14] rxrpc: Fix lack of loop around reply send in rxperf server

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

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

- [Low] Claimed infinite loop in the new `do { ... } while (n >= 0 &&
  msg_data_left(&msg) > 0)` retry loop in rxperf_process_call()…

--

rxrpc: Fix lack of loop around reply send in rxperf server

This wraps the magic-cookie send in rxperf_process_call() in a loop so a
short send is retried, and changes the error path to always propose an
abort with the send error rather than only on -ENOMEM.  The
rxperf_abort_oom trace enum is replaced by rxperf_abort_send_error.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824091645.415423-1-dhowells%40redhat.com

[Severity: Low]
This isn't a bug as far as I could tell, but should the new loop also treat
a zero return as a failure the way the reply-payload loop just above it in
rxperf_process_call() does?

The earlier loop in the same function bails out explicitly:

	n = rxrpc_kernel_send_data(rxperf_socket, call->rxcall, &msg,
				   len, rxperf_notify_end_reply_tx);
	if (n < 0)
		return n;
	if (n == 0)
		return -EIO;

whereas the new loop counts n == 0 as progress and re-enters
rxrpc_kernel_send_data() while msg_data_left(&msg) is still non-zero.

I tried to construct a path where rxrpc_kernel_send_data() returns exactly 0
with the four cookie bytes still pending and could not find one in this tree.
A positive return is the number of bytes copied out of msg->msg_iter, every
error path in rxrpc_send_data() returns a strictly negative value, and the
only routes to a zero return are:

net/rxrpc/sendmsg.c:rxrpc_kernel_send_data() {
	...
	if (ret == -ESHUTDOWN)
		ret = call->error;
	...
}

and the call_terminated path returning call->error, both of which need
call->error == 0, i.e. RXRPC_CALL_SUCCEEDED.  For a service call that state
is only reached from RXRPC_CALL_SERVER_AWAIT_ACK in
rxrpc_end_tx_phase()->rxrpc_call_completed(), and AWAIT_ACK is only entered
by rxrpc_close_tx_phase() once a txb carrying RXRPC_LAST_PACKET has been
transmitted, which itself requires the caller's iterator to have been fully
consumed.  So the two conditions look mutually exclusive and the loop
terminates today.

Given that, would adding the same zero-progress escape make the new loop
match its sibling and not depend on that implicit state-machine property?

	do {
		n = rxrpc_kernel_send_data(rxperf_socket, call->rxcall,
					   &msg, msg_data_left(&msg),
					   rxperf_notify_end_reply_tx);
		if (n == 0) {
			n = -EIO;
			break;
		}
	} while (n > 0 && msg_data_left(&msg) > 0);

For reference, the later patch "rxrpc: Fix sendmsg to not return an error if
last packet queued" documents the return contract as the number of bytes
buffered, which can be 0 when the iterator holds 0 bytes; that is consistent
with the loop terminating here.  rxperf.c is not touched again later in the
series, so the loop stays as introduced.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907113743.1453210-1-dhowells%40redhat.com