Re: [PATCH v2 2/6] NFSD: Prevent client use-after-free during delegation revoke

On Sun, 2026-07-05 at 21:25 -0400, Chuck Lever wrote:
> A delegation stateid stores only a bare pointer to its owning
> nfs4_client; a reference on the stateid does not keep the client
> alive.  The client outlives its stateids solely because
> __destroy_client() drains every delegation from cl_delegations and
> cl_revoked before free_client() runs.
> 
> nfs4_laundromat() breaks that invariant.  It unhashes an expired
> delegation from cl_delegations under deleg_lock, drops the lock, and
> calls revoke_delegation(), which reacquires clp->cl_lock and links the
> delegation onto clp->cl_revoked.  Between the unhash and the revoke the
> delegation is on neither client-reachable list, so client_has_state()
> can report no remaining state.
> 
> Every path that can free a client holding delegations first requires
> cl_rpc_users to be zero: DESTROY_CLIENTID and a superseding EXCHANGE_ID
> gate on mark_client_expired_locked(), and the admin "expire" write
> waits in force_expire_client().  The laundromat holds no such
> reference.  A client whose recalled delegation has just timed out -- a
> rebooted client whose new incarnation supersedes the old one, say --
> can therefore run __destroy_client() to completion and free the client
> while revoke_delegation() is still about to dereference clp->cl_lock
> and clp->cl_revoked, a use-after-free.
> 
> Pin the client with cl_rpc_users across the revoke so the teardown
> paths block until it completes and then reap it from cl_revoked
> themselves.  A client already expiring reaps its own delegations, so
> skip it and leave the delegation on del_recall_lru for
> __destroy_client() to handle.
> 
> Fixes: 3bd64a5ba171 ("nfsd4: implement SEQ4_STATUS_RECALLABLE_STATE_REVOKED")
> Signed-off-by: Chuck Lever <[email protected]>
> ---
>  fs/nfsd/netns.h     |  6 ++++--
>  fs/nfsd/nfs4state.c | 23 +++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
> index 03724bef10a7..a7bd7b67fa4f 100644
> --- a/fs/nfsd/netns.h
> +++ b/fs/nfsd/netns.h
> @@ -115,7 +115,8 @@ struct nfsd_net {
>  	struct list_head client_lru;
>  	struct list_head close_lru;
>  
> -	/* protects del_recall_lru and delegation hash/unhash */
> +	/* protects del_recall_lru and delegation hash/unhash;
> +	 * nests outside client_lock */
>  	spinlock_t deleg_lock ____cacheline_aligned;
>  	struct list_head del_recall_lru;
>  
> @@ -124,7 +125,8 @@ struct nfsd_net {
>  
>  	struct delayed_work laundromat_work;
>  
> -	/* client_lock protects the client lru list and session hash table */
> +	/* client_lock protects the client lru list and session hash
> +	 * table; nests inside deleg_lock */
>  	spinlock_t client_lock;
>  
>  	/* protects blocked_locks_lru */
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index e000ed3e96e9..efeb2a2e9c8f 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -7457,6 +7457,7 @@ nfs4_laundromat(struct nfsd_net *nn)
>  		.new_timeo = nn->nfsd4_lease
>  	};
>  	struct nfs4_cpntf_state *cps;
> +	struct nfs4_client *clp;
>  	copy_stateid_t *cps_t;
>  	int i;
>  
> @@ -7485,6 +7486,18 @@ nfs4_laundromat(struct nfsd_net *nn)
>  		dp = list_entry (pos, struct nfs4_delegation, dl_recall_lru);
>  		if (!state_expired(&lt, dp->dl_time))
>  			break;
> +		clp = dp->dl_stid.sc_client;
> +		spin_lock(&nn->client_lock);
> +		if (is_client_expired(clp)) {
> +			spin_unlock(&nn->client_lock);
> +			continue;
> +		}
> +		/*
> +		 * Pin without reviving: get_client_locked() would
> +		 * flip a courtesy client back to NFSD4_ACTIVE.
> +		 */
> +		atomic_inc(&clp->cl_rpc_users);
> +		spin_unlock(&nn->client_lock);
>  		refcount_inc(&dp->dl_stid.sc_count);
>  		unhash_delegation_locked(dp, SC_STATUS_REVOKED);
>  		list_add(&dp->dl_recall_lru, &reaplist);
> @@ -7493,8 +7506,18 @@ nfs4_laundromat(struct nfsd_net *nn)
>  	while (!list_empty(&reaplist)) {
>  		dp = list_first_entry(&reaplist, struct nfs4_delegation,
>  					dl_recall_lru);
> +		clp = dp->dl_stid.sc_client;
>  		list_del_init(&dp->dl_recall_lru);
>  		revoke_delegation(dp);
> +		/*
> +		 * Unpin without renewing: put_client_renew() would
> +		 * renew the reaped client's lease.
> +		 */
> +		if (atomic_dec_and_lock(&clp->cl_rpc_users, &nn->client_lock)) {
> +			if (is_client_expired(clp))
> +				wake_up_all(&expiry_wq);
> +			spin_unlock(&nn->client_lock);
> +		}
>  	}
>  
>  	spin_lock(&nn->client_lock);

Reviewed-by: Jeff Layton <[email protected]>