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.
--
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@xxxxxxxxxxxxxxx
