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: <cd4cdc83d81eb23faf919136b8ddfe5e52cfc052.camel@kernel.org>
Subject: Re: [PATCH] nfsd: Remove incorrect check in nfsd4_validate_stateid
From:   Trond Myklebust <trondmy@kernel.org>
To:     Chuck Lever III <chuck.lever@oracle.com>
Cc:     Jeff Layton <jlayton@kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>
Date:   Tue, 18 Jul 2023 10:30:56 -0400
In-Reply-To: <0F52C77C-C1AA-48E1-B30A-CD15342EEAFF@oracle.com>
References: <20230718123837.124780-1-trondmy@kernel.org>
         <6dc89e4859a6851773bc2931d919e1cb204ae690.camel@kernel.org>
         <106daf5e0242b67bdb04e2e8e4ae7e114a4b47ab.camel@kernel.org>
         <0F52C77C-C1AA-48E1-B30A-CD15342EEAFF@oracle.com>
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 14:12 +0000, Chuck Lever III wrote:
> 
> 
> > On Jul 18, 2023, at 9:51 AM, Trond Myklebust <trondmy@kernel.org>
> > wrote:
> > 
> > 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".
> 
> 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@hammerspace.com