Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <106daf5e0242b67bdb04e2e8e4ae7e114a4b47ab.camel@kernel.org>
Subject: Re: [PATCH] nfsd: Remove incorrect check in nfsd4_validate_stateid
From:   Trond Myklebust <trondmy@kernel.org>
To:     Jeff Layton <jlayton@kernel.org>,
        Chuck Lever <chuck.lever@oracle.com>
Cc:     linux-nfs@vger.kernel.org
Date:   Tue, 18 Jul 2023 09:51:31 -0400
In-Reply-To: <6dc89e4859a6851773bc2931d919e1cb204ae690.camel@kernel.org>
References: <20230718123837.124780-1-trondmy@kernel.org>
         <6dc89e4859a6851773bc2931d919e1cb204ae690.camel@kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
User-Agent: Evolution 3.48.4 (3.48.4-1.fc38) 
MIME-Version: 1.0
Precedence: bulk

On Tue, 2023-07-18 at 09:35 -0400, Jeff Layton wrote:
> On Tue, 2023-07-18 at 08:38 -0400, trondmy@kernel.org wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > 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@redhat.com>
> > Fixes: 663e36f07666 ("nfsd4: kill warnings on testing stateids with
> > mismatched clientids")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  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".

So yes, this is fixing a real bug.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com