Re: [PATCH net v9 04/14] rxrpc: Fix sendmsg to not return an error if last packet queued

[netdev-bot+sashiko]

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

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

- [Medium] rxrpc_send_data() (net/rxrpc/sendmsg.c): the patch deletes the
  call-completion/error check from the *unconditional* `success:` path…

Pre-existing issues:
- [Low] Divergence between Documentation/networking/rxrpc.rst and the
  kernel-doc for rxrpc_kernel_send_data().
- [Low] At this patch, rxrpc_send_data()'s `out:` label writes
  `call->tx_pending = txb` (net/rxrpc/sendmsg.c:456-457) and is…

--

rxrpc: Fix sendmsg to not return an error if last packet queued

This moves the "call completed with an error" check in rxrpc_send_data()
out of the success path and into the partial-send path, so that a sendmsg()
that queued the last packet of a call reports the byte count and leaves the
completion for recvmsg() to report.  The kernel-doc for
rxrpc_kernel_send_data() is updated to document a byte-count return and the
possibility of short sends.

[ ... fs/afs/rxrpc.c hunk snipped, unrelated to the comments below ... ]

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> index ed2c9a51005ad..1d66e98081624 100644
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c
> @@ -453,9 +453,6 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  
>  success:
>  	ret = copied;
> -	if (rxrpc_call_is_complete(call) &&
> -	    call->error < 0)
> -		ret = call->error;
>  out:
>  	call->tx_pending = txb;
>  	_leave(" = %d", ret);

[Severity: Medium]

The commit message describes the change as covering the case where sendmsg
"has successfully queued the last packet of a call", but the check is being
removed from the unconditional success: label in rxrpc_send_data(), which is
reached on every normal exit from the copy loop.  Is the broader scope
intended?

Two cases reach success: without any last packet having been queued:

 - a MSG_MORE send whose data does not fill a txbuf, where the bytes are
   only copied into call->tx_pending and nothing is queued at all;

 - a MSG_MORE send that queues a non-final packet.

For those sends the only completion test left is the one inside the loop:

	/* check for the far side aborting the call or a network error
	 * occurring */
	if (rxrpc_call_is_complete(call))
		goto call_terminated;

If the rxrpc I/O thread completes the call with an abort or network error
after that test and before the loop exits, sendmsg() now returns a positive
byte count for data that will never be transmitted.

The specific error is then not recoverable from the next sendmsg() either,
since rxrpc_do_sendmsg() substitutes a generic code before reaching
rxrpc_send_data():

	if (rxrpc_call_is_complete(call)) {
		/* it's too late for this call */
		ret = -ESHUTDOWN;
		goto out_put_unlock;
	}

so a remote abort delivered as -ECONNABORTED is reported as -ESHUTDOWN, and
userspace has to go to recvmsg() to obtain the abort code.

Since this patch is tagged for stable, would it be worth narrowing the
change to the "last packet queued" case, or describing the MSG_MORE
behaviour change in the commit message?

> @@ -467,8 +464,14 @@ static int rxrpc_send_data(struct rxrpc_sock *rx,
>  	return call->error;
>  
>  maybe_error:
> -	if (copied)
> +	if (copied) {
> +		if (rxrpc_call_is_complete(call) &&
> +		    call->error < 0) {
> +			ret = call->error;
> +			goto out;
> +		}
>  		goto success;
> +	}
>  	goto out;
>  
>  efault:

Reinstating the check here does not cover the cases above: once the last
packet has been queued the loop exits straight to success:, so maybe_error
is only reachable for partial or failed sends.

[Severity: Low]

This is a pre-existing issue and not introduced by this patch, but the
rewritten maybe_error block is the entry point for paths that jump to out:,
where the mutex-protected call->tx_pending is stored:

out:
	call->tx_pending = txb;

Is that store always made with call->user_mutex held?  wait_for_space
drops the mutex and then jumps back to maybe_error on failure:

wait_for_space:
	ret = -EAGAIN;
	if (msg->msg_flags & MSG_DONTWAIT)
		goto maybe_error;
	mutex_unlock(&call->user_mutex);
	*_dropped_lock = true;
	ret = rxrpc_wait_for_tx_window(rx, call, &timeo,
				       msg->msg_flags & MSG_WAITALL);
	if (ret < 0)
		goto maybe_error;
	if (call->interruptibility == RXRPC_INTERRUPTIBLE) {
		if (mutex_lock_interruptible(&call->user_mutex) < 0) {
			ret = sock_intr_errno(timeo);
			goto maybe_error;
		}

With two concurrent sendmsg() calls on the same user_call_ID, can the
unlocked store overwrite a partially filled txbuf that the thread actually
holding the mutex has just stored, leaking that rxrpc_txbuf and dropping
the data already copied into it?

For the record, the later commit "rxrpc: Fix update of call->tx_pending
without holding lock" in the same series removes this by moving the store
to an out_txb: label immediately followed by out_unlock: with the
mutex_unlock(), dropping the _dropped_lock parameter and returning the
lock-dropped failure paths via out_nolock: without touching
call->tx_pending.

> @@ -800,9 +803,16 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
>   * Allow a kernel service to send data on a call.  The call must be in an state
>   * appropriate to sending data.  No control data should be supplied in @msg,
>   * nor should an address be supplied.  MSG_MORE should be flagged if there's
> - * more data to come, otherwise this data will end the transmission phase.
> + * more data to come, otherwise this data will end the transmission phase if
> + * all the data is buffered.
> + *
> + * Note that this function may return a short send, in which case it should be
> + * called again for the remainder of the data or to pick up an error that
> + * caused the short send.
>   *
> - * Return: %0 if successful and a negative error code otherwise.
> + * Return: The number of bytes buffered (could be %0 if @len is 0 or
> + * msg_iter holds 0 bytes) if successful and a negative error code
> + * otherwise.
>   */

[Severity: Low]

This is a pre-existing documentation issue rather than something introduced
here, but Documentation/networking/rxrpc.rst describes a different contract
for rxrpc_kernel_send_data() than this kernel-doc: the rst still says len is
"the total amount of data to transmit" and mentions neither short sends nor
a byte-count return.

That paragraph is rewritten later in the series by "rxrpc: Fix packet
encryption error handling", which changes it to "len is the amount of data
to add to the transmission" and adds the short-write wording, so only one
sentence in the rst is left out of step:

     msg must not specify a destination address, control data or any flags
     other than MSG_MORE.

afs_make_call() passes MSG_WAITALL | MSG_MORE to rxrpc_kernel_send_data(),
which predates this series.  Should the flag restriction in the rst be
updated as well?

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