diff mbox series

nfsd: allow SC_STATUS_FREEABLE when searching via nfs4_lookup_stateid()

Message ID 20250212-nfsd-fixes-v1-1-935e3a4919fc@kernel.org (mailing list archive)
State Changes Requested
Delegated to: Chuck Lever
Headers show
Series nfsd: allow SC_STATUS_FREEABLE when searching via nfs4_lookup_stateid() | expand

Commit Message

Jeff Layton Feb. 12, 2025, 4:29 p.m. UTC 
When a delegation is revoked, it's initially marked with
SC_STATUS_REVOKED, or SC_STATUS_ADMIN_REVOKED and later, it's marked
with the SC_STATUS_FREEABLE flag, which denotes that it is waiting for
s FREE_STATEID call.

nfs4_lookup_stateid() accepts a statusmask that includes the status
flags that a found stateid is allowed to have. Currently, that mask
never includes SC_STATUS_FREEABLE, which means that revoked delegations
are (almost) never found.

Add SC_STATUS_FREEABLE to the always-allowed status flags.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
This fixes the pynfs DELEG8 test.
---
 fs/nfsd/nfs4state.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


---
base-commit: 4990d098433db18c854e75fb0f90d941eb7d479e
change-id: 20250212-nfsd-fixes-fa8047082335

Best regards,

Comments

Chuck Lever Feb. 12, 2025, 4:34 p.m. UTC | #1 
On 2/12/25 11:29 AM, Jeff Layton wrote:
> When a delegation is revoked, it's initially marked with
> SC_STATUS_REVOKED, or SC_STATUS_ADMIN_REVOKED and later, it's marked
> with the SC_STATUS_FREEABLE flag, which denotes that it is waiting for
> s FREE_STATEID call.
> 
> nfs4_lookup_stateid() accepts a statusmask that includes the status
> flags that a found stateid is allowed to have. Currently, that mask
> never includes SC_STATUS_FREEABLE, which means that revoked delegations
> are (almost) never found.
> 
> Add SC_STATUS_FREEABLE to the always-allowed status flags.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> This fixes the pynfs DELEG8 test.

A little release engineering:

 - The DELEG8 mention IMO should be the lede in the patch description

 - Do we want Fixes: and/or Cc: stable tags?


> ---
>  fs/nfsd/nfs4state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 153eeea2c7c999d003cd1f36cecb0dd4f6e049b8..56bf07d623d085589823f3fba18afa62c0b3dbd2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -7051,7 +7051,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  		 */
>  		statusmask |= SC_STATUS_REVOKED;
>  
> -	statusmask |= SC_STATUS_ADMIN_REVOKED;
> +	statusmask |= SC_STATUS_ADMIN_REVOKED | SC_STATUS_FREEABLE;
>  
>  	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
>  		CLOSE_STATEID(stateid))
> 
> ---
> base-commit: 4990d098433db18c854e75fb0f90d941eb7d479e
> change-id: 20250212-nfsd-fixes-fa8047082335
> 
> Best regards,
Jeff Layton Feb. 12, 2025, 4:36 p.m. UTC | #2 
On Wed, 2025-02-12 at 11:29 -0500, Jeff Layton wrote:
> When a delegation is revoked, it's initially marked with
> SC_STATUS_REVOKED, or SC_STATUS_ADMIN_REVOKED and later, it's marked
> with the SC_STATUS_FREEABLE flag, which denotes that it is waiting for
> s FREE_STATEID call.
> 
> nfs4_lookup_stateid() accepts a statusmask that includes the status
> flags that a found stateid is allowed to have. Currently, that mask
> never includes SC_STATUS_FREEABLE, which means that revoked delegations
> are (almost) never found.
> 
> Add SC_STATUS_FREEABLE to the always-allowed status flags.
> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> This fixes the pynfs DELEG8 test.
> ---
>  fs/nfsd/nfs4state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 153eeea2c7c999d003cd1f36cecb0dd4f6e049b8..56bf07d623d085589823f3fba18afa62c0b3dbd2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -7051,7 +7051,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  		 */
>  		statusmask |= SC_STATUS_REVOKED;
>  
> -	statusmask |= SC_STATUS_ADMIN_REVOKED;
> +	statusmask |= SC_STATUS_ADMIN_REVOKED | SC_STATUS_FREEABLE;
>  
>  	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
>  		CLOSE_STATEID(stateid))
> 
> ---
> base-commit: 4990d098433db18c854e75fb0f90d941eb7d479e
> change-id: 20250212-nfsd-fixes-fa8047082335
> 
> Best regards,

I didn't add this, but this seems like the likely place where it was
broken, since this was where SC_STATUS_FREEABLE was added:

    Fixes: 8dd91e8d31fe ("nfsd: fix race between laundromat and free_stateid")
