> On Jul 18, 2023, at 10:30 AM, Trond Myklebust <trondmy@xxxxxxxxxx> wrote:
>
> On Tue, 2023-07-18 at 14:12 +0000, Chuck Lever III wrote:
>>
>>
>>> 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?
>>
>
> Correct.
>
>> Fixes: 7df302f75ee2 ("NFSD: TEST_STATEID should not return
>> NFS4ERR_STALE_STATEID")
>>
>
> It can't fix anything older than that patch, because it won't apply.
Testing now. I plan to apply it to nfsd-fixes (for 6.5-rc).
--
Chuck Lever
