[PATCH] NFSD: Fix delegation reference leak in nfsd4_revoke_states

[PATCH] NFSD: Fix delegation reference leak in nfsd4_revoke_states

[Chuck Lever]

From: Chuck Lever <chuck.lever@oracle.com>

When revoking delegation state, nfsd4_revoke_states() takes an extra
reference on the stid before calling unhash_delegation_locked(). If
unhash_delegation_locked() returns false (the delegation was already
unhashed by a concurrent path), dp is set to NULL and
revoke_delegation() is skipped, but the extra reference is never
released. Each occurrence permanently pins the stid in memory. The
leaked reference also prevents nfs4_put_stid() from decrementing
cl_admin_revoked, leaving the counter permanently inflated.

Drop the extra reference in the failure path.

Fixes: 8dd91e8d31fe ("nfsd: fix race between laundromat and free_stateid")
Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/nfsd/nfs4state.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6b9c399b89df..0fefae6b0a48 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1389,7 +1389,8 @@ static void destroy_delegation(struct nfs4_delegation *dp)
  * stateid or it's called from a laundromat thread (nfsd4_landromat()) that
  * determined that this specific state has expired and needs to be revoked
  * (both mark state with the appropriate stid sc_status mode). It is also
- * assumed that a reference was taken on the @dp state.
+ * assumed that a reference was taken on the @dp state. This function
+ * consumes that reference.
  *
  * If this function finds that the @dp state is SC_STATUS_FREED it means
  * that a FREE_STATEID operation for this stateid has been processed and
@@ -1836,6 +1837,10 @@ void nfsd4_revoke_states(struct nfsd_net *nn, struct super_block *sb)
 					mutex_unlock(&stp->st_mutex);
 					break;
 				case SC_TYPE_DELEG:
+					/* Extra reference guards against concurrent
+					 * FREE_STATEID; revoke_delegation() consumes
+					 * it, otherwise release it directly.
+					 */
 					refcount_inc(&stid->sc_count);
 					dp = delegstateid(stid);
 					spin_lock(&state_lock);
@@ -1845,6 +1850,8 @@ void nfsd4_revoke_states(struct nfsd_net *nn, struct super_block *sb)
 					spin_unlock(&state_lock);
 					if (dp)
 						revoke_delegation(dp);
+					else
+						nfs4_put_stid(stid);
 					break;
 				case SC_TYPE_LAYOUT:
 					ls = layoutstateid(stid);
-- 
2.53.0

Re: [PATCH] NFSD: Fix delegation reference leak in nfsd4_revoke_states

[Jeff Layton]

On Tue, 2026-03-24 at 11:18 -0400, Chuck Lever wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> When revoking delegation state, nfsd4_revoke_states() takes an extra
> reference on the stid before calling unhash_delegation_locked(). If
> unhash_delegation_locked() returns false (the delegation was already
> unhashed by a concurrent path), dp is set to NULL and
> revoke_delegation() is skipped, but the extra reference is never
> released. Each occurrence permanently pins the stid in memory. The
> leaked reference also prevents nfs4_put_stid() from decrementing
> cl_admin_revoked, leaving the counter permanently inflated.
> 
> Drop the extra reference in the failure path.
> 
> Fixes: 8dd91e8d31fe ("nfsd: fix race between laundromat and free_stateid")
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/nfsd/nfs4state.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 6b9c399b89df..0fefae6b0a48 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1389,7 +1389,8 @@ static void destroy_delegation(struct nfs4_delegation *dp)
>   * stateid or it's called from a laundromat thread (nfsd4_landromat()) that
>   * determined that this specific state has expired and needs to be revoked
>   * (both mark state with the appropriate stid sc_status mode). It is also
> - * assumed that a reference was taken on the @dp state.
> + * assumed that a reference was taken on the @dp state. This function
> + * consumes that reference.
>   *
>   * If this function finds that the @dp state is SC_STATUS_FREED it means
>   * that a FREE_STATEID operation for this stateid has been processed and
> @@ -1836,6 +1837,10 @@ void nfsd4_revoke_states(struct nfsd_net *nn, struct super_block *sb)
>  					mutex_unlock(&stp->st_mutex);
>  					break;
>  				case SC_TYPE_DELEG:
> +					/* Extra reference guards against concurrent
> +					 * FREE_STATEID; revoke_delegation() consumes
> +					 * it, otherwise release it directly.
> +					 */
>  					refcount_inc(&stid->sc_count);
>  					dp = delegstateid(stid);
>  					spin_lock(&state_lock);
> @@ -1845,6 +1850,8 @@ void nfsd4_revoke_states(struct nfsd_net *nn, struct super_block *sb)
>  					spin_unlock(&state_lock);
>  					if (dp)
>  						revoke_delegation(dp);
> +					else
> +						nfs4_put_stid(stid);
>  					break;
>  				case SC_TYPE_LAYOUT:
>  					ls = layoutstateid(stid);

Good catch!

Reviewed-by: Jeff Layton <jlayton@kernel.org>