NeilBrown Feb. 12, 2025, 9:13 p.m. UTC | #3 
On Thu, 13 Feb 2025, Jeff Layton wrote:
> When a delegation is revoked, it's initially marked with
> SC_STATUS_REVOKED, or SC_STATUS_ADMIN_REVOKED and later, it's marked
> with the SC_STATUS_FREEABLE flag, which denotes that it is waiting for
> s FREE_STATEID call.
> 
> nfs4_lookup_stateid() accepts a statusmask that includes the status
> flags that a found stateid is allowed to have. Currently, that mask
> never includes SC_STATUS_FREEABLE, which means that revoked delegations
> are (almost) never found.
> 
> Add SC_STATUS_FREEABLE to the always-allowed status flags.

There are 4 calls to nfsd4_lookup_stateid().  One already has
SC_STATUS_FREEABLE passed.  Which of the others need it?
If all of them, then this patch is sensible but should also remove the
flag in the one place it is already passed.
If only one other call needs it, then maybe we should just pass it
there?

Could you at least include in the description some detail of what
request is failing and which particular nfsd4_lookup_stateid() call is
relevant in that case?

Thanks,
NeilBrown


> 
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> This fixes the pynfs DELEG8 test.
> ---
>  fs/nfsd/nfs4state.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 153eeea2c7c999d003cd1f36cecb0dd4f6e049b8..56bf07d623d085589823f3fba18afa62c0b3dbd2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -7051,7 +7051,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  		 */
>  		statusmask |= SC_STATUS_REVOKED;
>  
> -	statusmask |= SC_STATUS_ADMIN_REVOKED;
> +	statusmask |= SC_STATUS_ADMIN_REVOKED | SC_STATUS_FREEABLE;
>  
>  	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
>  		CLOSE_STATEID(stateid))
> 
> ---
> base-commit: 4990d098433db18c854e75fb0f90d941eb7d479e
> change-id: 20250212-nfsd-fixes-fa8047082335
> 
> Best regards,
> -- 
> Jeff Layton <jlayton@kernel.org>
> 
>
Dai Ngo Feb. 12, 2025, 9:41 p.m. UTC | #4 
On 2/12/25 8:29 AM, Jeff Layton wrote:
> When a delegation is revoked, it's initially marked with
> SC_STATUS_REVOKED, or SC_STATUS_ADMIN_REVOKED and later, it's marked
> with the SC_STATUS_FREEABLE flag, which denotes that it is waiting for
> s FREE_STATEID call.
>
> nfs4_lookup_stateid() accepts a statusmask that includes the status
> flags that a found stateid is allowed to have. Currently, that mask
> never includes SC_STATUS_FREEABLE, which means that revoked delegations
> are (almost) never found.
>
> Add SC_STATUS_FREEABLE to the always-allowed status flags.
>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
> This fixes the pynfs DELEG8 test.
> ---
>   fs/nfsd/nfs4state.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 153eeea2c7c999d003cd1f36cecb0dd4f6e049b8..56bf07d623d085589823f3fba18afa62c0b3dbd2 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -7051,7 +7051,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>   		 */
>   		statusmask |= SC_STATUS_REVOKED;
>   
> -	statusmask |= SC_STATUS_ADMIN_REVOKED;
> +	statusmask |= SC_STATUS_ADMIN_REVOKED | SC_STATUS_FREEABLE;

I think it's safer to add the SC_STATUS_FREEABLE inside the if statement
that checks for SC_TYPE_DELEG above.

Also, don't we also need to mark the delegation with SC_STATUS_REVOKED in
revoke_delegation()?

-Dai

>   
>   	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
>   		CLOSE_STATEID(stateid))
>
> ---
> base-commit: 4990d098433db18c854e75fb0f90d941eb7d479e
> change-id: 20250212-nfsd-fixes-fa8047082335
>
> Best regards,
Jeff Layton Feb. 13, 2025, 12:52 p.m. UTC | #5 
On Thu, 2025-02-13 at 08:13 +1100, NeilBrown wrote:
> On Thu, 13 Feb 2025, Jeff Layton wrote:
> > When a delegation is revoked, it's initially marked with
> > SC_STATUS_REVOKED, or SC_STATUS_ADMIN_REVOKED and later, it's marked
> > with the SC_STATUS_FREEABLE flag, which denotes that it is waiting for
> > s FREE_STATEID call.
> > 
> > nfs4_lookup_stateid() accepts a statusmask that includes the status
> > flags that a found stateid is allowed to have. Currently, that mask
> > never includes SC_STATUS_FREEABLE, which means that revoked delegations
> > are (almost) never found.
> > 
> > Add SC_STATUS_FREEABLE to the always-allowed status flags.
> 
> There are 4 calls to nfsd4_lookup_stateid().  One already has
> SC_STATUS_FREEABLE passed.  Which of the others need it?
 
