Re: [PATCH v4 9/9] NFSD: Release the export reference when reaping open stateids

On Thu, 2026-07-09 at 13:40 -0400, Chuck Lever wrote:
> nfs4_put_stid() releases the svc_export tracked in
> nfs4_stid.sc_export, but free_ol_stateid_reaplist() frees open and
> lock stateids by calling ->sc_free() directly, bypassing that path.
> An open stateid takes an sc_export reference in nfs4_open() and a
> lock stateid takes its own in init_lock_stateid(); both reach
> free_ol_stateid_reaplist() through their normal teardown, the open
> stateid via release_open_stateid() and the lock stateid via
> nfsd4_release_lockowner(), each through put_ol_stateid_locked().
> The reference is therefore never dropped, pinning the export and
> blocking unmount for the lifetime of the stateid.
> 
> Release sc_export in free_ol_stateid_reaplist() the way
> nfs4_put_stid() does. ->sc_free() runs once per stateid, and a
> stateid reaches free_ol_stateid_reaplist() or nfs4_put_stid() but
> never both, so the reference is dropped exactly once. Revoked
> stateids reach this path with sc_export already cleared by
> drop_stid_export(), so they are skipped rather than double-freed.
> 
> nfs4_put_stid() itself read sc_export before acquiring cl_lock.
> drop_stid_export() clears that field and releases the reference
> under cl_lock, so a concurrent revocation could drop the export in
> the window between the read and the final put, releasing the same
> reference twice. Read sc_export while cl_lock is held so the two
> paths serialize and the reference is released exactly once.
> 
> Fixes: ba0cde5dc81d ("NFSD: Track svc_export in nfs4_stid")
> Reported-by: sashiko-bot <[email protected]>
> Closes: https://sashiko.dev/#/patchset/[email protected]?part=9
> Signed-off-by: Chuck Lever <[email protected]>
> ---
>  fs/nfsd/nfs4state.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 20556b8f186a..e988dfebf75e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1272,9 +1272,9 @@ alloc_init_dir_deleg(struct nfs4_client *clp, struct nfs4_file *fp)
>  void
>  nfs4_put_stid(struct nfs4_stid *s)
>  {
> -	struct svc_export *exp = s->sc_export;
>  	struct nfs4_file *fp = s->sc_file;
>  	struct nfs4_client *clp = s->sc_client;
> +	struct svc_export *exp;
>  
>  	might_lock(&clp->cl_lock);
>  
> @@ -1285,6 +1285,8 @@ nfs4_put_stid(struct nfs4_stid *s)
>  	idr_remove(&clp->cl_stateids, s->sc_stateid.si_opaque.so_id);
>  	if (s->sc_status & SC_STATUS_ADMIN_REVOKED)
>  		atomic_dec(&s->sc_client->cl_admin_revoked);
> +	/* Read under cl_lock to serialize with drop_stid_export(). */
> +	exp = s->sc_export;
>  	nfs4_free_cpntf_statelist(clp->net, s);
>  	spin_unlock(&clp->cl_lock);
>  	s->sc_free(s);
> @@ -1744,6 +1746,7 @@ static void
>  free_ol_stateid_reaplist(struct list_head *reaplist)
>  {
>  	struct nfs4_ol_stateid *stp;
> +	struct svc_export *exp;
>  	struct nfs4_file *fp;
>  
>  	might_sleep();
> @@ -1753,9 +1756,12 @@ free_ol_stateid_reaplist(struct list_head *reaplist)
>  				       st_locks);
>  		list_del(&stp->st_locks);
>  		fp = stp->st_stid.sc_file;
> +		exp = stp->st_stid.sc_export;
>  		stp->st_stid.sc_free(&stp->st_stid);
>  		if (fp)
>  			put_nfs4_file(fp);
> +		if (exp)
> +			exp_put(exp);

nit: nfs4_put_stid() does this in the reverse order. It doesn't
actually matter, but it looks a bit weird if you want to fix it up
before merging.

>  	}
>  }
>  

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