> On Jul 18, 2023, at 9:51 AM, Trond Myklebust <trondmy@xxxxxxxxxx> wrote:
>
> On Tue, 2023-07-18 at 09:35 -0400, Jeff Layton wrote:
>> On Tue, 2023-07-18 at 08:38 -0400, trondmy@xxxxxxxxxx wrote:
>>> From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>>>
>>> If the client is calling TEST_STATEID, then it is because some
>>> event
>>> occurred that requires it to check all the stateids for validity
>>> and
>>> call FREE_STATEID on the ones that have been revoked. In this case,
>>> either the stateid exists in the list of stateids associated with
>>> that
>>> nfs4_client, in which case it should be tested, or it does not.
>>> There
>>> are no additional conditions to be considered.
>>>
>>> Reported-by: Frank Ch. Eigler <fche@xxxxxxxxxx>
>>> Fixes: 663e36f07666 ("nfsd4: kill warnings on testing stateids with
>>> mismatched clientids")
>>> Cc: stable@xxxxxxxxxxxxxxx
>>> Signed-off-by: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx>
>>> ---
>>> fs/nfsd/nfs4state.c | 2 --
>>> 1 file changed, 2 deletions(-)
>>>
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 6e61fa3acaf1..3aefbad4cc09 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -6341,8 +6341,6 @@ static __be32 nfsd4_validate_stateid(struct
>>> nfs4_client *cl, stateid_t *stateid)
>>> if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
>>> CLOSE_STATEID(stateid))
>>> return status;
>>> - if (!same_clid(&stateid->si_opaque.so_clid, &cl-
>>>> cl_clientid))
>>> - return status;
>>> spin_lock(&cl->cl_lock);
>>> s = find_stateid_locked(cl, stateid);
>>> if (!s)
>>
>> IDGI. Is this fixing an actual bug? Granted this code does seem
>> unnecessary, but removing it doesn't seem like it will cause any
>> user-visible change in behavior. Am I missing something?
>
> It was clearly triggering in
> https://bugzilla.redhat.com/show_bug.cgi?id=2176575
>
> Furthermore, if you look at commit 663e36f07666, you'll see that all it
> does is remove the log message because "it is expected". For some
> unknown reason, it did not register that "then the check is incorrect".
I don't think 663e36f altered this logic: it "returned status"
when it emitted the warning, and it "returned status" after
the warning was removed.
> So yes, this is fixing a real bug.
If there is a bug, wouldn't it have been introduced when the
"!same_clid()" check was added?
Fixes: 7df302f75ee2 ("NFSD: TEST_STATEID should not return NFS4ERR_STALE_STATEID")
--
Chuck Lever
