Re: [PATCH] nfsd: prevent a double put of revoked layout stateids

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2026-10-12 at 17:47 +0900, Jinpyo Lee wrote:
> nfsd4_free_stateid() finds an admin-revoked layout stateid with
> find_stateid_locked(), which does not acquire a reference. Since the
> layout case does not mark the stateid closed before dropping cl_lock,
> two FREE_STATEID operations in separate session slots can both select
> the same stateid and drop a reference they do not own.
> 
> With fence-work teardown in place, the second caller can also wait in
> nfsd4_stop_layout_fence() until the first caller frees the stateid, then
> resume through its freed fence mutex and layout lock.
> 
> Mark the layout stateid CLOSED under cl_lock and ls_lock so only one
> disposal caller proceeds. Take a temporary reference before dropping
> cl_lock and hold it while stopping fence work and draining the remaining
> layout segments, then release that reference with the final put.
> 
> The layout stateid IDR does not own a separate reference. The initial
> allocation reference is released when LAYOUTGET completes, and each
> layout segment holds its own reference. Serialize segment draining with
> in-flight layout operations using ls_mutex, and recheck CLOSED after
> acquiring that mutex so a stale LAYOUTGET cannot add a segment after the
> drain. Also make the newly allocated layout stateid the error-path put
> target.
> 
> Read CLOSED under ls_lock in FREE_STATEID, and restrict
> nfs40_clean_admin_revoked() to NFSv4.0 clients. Session clients dispose
> of revoked stateids through FREE_STATEID and must not be processed by
> the NFSv4.0 cleanup while layout disposal is in progress.
> 
> The vulnerability research and validation were conducted by members of
> the Tobabz team as part of the Best of the Best 15th program.
> 
> Fixes: 86b9898920a6 ("NFSD: Handle layout stid in nfsd4_drop_revoked_stid()")
> Assisted-by: LLM
> Signed-off-by: Jinpyo Lee <bint4b13@gmail.com>
> ---
> Validated on nfsd-testing d918c664efd5ec6a7309e98f567fb018bcde0e11
> (2026-10-12):
> - Applied cleanly and passed strict checkpatch with no errors or warnings.
> - Built and booted an x86_64 kernel and modules with GCC 13.3, W=1,
>   Generic KASAN, lockdep, and pNFS block and SCSI layouts enabled.
> - NFSv4.2, NFSv4.0, and NFSv3 read/write, fio, and unmount tests passed.
> - On a real loopback pNFS SCSI layout, administrator revocation followed
>   by one FREE_STATEID removed the layout stateid and reduced the
>   admin-revoked count from 2 to 1. The sequence passed twice.
> - Tracing observed nfsd4_stop_layout_fence() in both revocation and
>   FREE_STATEID processing, followed by nfsd_layoutstate_free.
> - No KASAN, lockdep, refcount, list-corruption, or Oops report occurred;
>   kernel taint remained zero.
> 
> The full xfstests suite and a forced concurrent two-slot FREE_STATEID
> race were not run on this revision.
> 
>  fs/nfsd/nfs4layouts.c | 24 ++++++++++++++++++++++++
>  fs/nfsd/nfs4state.c   | 37 ++++++++++++++++++++++++++++++++++++-
>  fs/nfsd/pnfs.h        |  6 ++++++
>  3 files changed, 66 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4layouts.c b/fs/nfsd/nfs4layouts.c
> index 898776c9d..74c39e216 100644
> --- a/fs/nfsd/nfs4layouts.c
> +++ b/fs/nfsd/nfs4layouts.c
> @@ -312,6 +312,7 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
>  		status = nfserr_jukebox;
>  		if (!ls)
>  			goto out;
> +		stid = &ls->ls_stid;
>  		mutex_lock(&ls->ls_mutex);
>  	} else {
>  		ls = container_of(stid, struct nfs4_layout_stateid, ls_stid);
> @@ -324,6 +325,15 @@ nfsd4_preprocess_layout_stateid(struct svc_rqst *rqstp,
>  			goto out_unlock_stid;
>  	}
>  
> +	/* A new or looked-up stateid may have been closed before ls_mutex. */
> +	spin_lock(&ls->ls_lock);
> +	if (ls->ls_stid.sc_status & SC_STATUS_CLOSED) {
> +		spin_unlock(&ls->ls_lock);
> +		status = nfserr_bad_stateid;
> +		goto out_unlock_stid;
> +	}
> +	spin_unlock(&ls->ls_lock);
> +
>  	*lsp = ls;
>  	return 0;
>  
> @@ -594,6 +604,20 @@ nfsd4_return_all_layouts(struct nfs4_layout_stateid *ls,
>  	spin_unlock(&ls->ls_lock);
>  }
>  
> +void nfsd4_return_revoked_layout(struct nfs4_layout_stateid *ls)
> +{
> +	LIST_HEAD(reaplist);
> +
> +	/*
> +	 * The caller holds a stateid reference until segment draining ends.
> +	 * Wait for any LAYOUTGET that passed the CLOSED check.
> +	 */
> +	mutex_lock(&ls->ls_mutex);
> +	nfsd4_return_all_layouts(ls, &reaplist);
> +	mutex_unlock(&ls->ls_mutex);
> +	nfsd4_free_layouts(&reaplist);
> +}
> +
>  void
>  nfsd4_return_all_client_layouts(struct nfs4_client *clp)
>  {
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 920967dd6..02bfbd242 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -5837,6 +5837,24 @@ nfsd4_find_existing_open(struct nfs4_file *fp, struct nfsd4_open *open)
>  	return ret;
>  }
>  
> +/* cl_lock is held; layout stateid status also requires ls_lock. */
> +static bool nfsd4_stid_is_closed(struct nfs4_stid *s)
> +{
> +	struct nfs4_layout_stateid *ls;
> +	bool closed;
> +
> +	lockdep_assert_held(&s->sc_client->cl_lock);
> +
> +	if (s->sc_type != SC_TYPE_LAYOUT)
> +		return s->sc_status & SC_STATUS_CLOSED;
> +
> +	ls = layoutstateid(s);
> +	spin_lock(&ls->ls_lock);
> +	closed = s->sc_status & SC_STATUS_CLOSED;
> +	spin_unlock(&ls->ls_lock);
> +	return closed;
> +}
> +

^^^ 
I must say that I'm not thrilled with the different locking rules for
SC_STATUS_CLOSED with different types of stateids. That has a strong
code smell in my opinion and will be difficult (for humans) to keep
straight.

I don't have a better suggestion for the moment, but I'd strongly
prefer something that didn't add these extra rules around a flag common
to all stateids.

>  static void nfsd4_drop_revoked_stid(struct nfs4_stid *s)
>  	__releases(&s->sc_client->cl_lock)
>  {
> @@ -5871,9 +5889,24 @@ static void nfsd4_drop_revoked_stid(struct nfs4_stid *s)
>  		break;
>  	case SC_TYPE_LAYOUT:
>  		ls = layoutstateid(s);
> +		spin_lock(&ls->ls_lock);
> +		if (s->sc_status & SC_STATUS_CLOSED) {
> +			spin_unlock(&ls->ls_lock);
> +			spin_unlock(&cl->cl_lock);
> +			break;
> +		}
> +		s->sc_status |= SC_STATUS_CLOSED;
> +		spin_unlock(&ls->ls_lock);
> +		/*
> +		 * The IDR does not own a layout stateid reference. Keep ls
> +		 * alive while draining worker and segment references below;
> +		 * the final put releases this temporary reference.
> +		 */
> +		refcount_inc(&s->sc_count);
>  		list_del_init(&ls->ls_perclnt);
>  		spin_unlock(&cl->cl_lock);
>  		nfsd4_stop_layout_fence(ls);
> +		nfsd4_return_revoked_layout(ls);
>  		nfs4_put_stid(s);
>  		break;
>  	default:
> @@ -7899,6 +7932,8 @@ static void nfs40_clean_admin_revoked(struct nfsd_net *nn,
>  		unsigned long id, tmp;
>  		struct nfs4_stid *stid;
>  
> +		if (clp->cl_minorversion != 0)
> +			continue;
>  		if (atomic_read(&clp->cl_admin_revoked) == 0)
>  			continue;
>  		if (is_client_expired(clp))
> @@ -8698,7 +8733,7 @@ nfsd4_free_stateid(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  	spin_lock(&cl->cl_lock);
>  	s = find_stateid_locked(cl, stateid);
> -	if (!s || s->sc_status & SC_STATUS_CLOSED)
> +	if (!s || nfsd4_stid_is_closed(s))
>  		goto out_unlock;
>  	if (s->sc_status & SC_STATUS_ADMIN_REVOKED) {
>  		nfsd4_drop_revoked_stid(s);
> diff --git a/fs/nfsd/pnfs.h b/fs/nfsd/pnfs.h
> index 559df0e5b..9b0f17a4d 100644
> --- a/fs/nfsd/pnfs.h
> +++ b/fs/nfsd/pnfs.h
> @@ -75,6 +75,7 @@ struct nfsd4_deviceid_map *nfsd4_find_devid_map(int idx);
>  #ifdef CONFIG_NFSD_PNFS
>  void nfsd4_setup_layout_type(struct svc_export *exp);
>  void nfsd4_return_all_client_layouts(struct nfs4_client *);
> +void nfsd4_return_revoked_layout(struct nfs4_layout_stateid *ls);
>  void nfsd4_return_all_file_layouts(struct nfs4_client *clp,
>  		struct nfs4_file *fp);
>  void nfsd4_close_layout(struct nfs4_layout_stateid *ls);
> @@ -93,6 +94,11 @@ static inline void nfsd4_setup_layout_type(struct svc_export *exp)
>  static inline void nfsd4_return_all_client_layouts(struct nfs4_client *clp)
>  {
>  }
> +
> +static inline void nfsd4_return_revoked_layout(struct nfs4_layout_stateid *ls)
> +{
> +}
> +
>  static inline void nfsd4_return_all_file_layouts(struct nfs4_client *clp,
>  		struct nfs4_file *fp)
>  {
> 
> base-commit: d918c664efd5ec6a7309e98f567fb018bcde0e11

-- 
Jeff Layton <jlayton@kernel.org>