Re: [PATCH net v10 01/14] rxrpc: Fix lack of short-send handling in rxrpc_kernel_send_data()

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The new retry loop in rxrpc_kernel_send_data()
  (net/rxrpc/sendmsg.c:817-838) re-acquires call->user_mutex…

--

rxrpc: Fix lack of short-send handling in rxrpc_kernel_send_data()

rxrpc_kernel_send_data() now loops internally when rxrpc_send_data()
reports a short send, the len argument is dropped in favour of the
iterator count, and the function returns 0 rather than the number of
bytes copied.  The AFS and rxperf callers are updated accordingly.

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

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> index ed2c9a51005ad..34aae8e789a43 100644
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c

[ ... ]

> @@ -805,8 +804,7 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
>   * Return: %0 if successful and a negative error code otherwise.
>   */
>  int rxrpc_kernel_send_data(struct socket *sock, struct rxrpc_call *call,
> -			   struct msghdr *msg, size_t len,
> -			   rxrpc_notify_end_tx_t notify_end_tx)
> +			   struct msghdr *msg, rxrpc_notify_end_tx_t notify_end_tx)
>  {
>  	bool dropped_lock = false;
       ^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Should dropped_lock be reset inside the new loop rather than being
initialised once here?

rxrpc_send_data() only ever writes to this flag in its wait_for_space
path, and never initialises it on entry:

net/rxrpc/sendmsg.c:rxrpc_send_data() {
	...
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;
	...
	*_dropped_lock = false;
	goto reload;
}

and maybe_error can return a positive short count with the flag still
set:

maybe_error:
	if (copied)
		goto success;
	goto out;
...
success:
	ret = copied;

>  	int ret;
> @@ -816,15 +814,29 @@ int rxrpc_kernel_send_data(struct socket *sock, struct rxrpc_call *call,
>  	ASSERTCMP(msg->msg_name, ==, NULL);
>  	ASSERTCMP(msg->msg_control, ==, NULL);
>  
> -	mutex_lock(&call->user_mutex);
> +	for (;;) {
> +		mutex_lock(&call->user_mutex);
>  
> -	ret = rxrpc_send_data(rxrpc_sk(sock->sk), call, msg, len,
> -			      notify_end_tx, &dropped_lock);
> -	if (ret == -ESHUTDOWN)
> -		ret = call->error;
> +		ret = rxrpc_send_data(rxrpc_sk(sock->sk), call, msg,
> +				      msg_data_left(msg),
> +				      notify_end_tx, &dropped_lock);
> +		if (ret == -ESHUTDOWN)
> +			ret = call->error;
> +
> +		if (!dropped_lock)
> +			mutex_unlock(&call->user_mutex);
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Can call->user_mutex be returned still held here?  The mutex_lock() at
the top of the loop is unconditional on every iteration, but the unlock
is gated on a flag that is only cleared by rxrpc_send_data() when it
re-acquires the lock after waiting.  So the sequence looks like:

  iteration 1: rxrpc_send_data() takes wait_for_space, unlocks,
               dropped_lock = true, the relock fails, maybe_error sees
               copied > 0 and returns a positive count
  loop:        msg_data_left(msg) != 0 and ret > 0, so we go round again
  iteration 2: mutex_lock() succeeds, this send never reaches
               wait_for_space, dropped_lock is still true, so
               mutex_unlock() is skipped and we break out of the loop

The kernel callers then take the same mutex again on the following
statement, e.g. rxrpc_kernel_abort_call() does mutex_lock(&call->user_mutex)
on the AFS and rxperf error paths, and afs_make_call() issues a second
rxrpc_kernel_send_data() for call->write_iter, so the task would block on
a mutex it already owns.

Reachability of the seeding iteration looks narrow: for the
RXRPC_UNINTERRUPTIBLE and RXRPC_PREINTERRUPTIBLE calls used by the AFS
client sends, the non-interruptible waiter only returns negative when the
call is complete with call->error < 0, and success: then overwrites ret
with that negative error.  It appears to need an RXRPC_INTERRUPTIBLE call
whose task has a pending signal, which I could not demonstrate for the
in-tree kernel senders running on workqueue workers.

For what it's worth, the later patch "rxrpc: Fix update of call->tx_pending
without holding lock" removes the dropped_lock out-parameter entirely and
moves the unlock inside rxrpc_send_data() with an __releases() annotation,
so the flag no longer exists at the end of the series.  Would it be worth
reordering so that this patch does not introduce the intermediate state,
or clearing dropped_lock at the top of each iteration here?

> +		if (ret < 0)
> +			break;
> +		if (msg_data_left(msg) == 0) {
> +			ret = 0;
> +			break;
> +		}
> +		if (ret == 0) {
> +			ret = -EIO;
> +			break;
> +		}
> +	}
>  
> -	if (!dropped_lock)
> -		mutex_unlock(&call->user_mutex);
>  	_leave(" = %d", ret);
>  	return ret;
>  }

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