Re: [PATCH net v11 10/17] rxrpc: Fix return in rxrpc_recvmsg_data() for service calls

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit is Cc'd to stable@vger.kernel.org but has no Fixes:
  tag.
- [Low] The patch adds a new normal return value, 2, to the exported
  kernel API rxrpc_kernel_recv_data(), but…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but rxperf can
  leak rxperf_call objects, and the rxrpc_call references they hold,…

--

rxrpc: Fix return in rxrpc_recvmsg_data() for service calls

This patch makes rxrpc_kernel_recv_data() return a new value, 2, when a
service call has received all of its request but has not finished yet. It
no longer returns 1 in that case, which made the call look complete. The
AFS and rxperf callers are updated to handle the new return value.

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

> cc: linux-afs@lists.infradead.org
> cc: stable@vger.kernel.org

[Severity: Low]
Should this have a Fixes: tag? It is Cc'd to stable, but nothing says
which trees need it.

The behaviour being fixed looks like it came from:

Fixes: 93368b6bd58a ("rxrpc: Move call state changes from recvmsg to I/O thread")

That commit dropped the state switch in rxrpc_kernel_recv_data(). The
switch had a default case that returned -EINPROGRESS for service calls
that were still processing, replying or waiting for the final ACK. In its
place, it added the RXRPC_CALL_RECVMSG_READ_ALL early return in
rxrpc_recvmsg_data(), which returns 1:

    if (test_bit(RXRPC_CALL_RECVMSG_READ_ALL, &call->flags)) {
        seq = call->ackr_window - 1;
        ret = 1;
        goto done;
    }

Since then, the ret == -EINPROGRESS checks in afs_deliver_to_call() and
rxperf_deliver_to_call() have been dead code.

Trees older than v6.2 still use the -EINPROGRESS contract. Without the
tag, stable maintainers could backport this to trees that don't need it.

> diff --git a/net/rxrpc/recvmsg.c b/net/rxrpc/recvmsg.c
> index efcba4b2e74f0..716f7f47d7e46 100644
> --- a/net/rxrpc/recvmsg.c
> +++ b/net/rxrpc/recvmsg.c
> @@ -637,9 +637,11 @@ int rxrpc_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>   * Note that we may return %-EAGAIN to drain empty packets at the end
>   * of the data, even if we've already copied over the requested data.
>   *
> - * Return: %0 if got what was asked for and there's more available, %1
> - * if we got what was asked for and we're at the end of the data and
> - * %-EAGAIN if we need more data.
> + * Return: %0 if got what was asked for and there's more available, %1 if we
> + * got what was asked for and we're at the end of the call, %2 if a service
> + * call received all of the request but is still in progress and %-EAGAIN if we
> + * need more data.  A variety of other errors can be returned if the call
> + * completed with failure.
>   */
>  int rxrpc_kernel_recv_data(struct socket *sock, struct rxrpc_call *call,
>  			   struct iov_iter *iter, size_t *_len,
> @@ -678,6 +680,11 @@ int rxrpc_kernel_recv_data(struct socket *sock, struct rxrpc_call *call,
>  
>  read_phase_complete:
>  	ret = 1;
> +	if (rxrpc_is_service_call(call)) {
> +		if (rxrpc_call_is_complete(call))
> +			goto call_failed;
> +		ret = 2;
> +	}

[Severity: Low]
Should Documentation/networking/rxrpc.rst be updated as well? Its section
on rxrpc_kernel_recv_data() still lists only three normal returns:

      There are three normal returns: 0 if the buffer was filled and want_more
      was true; 1 if the buffer was filled, the last DATA packet has been
      emptied and want_more was false; and -EAGAIN if the function needs to be
      called again.

With this change, a service call in that situation gets 2, or goes to
call_failed if the call has already completed. It no longer gets 1.

The kernel-doc above now lists %2, so the tree describes two different
return contracts for this exported function. A kernel service written from
rxrpc.rst would not know to handle 2.

>  out:
>  	if (_service)
>  		*_service = call->dest_srx.srx_service;
> diff --git a/net/rxrpc/rxperf.c b/net/rxrpc/rxperf.c
> index 5042e8bfca550..6ccfd40b5388c 100644
> --- a/net/rxrpc/rxperf.c
> +++ b/net/rxrpc/rxperf.c
> @@ -300,7 +300,7 @@ static void rxperf_deliver_to_call(struct work_struct *work)
>  						     &len, false, &remote_abort,
>  						     &call->service_id);
>  
> -			if (ret == -EINPROGRESS || ret == -EAGAIN)
> +			if (ret == -EAGAIN || ret == 2)
>  				return;

[Severity: Medium]
This is a pre-existing issue that this patch did not introduce, but can
rxperf leak rxperf_call objects, and the rxrpc_call refs they hold, when
the module is unloaded?

The only place an rxperf_call is freed is the call_complete path in
rxperf_deliver_to_call(). That path runs only when a notify_rx callback
queues the work item.

The teardown in rxperf_close_socket() looks like this:

    kernel_listen(rxperf_socket, 0);
    kernel_sock_shutdown(rxperf_socket, SHUT_RDWR);
    flush_workqueue(rxperf_workqueue);
    sock_release(rxperf_socket);

kernel_sock_shutdown() goes into rxrpc_shutdown(), which sets sk_state to
RXRPC_CLOSE. After that, sock_release()->rxrpc_release_calls_on_socket()
aborts any remaining calls with rxrpc_propose_abort(). The completion
notification is then dropped by this check in rxrpc_notify_socket():

    if (rx && sk->sk_state < RXRPC_CLOSE) {
        if (call->notify_rx) {

rxperf does not keep a list of outstanding calls. So a call still in
flight at unload, such as a partly received request or a call in
SV_AWAIT_ACK, never gets its rxperf_call freed or its rxcall ref dropped.

This patch returns early on ret == 2 in SV_AWAIT_ACK. Such calls now
reliably stay alive until the final ACK or completion arrives, so the
window for this is somewhat longer. The teardown gap itself is older than
this patch.

[ ... ]

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