On Tue, 2023-07-18 at 09:51 -0400, Trond Myklebust 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". > Yeah, that commit just removes the warning, AFAICT. > So yes, this is fixing a real bug. > My assumption was that for any stateid that the server hands out, the si_opaque.so_clid must match the clid. But...it looks like s2s copy might have changed that rule? In any case, the patch looks fine, so I have no objection. I'm just trying to understand how this could happen. Reviewed-by: Jeff Layton <jlayton@xxxxxxxxxx>