Re: [PATCH net v11 05/17] 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 1 potential
issue(s) to consider.

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

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).

- [High] When sending a reply fails on an incoming cache-manager (CB)
  service call, the afs_call is never completed.

--

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

This patch makes afs_send_empty_reply() and afs_send_simple_reply() abort
the rxrpc call on any send error, not only on -ENOMEM. It also makes
afs_make_call() abort on any send error. The afs_abort_oom trace reason is
replaced with 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..c38458157eccf 100644
> --- a/fs/afs/rxrpc.c
> +++ b/fs/afs/rxrpc.c

[ ... ]

> @@ -869,22 +868,12 @@ 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);
>  }

[Severity: High]
Is the afs_call still abandoned here when the send fails? The rxrpc call
is aborted, but nothing calls afs_set_call_complete(), so the afs_call
stays in AFS_CALL_SV_REPLYING. afs_send_simple_reply() has the same
pattern.

The commit message says the change stops "otherwise abandoning it". That
is true for the rxrpc call, but it does not seem to hold for the afs_call.

The only way out of SV_REPLYING is afs_notify_end_reply_tx():

    afs_set_call_state(call, AFS_CALL_SV_REPLYING, AFS_CALL_SV_AWAIT_ACK);

That callback only runs once the final DATA packet has been queued, and
that does not happen on a send error.

A later notification (from this abort or from a received abort) goes
through afs_wake_up_async_call() -> afs_process_async_call() ->
afs_deliver_to_call(). The loop there does not handle SV_REPLYING:

    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
           ) {

None of the CB call types has a ->done hook. So the drop_ref reference
set by afs_charge_preallocation() is never released, because only
afs_set_call_complete() drops it.

If that's right, the afs_call leaks along with its rxrpc_call, peer and
server references and its buffers. net->nr_outstanding_calls would then
never reach zero, and afs_close_socket() would block forever here during
netns teardown or module unload:

    wait_var_event(&net->nr_outstanding_calls,
                   !atomic_read(&net->nr_outstanding_calls));

A remote peer seems able to trigger this. It can send CB.Probe or
CB.CallBack and abort right after the last request packet. Then
SRXAFSCB_Probe() -> afs_send_empty_reply() -> rxrpc_kernel_send_data()
returns -ESHUTDOWN, and rxrpc_kernel_abort_call() does nothing.
-ENOMEM from txbuf or txqueue allocation leads to the same state.

afs_abort_service_call() in fs/afs/cmservice.c already pairs the two
steps:

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

Should the send-error paths in afs_send_empty_reply() and
afs_send_simple_reply() also call afs_set_call_complete(call, ret, 0)
after the abort? No later patch in the series changes these two
functions.

[ ... ]

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