Re: [PATCH 1/2] NFSv4: Ensure test_and_free_stateid callers use private memory

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2025-04-23 at 13:59 -0400, Benjamin Coddington wrote:
> A follow-up patch intends to signal the success or failure of FREE_STATEID
> by modifying the nfs4_stateid's type which requires the const qualifier for
> the nfs4_stateid to be dropped.  Since it will no longer safe to operate
> directly on shared stateid objects in this path, ensure that callers send a
> copy.
>
> Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
> ---
>  fs/nfs/nfs4proc.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 6e95db6c17e9..bfb9e980d662 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -2990,6 +2990,7 @@ static void nfs41_delegation_recover_stateid(struct nfs4_state *state)
>  static int nfs41_check_expired_locks(struct nfs4_state *state)
>  {
>  	int status, ret = NFS_OK;
> +	nfs4_stateid stateid;
>  	struct nfs4_lock_state *lsp, *prev = NULL;
>  	struct nfs_server *server = NFS_SERVER(state->inode);
>  
> @@ -3007,9 +3008,9 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
>  			nfs4_put_lock_state(prev);
>  			prev = lsp;
>  
> +			nfs4_stateid_copy(&stateid, &lsp->ls_stateid);
>  			status = nfs41_test_and_free_expired_stateid(server,
> -					&lsp->ls_stateid,
> -					cred);
> +					&stateid, cred);
>  			trace_nfs4_test_lock_stateid(state, lsp, status);
>  			if (status == -NFS4ERR_EXPIRED ||
>  			    status == -NFS4ERR_BAD_STATEID) {
> @@ -3042,17 +3043,18 @@ static int nfs41_check_expired_locks(struct nfs4_state *state)
>  static int nfs41_check_open_stateid(struct nfs4_state *state)
>  {
>  	struct nfs_server *server = NFS_SERVER(state->inode);
> -	nfs4_stateid *stateid = &state->open_stateid;
> +	nfs4_stateid stateid;
>  	const struct cred *cred = state->owner->so_cred;
>  	int status;
>  
>  	if (test_bit(NFS_OPEN_STATE, &state->flags) == 0)
>  		return -NFS4ERR_BAD_STATEID;
> -	status = nfs41_test_and_free_expired_stateid(server, stateid, cred);
> +	nfs4_stateid_copy(&stateid, &state->open_stateid);
> +	status = nfs41_test_and_free_expired_stateid(server, &stateid, cred);
>  	trace_nfs4_test_open_stateid(state, NULL, status);
>  	if (status == -NFS4ERR_EXPIRED || status == -NFS4ERR_BAD_STATEID) {
>  		nfs_state_clear_open_state_flags(state);
> -		stateid->type = NFS4_INVALID_STATEID_TYPE;
> +		state->open_stateid.type = NFS4_INVALID_STATEID_TYPE;
>  		return status;
>  	}
>  	if (nfs_open_stateid_recover_openmode(state))

I don't know that you really need to do this. In the cases where you
end up setting the type to FREED, you will also return NFS4ERR_EXPIRED,
which will make the callers set the type to INVALID.

There will be a brief window where the type will be set to FREED, but
that should be no big deal.
-- 
Jeff Layton <jlayton@kernel.org>