nfsd: Remove incorrect check in nfsd4_validate_stateid

Commit Message

Trond Myklebust July 18, 2023, 12:38 p.m. UTC

From: Trond Myklebust <[e-mail]>

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 <[e-mail]>
Fixes: 663e36f07666 ("nfsd4: kill warnings on testing stateids with mismatched clientids")
Cc: [e-mail]
Signed-off-by: Trond Myklebust <[e-mail]>
---
 fs/nfsd/nfs4state.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Jeff Layton July 18, 2023, 1:35 p.m. UTC | #1

On Tue, 2023-07-18 at 08:38 -0400, [e-mail] wrote:
> From: Trond Myklebust <[e-mail]>
> 
> 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 <[e-mail]>
> Fixes: 663e36f07666 ("nfsd4: kill warnings on testing stateids with mismatched clientids")
> Cc: [e-mail]
> Signed-off-by: Trond Myklebust <[e-mail]>
> ---
>  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?

Trond Myklebust July 18, 2023, 1:51 p.m. UTC | #2

On Tue, 2023-07-18 at 09:35 -0400, Jeff Layton wrote:
> On Tue, 2023-07-18 at 08:38 -0400, [e-mail] wrote:
> > From: Trond Myklebust <[e-mail]>
> > 
> > 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 <[e-mail]>
> > Fixes: 663e36f07666 ("nfsd4: kill warnings on testing stateids with
> > mismatched clientids")
> > Cc: [e-mail]
> > Signed-off-by: Trond Myklebust <[e-mail]>
> > ---
> >  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.

Jeff Layton July 18, 2023, 2:10 p.m. UTC | #3

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, [e-mail] wrote:
> > > From: Trond Myklebust <[e-mail]>
> > > 
> > > 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 <[e-mail]>
> > > Fixes: 663e36f07666 ("nfsd4: kill warnings on testing stateids with
> > > mismatched clientids")
> > > Cc: [e-mail]
> > > Signed-off-by: Trond Myklebust <[e-mail]>
> > > ---
> > >  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 <[e-mail]>

Chuck Lever III July 18, 2023, 2:12 p.m. UTC | #4

> On Jul 18, 2023, at 9:51 AM, Trond Myklebust <[e-mail]> wrote:
> 
> On Tue, 2023-07-18 at 09:35 -0400, Jeff Layton wrote:
>> On Tue, 2023-07-18 at 08:38 -0400, [e-mail] wrote:
>>> From: Trond Myklebust <[e-mail]>
>>> 
>>> 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 <[e-mail]>
>>> Fixes: 663e36f07666 ("nfsd4: kill warnings on testing stateids with
>>> mismatched clientids")
>>> Cc: [e-mail]
>>> Signed-off-by: Trond Myklebust <[e-mail]>
>>> ---
>>>  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?

Fixes: 7df302f75ee2 ("NFSD: TEST_STATEID should not return NFS4ERR_STALE_STATEID")


--
Chuck Lever

Trond Myklebust July 18, 2023, 2:30 p.m. UTC | #5

On Tue, 2023-07-18 at 14:12 +0000, Chuck Lever III wrote:
> 
> 
> > On Jul 18, 2023, at 9:51 AM, Trond Myklebust <[e-mail]>
> > wrote:
> > 
> > On Tue, 2023-07-18 at 09:35 -0400, Jeff Layton wrote:
> > > On Tue, 2023-07-18 at 08:38 -0400, [e-mail] wrote:
> > > > From: Trond Myklebust <[e-mail]>
> > > > 
> > > > 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 <[e-mail]>
> > > > Fixes: 663e36f07666 ("nfsd4: kill warnings on testing stateids
> > > > with
> > > > mismatched clientids")
> > > > Cc: [e-mail]
> > > > Signed-off-by: Trond Myklebust
> > > > <[e-mail]>
> > > > ---
> > > >  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.

Chuck Lever III July 18, 2023, 6:15 p.m. UTC | #6

> On Jul 18, 2023, at 10:30 AM, Trond Myklebust <[e-mail]> wrote:
> 
> On Tue, 2023-07-18 at 14:12 +0000, Chuck Lever III wrote:
>> 
>> 
>>> On Jul 18, 2023, at 9:51 AM, Trond Myklebust <[e-mail]>
>>> wrote:
>>> 
>>> On Tue, 2023-07-18 at 09:35 -0400, Jeff Layton wrote:
>>>> On Tue, 2023-07-18 at 08:38 -0400, [e-mail] wrote:
>>>>> From: Trond Myklebust <[e-mail]>
>>>>> 
>>>>> 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 <[e-mail]>
>>>>> Fixes: 663e36f07666 ("nfsd4: kill warnings on testing stateids
>>>>> with
>>>>> mismatched clientids")
>>>>> Cc: [e-mail]
>>>>> Signed-off-by: Trond Myklebust
>>>>> <[e-mail]>
>>>>> ---
>>>>>  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

Patch

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)