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

[Benjamin Coddington <bcodding@redhat.com>]

On 23 Apr 2025, at 16:35, Jeff Layton wrote:

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

Definitely playing it safe here, I think I have to worry about concurrency -
who else might be simultaneously looking at that nfs4_state->open_stateid.type?
I really don't want to introduce a regression because we knew we weren't
modifying that stateid before.

I suppose I can dive into an audit and see if that can actually happen.

Thanks for looking at this,

Ben