Re: [PATCH net v10 13/14] afs: Fix uncleared op->call pointer

[netdev-bot+sashiko]

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

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

- [Medium] The Fixes: tag is too narrow.

Pre-existing issues:
- [High] The asynchronous fileserver-rotation peer of the loop fixed here
  does not handle the 'issue helper returned without creating a call'…
- [High] An RPC that was never issued because of an allocation failure is
  reported to userspace as a successful operation.

--

afs: Fix uncleared op->call pointer

This adds op->call = NULL after the afs_put_call() in
afs_wait_for_operation(), so that a subsequent fileserver rotation
iteration cannot observe a pointer to a call that has already been put.

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

A couple of questions about the Fixes: tag and the changelog wording, and
two observations about neighbouring paths that the new op->call = NULL
interacts with.

> Fixes: 6f2ff7e89bd0 ("afs: Don't put afs_call in afs_wait_for_call_to_complete()")

[Severity: Medium]
Is this tag scoped correctly?  6f2ff7e89bd0 moved the afs_put_call() out of
afs_wait_for_call_to_complete() and into afs_wait_for_operation(), but the
stale op->call looks like it predates that move.

At 6f2ff7e89bd0^ the loop in afs_wait_for_operation() reads:

	if (op->call)
		op->error = afs_wait_for_call_to_complete(op->call, &op->ac);

and the tail of afs_wait_for_call_to_complete() that the blamed commit
removed was:

out:
	_debug("call complete");
	afs_put_call(call);

so the call was already freed with op->call left pointing at it, and
git grep 'op->call =' at that revision finds only the assignment in
afs_make_op_call().  Since the tag names a v6.7-era commit, would the
older stable trees be skipped by the backport tooling while carrying the
same stale pointer?

> Fix uncleared op->call pointer in afs_wait_for_operation() lest server
> rotation occur and try to reuse the call.

Could the changelog say what the reuse actually is?  On a later rotation
iteration where the issue helper bails out through afs_op_nomem() without
assigning op->call, the stale pointer is handed to
afs_wait_for_call_to_complete() and afs_put_call() a second time, i.e. a
use-after-free of a freed afs_call rather than just a "reuse".

> diff --git a/fs/afs/fs_operation.c b/fs/afs/fs_operation.c
> index 20801b29521d1..94fa65548d712 100644
> --- a/fs/afs/fs_operation.c
> +++ b/fs/afs/fs_operation.c
> @@ -297,6 +297,7 @@ void afs_wait_for_operation(struct afs_operation *op)
>  			op->call_error = op->call->error;
>  			op->call_responded = op->call->responded;
>  			afs_put_call(op->call);
> +			op->call = NULL;
>  		}
>  	}

[Severity: High]
This is a pre-existing issue and not introduced by the patch, but with
op->call now guaranteed to be NULL at the top of every iteration, what
happens to an -ENOMEM from the issue helper?

Each iteration of the loop starts by clearing the per-call state:

		op->call_responded = false;
		op->call_error = 0;
		op->call_abort_code = 0;

and the issue helpers bail out on allocation failure with:

	call = afs_alloc_flat_call(op->net, &afs_RXFSFetchData, ...);
	if (!call)
		return afs_op_nomem(op);

while afs_op_nomem() in fs/afs/internal.h only records:

static inline void afs_op_nomem(struct afs_operation *op)
{
	op->cumul_error.error = -ENOMEM;
}

With op->call NULL the completion block above is skipped, so op->call_error
stays 0 and AFS_OPERATION_STOP is not set.  afs_select_fileserver() then
evaluates the previous "result":

	int error = op->call_error, ...
	switch (op->call_error) {
	case 0:
		...
		error = afs_update_volume_state(op);
		...
		fallthrough;
	default:
		/* Success or local failure.  Stop. */
		afs_op_set_error(op, error);

and afs_op_set_error() assigns unconditionally:

static inline int afs_op_set_error(struct afs_operation *op, int error)
{
	return op->cumul_error.error = error;
}

Does that erase the recorded -ENOMEM, so afs_wait_for_operation() takes the
!afs_op_error(op) branch and runs op->ops->success(op) on an afs_status_cb
that was never filled in?  For a store that reaches
afs_store_data_success():

	if (!afs_op_error(op)) {
		afs_pages_written_back(vnode, op->store.pos, op->store.size);

which would mark the pages written back and return success from write(2)
although no FS.StoreData was ever sent.  For create/mkdir, would
->success() and ->edit_dir() instantiate an inode and a directory entry
from a zeroed status and fid?

[Severity: High]
This isn't a bug introduced by this patch either, but the asynchronous peer
of this loop has no equivalent of the "no call was created" handling that
the if (op->call) test provides here.

In afs_issue_read() the async branch is:

		if (!afs_select_fileserver(op)) {
			afs_end_read(op);
			return;
		}

		afs_issue_read_call(op);

and afs_issue_read_call() returns void without inspecting op->call or
op->cumul_error:

static void afs_issue_read_call(struct afs_operation *op)
{
	op->call_responded = false;
	op->call_error = 0;
	op->call_abort_code = 0;
	if (test_bit(AFS_SERVER_FL_IS_YFS, &op->server->flags))
		yfs_fs_fetch_data(op);
	else
		afs_fs_fetch_data(op);
}

If afs_alloc_flat_call() fails in afs_fs_fetch_data()/yfs_fs_fetch_data(),
afs_make_op_call() is never reached, so op->call stays NULL and no rxrpc
call exists.  All the async completion work hangs off an existing call via
afs_fetch_data_async_rx() -> afs_read_receive() -> afs_end_read() ->
afs_fetch_data_notify() -> netfs_read_subreq_terminated().

Does that leave the netfs_io_subrequest unterminated for readahead and iocb
reads, with afs_put_operation() never called and the operation's
key/volume/server_list references leaked?

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