Re: [PATCH net v10 06/14] rxrpc: Fix error handling in rxrpc_send_data()

[netdev-bot+sashiko@kernel.org]

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

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

- [High] rxrpc_send_data returns call->error (e.g., -ECONNABORTED)
  instead of -ESHUTDOWN when the call completes while sleeping, causing…
- [Medium] rxrpc_send_data(): the revalidation block moved/changed at the
  `reload:` label now discards an already-non-zero `copied`, and the…
- [Medium] net/rxrpc/rxperf.c: converting rxperf_process_call() to `void`
  and calling it as `case 0: rxperf_process_call(call); return;` removes…
- [Medium] Moving the RXRPC_CALL_TX_NO_MORE test from the top of
  rxrpc_send_data() to the `reload:` label (and turning `return…
- [Medium] net/rxrpc/rxperf.c, rxperf_deliver_to_call(): the new
  RXPERF_CALL_SV_AWAIT_ACK handler cannot distinguish "still waiting for…
- [Low] The patch changes partial-send semantics for conditions unrelated
  to call completion without disclosing it in the commit message.
- [Low] The explanatory comment added by this patch after the new `out:`
  label states a precedence in which "(6) If some data has been copied…

--

rxrpc: Fix error handling in rxrpc_send_data()

This reworks the return-value handling of rxrpc_send_data() so that a call
which completes after the final packet has been queued no longer causes
sendmsg() to report an error, and returns -ESHUTDOWN when the call is
already complete and the last packet has not been queued yet.  The rxperf
sample server and the afs error path are adjusted to match.

A few questions about the new control flow below.

> diff --git a/net/rxrpc/rxperf.c b/net/rxrpc/rxperf.c
> index 7e9a536153098..83016830e6965 100644
> --- a/net/rxrpc/rxperf.c
> +++ b/net/rxrpc/rxperf.c

[ ... ]

> @@ -293,18 +293,28 @@ static void rxperf_deliver_to_call(struct work_struct *work)
>  	       state == RXPERF_CALL_SV_AWAIT_ACK
>  	       ) {
>  		if (state == RXPERF_CALL_SV_AWAIT_ACK) {
> -			if (!rxrpc_kernel_check_life(rxperf_socket, call->rxcall))
> +			size_t len = 0;
> +			iov_iter_kvec(&call->iter, ITER_DEST, NULL, 0, 0);
> +			ret = rxrpc_kernel_recv_data(rxperf_socket,
> +						     call->rxcall, &call->iter,
> +						     &len, false, &remote_abort,
> +						     &call->service_id);
> +
> +			if (ret == -EINPROGRESS || ret == -EAGAIN)
> +				return;
> +			if (ret < 0 || ret == 1) {
> +				if (ret == 1)
> +					ret = 0;
>  				goto call_complete;
> +			}
>  			return;
>  		}

[Severity: Medium]
Can this new AWAIT_ACK check tell "still waiting for the final ACK" apart
from "call complete"?

Once the whole request has been read, rxrpc_rotate_rx_window() has set the
read-all flag:

net/rxrpc/recvmsg.c:rxrpc_rotate_rx_window() {
	...
	set_bit(RXRPC_CALL_RECVMSG_READ_ALL, &call->flags);
	...
}

and rxrpc_recvmsg_data() then short-circuits:

net/rxrpc/recvmsg.c:rxrpc_recvmsg_data() {
	...
	if (test_bit(RXRPC_CALL_RECVMSG_READ_ALL, &call->flags)) {
		seq = call->ackr_window - 1;
		ret = 1;
		goto done;
	}
	...
}

so rxrpc_kernel_recv_data() returns 1 (read_phase_complete) and the new code
always takes the "ret == 1" branch to call_complete, running
rxperf_set_call_complete(), rxrpc_kernel_shutdown_call(),
rxrpc_kernel_put_call() and kfree(call) while the final ACK for the reply is
still outstanding.  rxrpc_kernel_shutdown_call() documents the opposite
expectation:

/* Allow a kernel service to shut down a call it was using.  The call must be
 * complete before this is called (the call should be aborted if necessary).
 */

The replaced rxrpc_kernel_check_life() returned true while the call was not
complete, so the handler waited instead.  Re-entering the work item in
SV_AWAIT_ACK is routine: any rxrpc_notify_socket() that fires while the work
item runs (for example request DATA packets queued in sequence for a
multi-packet request) re-queues call->work, which then runs again after
rxperf_notify_end_reply_tx() has moved the state to SV_AWAIT_ACK.

Two of the new tests also look unreachable: rxrpc_kernel_recv_data() does not
return -EINPROGRESS (that value only comes from rxperf's own ->deliver()
functions), and with want_more clear a 0 result from rxrpc_recvmsg_data()
is turned into -EMSGSIZE by the excess_data path, so the trailing "return;"
for the keep-waiting case cannot be reached.

>  
>  		ret = call->deliver(call);
> -		if (ret == 0)
> -			ret = rxperf_process_call(call);
> -
>  		switch (ret) {
>  		case 0:
> -			continue;
> +			rxperf_process_call(call);
> +			return;

[Severity: Medium]
Can this leak the struct rxperf_call and the rxrpc_call reference?

Discarding the reply-send result here removes the only path by which a send
failure reached call_complete (rxperf_set_call_complete() +
rxrpc_kernel_shutdown_call() + rxrpc_kernel_put_call() + cancel_work() +
kfree(call)).  Teardown now depends on a further rxrpc notification
re-queueing call->work, and notifications stop once the socket is closing:

net/rxrpc/recvmsg.c:rxrpc_notify_socket() {
	...
	if (rx && sk->sk_state < RXRPC_CLOSE) {
	...
}

net/rxrpc/af_rxrpc.c:rxrpc_shutdown() {
	...
	sk->sk_state = RXRPC_CLOSE;
	sk->sk_shutdown = SHUTDOWN_MASK;
	...
}

So for rxperf_exit() -> rxperf_close_socket() -> kernel_sock_shutdown() while
a work item is inside rxperf_process_call(), the next
rxrpc_kernel_send_data() returns -EPIPE, the new void error path only
proposes an abort:

send_error:
	rxrpc_kernel_abort_call(rxperf_socket, call->rxcall,
				RXGEN_SS_MARSHAL, n,
				rxperf_abort_send_error);

and rxperf_deliver_to_call() has already returned.  The abort completes the
call in the I/O thread, but the notification is suppressed because sk_state
is RXRPC_CLOSE, so call->work is never re-queued and nothing frees the call.
Before this patch the same -EPIPE reached the switch default and always went
to call_complete.

[ ... ]

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> index b370e440e2fdb..1012d2d337a55 100644
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c
> @@ -329,13 +329,6 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  	bool more = msg->msg_flags & MSG_MORE;
>  	int ret, copied = 0;
>  
> -	if (test_bit(RXRPC_CALL_TX_NO_MORE, &call->flags)) {
> -		trace_rxrpc_abort(call->debug_id, rxrpc_sendmsg_late_send,
> -				  call->cid, call->call_id, call->rx_consumed,
> -				  0, -EPROTO);
> -		return -EPROTO;
> -	}
> -

[Severity: Medium]
Does this hunk also fix a leaked call->user_mutex introduced earlier in the
same series?

In the pre-image rxrpc_send_data() is already annotated
__releases(&call->user_mutex) and both callers depend on it to unlock:

net/rxrpc/sendmsg.c:rxrpc_kernel_send_data() {
	...
	mutex_lock(&call->user_mutex);
	ret = rxrpc_send_data(rxrpc_sk(sock->sk), call, msg,
			      msg_data_left(msg), notify_end_tx);
	if (ret < 0)
		break;
	...
}

and rxrpc_do_sendmsg() jumps to error_put, which only drops the call ref.
The removed test returns -EPROTO without unlocking, so at that intermediate
commit a sendmsg() issued after the final packet had been queued would leave
call->user_mutex held and any later sendmsg/recvmsg/abort/close on that call
would block.

Both commits carry Fixes: and cc: stable.  Would it be worth saying in the
changelog that this hunk is also a lock-leak fix, so the earlier commit is
not backported on its own?

>  	timeo = sock_sndtimeo(sk, msg->msg_flags & MSG_DONTWAIT);
>  
>  	ret = rxrpc_wait_to_be_connected(call, &timeo);
> @@ -352,13 +345,20 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  	sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
>  
>  reload:
> +	if (unlikely(test_bit(RXRPC_CALL_TX_NO_MORE, &call->flags))) {
> +		trace_rxrpc_abort(call->debug_id, rxrpc_sendmsg_late_send,
> +				  call->cid, call->call_id, call->rx_consumed,
> +				  0, -EPROTO);
> +		ret = -EPROTO;
> +		goto out_unlock;
> +	}
> +
>  	ret = -EPIPE;
>  	if (sk->sk_shutdown & SEND_SHUTDOWN)
> -		goto maybe_error;
> +		goto out_unlock;
>  	state = rxrpc_call_state(call);
> -	ret = -ESHUTDOWN;
>  	if (state >= RXRPC_CALL_COMPLETE)
> -		goto maybe_error;
> +		goto call_terminated;

[Severity: Medium]
Can these three exits throw away a non-zero copied?

All of them now bypass maybe_error and return -EPROTO, -EPIPE or -ESHUTDOWN
from out_unlock.  The reload label is also reached after the mutex has been
dropped and the thread has slept:

wait_for_space:
	ret = -EAGAIN;
	if (msg->msg_flags & MSG_DONTWAIT)
		goto maybe_error;
	mutex_unlock(&call->user_mutex);

	ret = rxrpc_wait_for_tx_window(rx, call, &timeo,
				       msg->msg_flags & MSG_WAITALL);
	...
	goto reload;

By that point copied can already be non-zero: the first loop iteration filled
a txbuf, rxrpc_queue_packet() published it and cleared call->tx_pending, and
the following iteration failed rxrpc_check_tx_space().  While user_mutex is
dropped, a second sendmsg() on the same user_call_ID can queue the final
packet and set RXRPC_CALL_TX_NO_MORE, shutdown() can set SEND_SHUTDOWN, or
the I/O thread can complete the call.  sendmsg() then returns a hard error
although this invocation has already consumed bytes from msg->msg_iter and
queued full DATA packets that will still be transmitted, so the application
cannot tell how much of its buffer was accepted.

Related to the same window, is the new maybe_error branch reachable at all?

maybe_error:
	if (copied) {
		if (test_bit(RXRPC_CALL_TX_NO_MORE, &call->flags)) {
			/* If we've get here, we must have slept waiting for space and .
			 */
			ret = copied;
			goto out_unlock;
		}

The bit is either set by this thread queueing the last packet, in which case
the do/while loop exits to "ret = copied", or set by another thread while the
mutex was dropped, which the new test at reload catches first.  The comment
also reads as an unfinished sentence ("waiting for space and .").

>  	ret = -EPROTO;
>  	if (state != RXRPC_CALL_CLIENT_PRE_SEND &&
>  	    state != RXRPC_CALL_CLIENT_SEND_REQUEST &&

[ ... ]

> @@ -448,31 +448,73 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,

[ ... ]

> -success:
> +	/* Don't check for call completeness here, but leave that to recvmsg or
> +	 * a further call to sendmsg().
> +	 */
>  	ret = copied;
> -	if (rxrpc_call_is_complete(call) &&
> -	    call->error < 0)
> -		ret = call->error;
>  out_unlock:
>  	mutex_unlock(&call->user_mutex);
> +out:
> +
> +	/* The return value is a bit complicated as we want to avoid returning
> +	 * an error if we have queued the final packet.  In descending order of
> +	 * preference:
> +	 *
> +	 * (1) If we queue the last packet: the amount copied (which may be
> +	 *     zero).  recvmsg() should be used to collect the result.
> +	 *
> +	 * (2) If another sendmsg() has already queued the last packet: -EPROTO.
> +	 *
> +	 * (3) If the send side of the socket is shut down, -EPIPE.
> +	 *
> +	 * (4) If the call is in the wrong state to transmit: -EPROTO.
> +	 *
> +	 * (5) If the call has terminated early, likely due to an external
> +	 *     event such as being remotely aborted: -ESHUTDOWN.
> +	 *
> +	 * (6) If some data has been copied by this call: the amount copied
> +	 *     (which will be greater than zero).
> +	 *
> +	 * (7) Any other error.

[Severity: Low]
Does the code implement item (6) taking precedence over item (7)?  The
->secure_packet() failure path inside the loop does:

			ret = call->security->secure_packet(call, txb);
			if (ret < 0)
				goto out_unlock;

so an -ENOMEM there is returned even though copied is non-zero (the bytes
were copied into txb just above and the txbuf stays as call->tx_pending),
while every other in-loop error exit goes through maybe_error.  The
retryable case is rerouted to maybe_error_rewind by the following patch,
"rxrpc: Fix packet encryption error handling", which also rewrites this
list, but as written here the comment describes behaviour the function does
not have.

> +	 *
> +	 * For (2)-(5), there's no point in continuing with the sendmsg().  The
> +	 * app should abort the call (just in case the error came from
> +	 * somewhere else) and then use recvmsg() to collect the final result
> +	 * of the call.
> +	 */
>  	_leave(" = %d", ret);
>  	return ret;
>  
>  call_terminated:
> -	ret = call->error;
> +	ret = -ESHUTDOWN;
>  	goto out_unlock;
>  
>  maybe_error:
> -	if (copied)
> -		goto success;
> +	if (copied) {
> +		if (test_bit(RXRPC_CALL_TX_NO_MORE, &call->flags)) {
> +			/* If we've get here, we must have slept waiting for space and .
> +			 */
> +			ret = copied;
> +			goto out_unlock;
> +		}
> +		if (rxrpc_call_is_complete(call))
> +			goto call_terminated;
> +		ret = copied;
> +	}
>  	goto out_unlock;
>  
>  efault:
>  	ret = -EFAULT;
> -	goto out_unlock;
> +	goto maybe_error;

[Severity: High]
Is -ESHUTDOWN really the only error afs_make_call can now see for
a call that terminated early?

The new return-value contract documented in rxrpc_send_data() says
that a call which has terminated early (e.g. remotely aborted) gets
-ESHUTDOWN, and this hunk narrows the afs recovery path from
-ECONNABORTED to exactly that value.  But the two sleeping paths in
rxrpc_send_data() still pass call->error straight back to the caller:

	ret = rxrpc_wait_to_be_connected(call, &timeo);
	if (ret < 0)
		goto out_unlock;

and

	ret = rxrpc_wait_for_tx_window(rx, call, &timeo,
				       msg->msg_flags & MSG_WAITALL);
	if (ret < 0)
		goto out_nolock;

where rxrpc_wait_for_tx_window_intr() and its non-interruptible
counterpart do:

	if (rxrpc_call_is_complete(call))
		return call->error;

So if the peer aborts the call (or the connection errors out) while
this thread is blocked waiting to be connected or waiting for Tx
window space, sendmsg() returns -ECONNABORTED (or -ENETUNREACH, etc.)
rather than -ESHUTDOWN; out_nolock leaves ret alone when copied is
zero, so nothing remaps it.  rxrpc_kernel_send_data() then propagates
that to afs_make_call(), which no longer matches on it: the
rxrpc_kernel_recv_data() call that collects call->abort_code is
skipped and call->responded stays false, so afs records a plain local
error and loses the remote abort reason it used to pick up.  That is
exactly the case the previous -ECONNABORTED test existed to handle.

Which end should be adjusted?  Making the wait helpers (or the
out_unlock/out_nolock paths that consume their result) return
-ESHUTDOWN when rxrpc_call_is_complete() would keep the documented
contract in one place; alternatively afs_make_call() has to keep
accepting -ECONNABORTED and friends.  Either way the two halves of
this patch currently disagree about what sendmsg() reports for a call
that died while we were asleep.

[Severity: Low]
Should the changelog also mention the partial-write behaviour changes that
are unrelated to call completion?

With efault now going to maybe_error, a copy_from_iter_full() fault on a
later segment returns the positive byte count instead of -EFAULT.  The
SEND_SHUTDOWN and wrong-transmit-state tests move the other way: they used
to "goto maybe_error" and hence return the byte count when copied was
non-zero, and now return -EPIPE / -EPROTO directly.

The changelog only discusses completed calls, the final packet and
-ESHUTDOWN.

[ ... ]

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