> If all of them, then this patch is sensible but should also remove the
> flag in the one place it is already passed.
> If only one other call needs it, then maybe we should just pass it
> there?
> 
> Could you at least include in the description some detail of what
> request is failing and which particular nfsd4_lookup_stateid() call is
> relevant in that case?
> 
> Thanks,
> NeilBrown
> 

I think they all need it.

The DELEG8 test gets a delegation, and then lets the lease time out. It
then does a READ op and tests that it gets back NFS4ERR_DELEG_REVOKED.
The read path calls nfs4_preprocess_stateid_op(), so it's needed there
specifically to fix that bug.

1ac3629bf0125 ensures that SC_STATUS_ADMIN_REVOKED is always set in the
allowed mask. If that's always allowed, then FREEABLE must be too.
There is only a very narrow window of time when ADMIN_REVOKED or
REVOKED is set, and FREEABLE is not.

I'll respin and remove the other FREEABLE flag, since I think it should
be implied everywhere.

> 
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > This fixes the pynfs DELEG8 test.
> > ---
> >  fs/nfsd/nfs4state.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 153eeea2c7c999d003cd1f36cecb0dd4f6e049b8..56bf07d623d085589823f3fba18afa62c0b3dbd2 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -7051,7 +7051,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
> >  		 */
> >  		statusmask |= SC_STATUS_REVOKED;
> >  
> > -	statusmask |= SC_STATUS_ADMIN_REVOKED;
> > +	statusmask |= SC_STATUS_ADMIN_REVOKED | SC_STATUS_FREEABLE;
> >  
> >  	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
> >  		CLOSE_STATEID(stateid))
> > 
> > ---
> > base-commit: 4990d098433db18c854e75fb0f90d941eb7d479e
> > change-id: 20250212-nfsd-fixes-fa8047082335
> > 
> > Best regards,
> > -- 
> > Jeff Layton <jlayton@kernel.org>
> > 
> > 
>
Jeff Layton Feb. 13, 2025, 12:55 p.m. UTC | #6 
On Wed, 2025-02-12 at 13:41 -0800, Dai Ngo wrote:
> On 2/12/25 8:29 AM, Jeff Layton wrote:
> > When a delegation is revoked, it's initially marked with
> > SC_STATUS_REVOKED, or SC_STATUS_ADMIN_REVOKED and later, it's marked
> > with the SC_STATUS_FREEABLE flag, which denotes that it is waiting for
> > s FREE_STATEID call.
> > 
> > nfs4_lookup_stateid() accepts a statusmask that includes the status
> > flags that a found stateid is allowed to have. Currently, that mask
> > never includes SC_STATUS_FREEABLE, which means that revoked delegations
> > are (almost) never found.
> > 
> > Add SC_STATUS_FREEABLE to the always-allowed status flags.
> > 
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> > This fixes the pynfs DELEG8 test.
> > ---
> >   fs/nfsd/nfs4state.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > index 153eeea2c7c999d003cd1f36cecb0dd4f6e049b8..56bf07d623d085589823f3fba18afa62c0b3dbd2 100644
> > --- a/fs/nfsd/nfs4state.c
> > +++ b/fs/nfsd/nfs4state.c
> > @@ -7051,7 +7051,7 @@ nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
> >   		 */
> >   		statusmask |= SC_STATUS_REVOKED;
> >   
> > -	statusmask |= SC_STATUS_ADMIN_REVOKED;
> > +	statusmask |= SC_STATUS_ADMIN_REVOKED | SC_STATUS_FREEABLE;
> 
> I think it's safer to add the SC_STATUS_FREEABLE inside the if statement
> that checks for SC_TYPE_DELEG above.
> 

That's not enough. SC_STATUS_ADMIN_REVOKED is always allowed, and for
that to work properly, we need to always allow SC_STATUS_FREEABLE too.

> Also, don't we also need to mark the delegation with SC_STATUS_REVOKED in
> revoke_delegation()?
> 

No. The idea is that the REVOKED flag should be set before calling
revoke_delegation(). Typically they get marked REVOKED or ADMIN_REVOKED
under spinlock, and then we call revoke_delegation() afterward to
finish the job.
 
> >   
> >   	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
> >   		CLOSE_STATEID(stateid))
> > 
> > ---
> > base-commit: 4990d098433db18c854e75fb0f90d941eb7d479e
> > change-id: 20250212-nfsd-fixes-fa8047082335
> > 
> > Best regards,
diff mbox series

Patch

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 153eeea2c7c999d003cd1f36cecb0dd4f6e049b8..56bf07d623d085589823f3fba18afa62c0b3dbd2 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7051,7 +7051,7 @@  nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
 		 */
 		statusmask |= SC_STATUS_REVOKED;
 
-	statusmask |= SC_STATUS_ADMIN_REVOKED;
+	statusmask |= SC_STATUS_ADMIN_REVOKED | SC_STATUS_FREEABLE;
 
 	if (ZERO_STATEID(stateid) || ONE_STATEID(stateid) ||
 		CLOSE_STATEID(stateid))