Re: [PATCH net v3 07/14] afs: Simplify call refcounting

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://sashiko.dev/#/patchset/20260713081022.2186481-8-dhowells@xxxxxxxxxx?part=7
---
--- Patch [7]: [PATCH net v3 07/14] afs: Simplify call refcounting ---
commit 467b175328043584d311b0ca542f46ebdef122fe
Author: David Howells <dhowells@xxxxxxxxxx>

afs: Simplify call refcounting

This commit simplifies afs_call refcounting by removing the ref held by queued
async work items and dropping the retaining ref upon async call completion
instead. It also dispatches cache manager server RPC handlers directly from
afs_deliver_to_call() and changes call->work to a function pointer.

> diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
> index 06c711c75f556..a404b6f0cc7c2 100644
> --- a/fs/afs/rxrpc.c
> +++ b/fs/afs/rxrpc.c
[ ... ]
> @@ -479,8 +445,7 @@ void afs_make_call(struct afs_call *call, gfp_t gfp)
>  	if (call->rxcall)
>  		rxrpc_kernel_shutdown_call(call->net->socket, call->rxcall);
>  	if (call->async) {
> -		if (cancel_work_sync(&call->async_work))
> -			afs_put_call(call);
> +		cancel_work_sync(&call->async_work);
>  		afs_set_call_complete(call, ret, 0);
>  	}

[Severity: Medium]
Can this leak the struct afs_call object?

When rxrpc_kernel_begin_call() fails, the code jumps to error_kill_call and
cancels the async work here. Since afs_set_call_complete() no longer drops
call->drop_ref, and the async work function is cancelled (so it never runs to
drop the retaining ref), it seems the retaining ref acquired via afs_get_call()
earlier in afs_make_call() is never released on this error path.

[ ... ]
> @@ -704,19 +671,16 @@ static void afs_wake_up_async_call(struct sock *sk, struct rxrpc_call *rxcall,
>  				   unsigned long call_user_ID)
>  {
>  	struct afs_call *call = (struct afs_call *)call_user_ID;
> -	int r;
>  
>  	trace_afs_notify_call(rxcall, call);
>  	call->need_attention = true;
>  
> -	if (__refcount_inc_not_zero(&call->ref, &r)) {
> -		trace_afs_call(call->debug_id, afs_call_trace_wake, r + 1,
> -			       atomic_read(&call->net->nr_outstanding_calls),
> -			       __builtin_return_address(0));
> +	trace_afs_call(call->debug_id, afs_call_trace_wake,
> +		       refcount_read(&call->ref),
> +		       atomic_read(&call->net->nr_outstanding_calls),
> +		       __builtin_return_address(0));
>  
> -		if (!queue_work(afs_async_calls, &call->async_work))
> -			afs_deferred_put_call(call);
> -	}
> +	queue_work(afs_async_calls, &call->async_work);
>  }

[Severity: Critical]
Can this result in a use-after-free and workqueue corruption if an RxRPC
notification arrives right as the call completes?

afs_process_async_call() drops the final retaining reference and synchronously
frees the afs_call object when call->state == AFS_CALL_COMPLETE. If
afs_wake_up_async_call() is executed concurrently by the RxRPC thread before
rxrpc_kernel_shutdown_call() unhooks the callback, it will now blindly modify
call->need_attention and call queue_work() on a freed &call->async_work without
first acquiring a reference.

> diff --git a/fs/afs/file.c b/fs/afs/file.c
> index 0467742bfeee3..35d68f7f498df 100644
> --- a/fs/afs/file.c
> +++ b/fs/afs/file.c
[ ... ]
> @@ -316,15 +316,17 @@ void afs_fetch_data_async_rx(struct work_struct *work)
>  	struct afs_call *call = container_of(work, struct afs_call, async_work);
>  
>  	afs_read_receive(call);
> -	afs_put_call(call);
> +
> +	if (call->state == AFS_CALL_COMPLETE) {
> +		cancel_work(&call->async_work);
> +		afs_put_call(call);
> +	}
>  }
>  
>  void afs_fetch_data_immediate_cancel(struct afs_call *call)
>  {
>  	if (call->async) {
> -		afs_get_call(call, afs_call_trace_wake);
> -		if (!queue_work(afs_async_calls, &call->async_work))
> -			afs_deferred_put_call(call);
> +		queue_work(afs_async_calls, &call->async_work);
>  		flush_work(&call->async_work);
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but does flushing the async work here cause a
NULL pointer dereference?

If afs_make_call() fails early (e.g. rxrpc_kernel_begin_call() fails),
call->rxcall is NULL. The error path calls call->type->immediate_cancel(call),
which resolves to afs_fetch_data_immediate_cancel(). This queues and
synchronously flushes the work.

afs_read_receive() is then executed while call->state is still less than
AFS_CALL_COMPLETE. This calls rxrpc_kernel_check_life() and passes it
call->rxcall:

fs/afs/file.c:afs_read_receive() {
	...
	if (state < AFS_CALL_COMPLETE) {
		netfs_read_subreq_progress(op->fetch.subreq);
		if (rxrpc_kernel_check_life(call->net->socket, call->rxcall))
			return;
	...
}

Because call->rxcall is NULL, wouldn't rxrpc_kernel_check_life()
unconditionally dereference it when calling rxrpc_call_is_complete(), causing
an immediate kernel panic?