Re: [PATCH net v12 05/15] afs: Fix afs to abort the rxrpc call on send error

[netdev-bot+sashiko@kernel.org]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message leaves out the patch's most important
  change.
- [Low] On send failure, afs_send_empty_reply() and
  afs_send_simple_reply() now call afs_set_call_complete(call, ret,…

Pre-existing issues:
- [High] In afs_make_call(), async client calls that use the default
  afs_process_async_call() handler are never completed when the send…

--

afs: Fix afs to abort the rxrpc call on send error

With this patch, afs_send_empty_reply() and afs_send_simple_reply() abort
the rxrpc call on any send error, not only on -ENOMEM, and also mark the
afs_call complete. afs_make_call() now aborts unconditionally on send
error. The afs_abort_oom trace reason is replaced by afs_abort_send_error.

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

> diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
> index bcb8bb15d471d..64dd32df8a34e 100644
> --- a/fs/afs/rxrpc.c
> +++ b/fs/afs/rxrpc.c
> @@ -441,10 +441,8 @@ void afs_make_call(struct afs_call *call, gfp_t gfp)
>  	return;
>  
>  error_do_abort:
> -	if (ret != -ECONNABORTED)
> -		rxrpc_kernel_abort_call(call->net->socket, rxcall,
> -					RX_USER_ABORT, ret,
> -					afs_abort_send_data_error);
> +	rxrpc_kernel_abort_call(call->net->socket, rxcall,
> +				RX_USER_ABORT, ret, afs_abort_send_data_error);
>  	if (call->async) {
>  		afs_see_call(call, afs_call_trace_async_abort);
>  		return;

[Severity: High]
This isn't a bug introduced by this patch, but the async branch here
returns without completing the call. Can this leak the afs_call for async
client calls that use afs_process_async_call()? Examples are
afs_fs_get_capabilities() and the VL.GetCapabilities call in vlclient.c.

Suppose rxrpc_kernel_send_data() fails before the last packet is queued,
for example with -ENOMEM from alloc_txbuf() in rxrpc_send_data(). Then
afs_notify_end_request_tx() never runs, call->state stays at
AFS_CALL_CL_REQUESTING, and call->error is left at 0.

The abort notification then queues afs_process_async_call(). However,
afs_deliver_to_call() has no case for that state:

	while (state = READ_ONCE(call->state),
	       state == AFS_CALL_CL_AWAIT_REPLY ||
	       state == AFS_CALL_SV_AWAIT_OP_ID ||
	       state == AFS_CALL_SV_AWAIT_REQUEST ||
	       state == AFS_CALL_SV_AWAIT_ACK
	       ) {

So it jumps straight to done: and calls type->done(), which is
afs_fileserver_probe_result() or afs_vlserver_probe_result(), with
call->error == 0. The failed probe is then handled by the success branch.

Nothing moves the call to AFS_CALL_COMPLETE, so the drop_ref reference
taken earlier in afs_make_call() is never released:

	if (call->async) {
		afs_get_call(call, afs_call_trace_get);
		call->drop_ref = true;
	}

Wouldn't that leak the afs_call along with its rxrpc call, peer, server,
estate and key references? It would also keep net->nr_outstanding_calls
non-zero, so afs_close_socket() would wait forever in wait_var_event().

This patch adds afs_set_call_complete() for the same pattern in
afs_send_empty_reply() and afs_send_simple_reply(). Would something
similar be needed here?

Async FetchData does not look affected, because afs_read_receive() checks
rxrpc_kernel_check_life() and completes the call itself. The early return
came from commit 9750be93b2be ("afs: Fix cleanup of immediately failed
async calls").

[ ... ]

> @@ -869,21 +868,13 @@ void afs_send_empty_reply(struct afs_call *call)
>  	msg.msg_controllen	= 0;
>  	msg.msg_flags		= 0;
>  
> -	switch (rxrpc_kernel_send_data(net->socket, call->rxcall, &msg,
> -				       afs_notify_end_reply_tx)) {
> -	case 0:
> -		_leave(" [replied]");
> -		return;
> -
> -	case -ENOMEM:
> -		_debug("oom");
> +	ret = rxrpc_kernel_send_data(net->socket, call->rxcall, &msg,
> +				     afs_notify_end_reply_tx);
> +	if (ret < 0) {
>  		rxrpc_kernel_abort_call(net->socket, call->rxcall,
> -					RXGEN_SS_MARSHAL, -ENOMEM,
> -					afs_abort_oom);
> -		fallthrough;
> -	default:
> -		_leave(" [error]");
> -		return;
> +					RXGEN_SS_MARSHAL, ret,
> +					afs_abort_send_error);
> +		afs_set_call_complete(call, ret, RXGEN_SS_MARSHAL);

[Severity: Medium]
The commit message says:

  Fix afs_send_empty_reply() and afs_send_simple_reply() to always try to
  abort the rxrpc call rather than just aborting on -ENOMEM and otherwise
  abandoning it.

It doesn't mention the new afs_set_call_complete() calls here and in
afs_send_simple_reply(). Should the commit message describe them?

afs_charge_preallocation() sets call->drop_ref for incoming cache manager
calls, so this call releases the call's self reference.

Before the patch, a failed reply left the call in AFS_CALL_SV_REPLYING.
afs_deliver_to_call() has no case for that state, so the drop_ref
reference was never put and net->nr_outstanding_calls never drained.

Completing the call is therefore what fixes the leak. A stable backport
could leave it out if the commit message only covers the abort.

>  	}
>  }
>  

[ ... ]

> @@ -913,12 +904,10 @@ void afs_send_simple_reply(struct afs_call *call, const void *buf, size_t len)
>  	ret = rxrpc_kernel_send_data(net->socket, call->rxcall, &msg,
>  				     afs_notify_end_reply_tx);
>  	if (ret < 0) {
> -		if (ret == -ENOMEM) {
> -			_debug("oom");
> -			rxrpc_kernel_abort_call(net->socket, call->rxcall,
> -						RXGEN_SS_MARSHAL, -ENOMEM,
> -						afs_abort_oom);
> -		}
> +		rxrpc_kernel_abort_call(net->socket, call->rxcall,
> +					RXGEN_SS_MARSHAL, ret,
> +					afs_abort_send_error);
> +		afs_set_call_complete(call, ret, RXGEN_SS_MARSHAL);

[Severity: Low]
Is RXGEN_SS_MARSHAL the right value for the third argument of
afs_set_call_complete()? That parameter is remote_abort. It is stored in
call->abort_code, which is documented as:

	u32			abort_code;	/* Remote abort ID or 0 */

Other places that complete a call after a local abort pass 0. One example
is afs_abort_service_call() in fs/afs/cmservice.c:

	rxrpc_kernel_abort_call(call->net->socket, call->rxcall,
				abort_code, error, why);
	afs_set_call_complete(call, error, 0);

The local_abort path in afs_deliver_to_call() and the -EINTR path in
afs_wait_for_call_to_complete() do the same.

The send may fail because the peer already aborted the call or because of
a network error, such as -ECONNABORTED. In that case the rxrpc call is
already complete and rxrpc_kernel_abort_call() does nothing. RXGEN_SS_MARSHAL
would still be recorded as the remote abort code, although no such abort
was sent.

The last packet may also have been queued already (AFS_CALL_SV_AWAIT_ACK).
If this path then takes state_lock before afs_deliver_to_call(), it would
also overwrite the real remote abort code.

For service calls, this value seems to appear only in the afs_call_state
and afs_call_done tracepoints. The same question applies to the new call
in afs_send_empty_reply().

>  		_leave(" [error]");
>  	}
>  }

[ ... ]

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