Re: [PATCH v2] nfsd: pin source client while using COPY_NOTIFY stateid

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2026-09-30 at 14:39 +0900, Hyunsol Mun wrote:
> A COPY_NOTIFY token can resolve to a stateid owned by the source client.
> find_cpntf_state() drops its source-client reference before returning
> that stateid, so concurrent source-client expiry can free sc_client
> while stateid validation or the caller still uses the stateid.
> 
> Add a local nfsd4_get_client() helper as the counterpart to
> nfsd4_put_client(). Return a paired client reference from
> nfs4_preprocess_stateid_op() when lookup crosses through COPY_NOTIFY
> state. Keep it until the caller releases the returned stateid; internal
> and error paths release it locally.
> 
> An unprivileged NFSv4.2 client with access to the exported file can execute
> the OPEN, COPY_NOTIFY, source-client replacement, and token READ sequence.
> Four hundred natural trials reached the protocol path without reproducing
> the report, so a reliable timing-assistance-free trigger was not
> demonstrated.
> 
> For deterministic validation, a test-only kprobe delayed nfsd_permission()
> after COPY_NOTIFY state lookup. The module only widened the race window; it
> did not allocate, free, or modify an NFSD object. Validation used the
> nfsd-testing base recorded below. On the unmodified KASAN kernel, five
> trials registered four delayed calls and reproduced the source-client
> use-after-free in nfs4_put_stid(). With this patch, five trials registered
> five delayed calls and produced no KASAN report or kernel failure. Three
> token READs returned NFS4_OK with 23 bytes; two returned NFS4ERR_EXPIRED as
> client replacement won the race.
> 
> The full KASAN kernel built with CONFIG_WERROR without a compiler
> diagnostic. Basic NFSv4.2 and NFSv3 read/write/unmount smoke tests passed.
> A source reproducer, timing-module source, complete logs, and the kernel
> configuration are available privately on request.
> 
> The vulnerability research and validation were conducted by members of
> the Tobabz team as part of the Best of the Best 15th program.
> 
> Fixes: 624322f1adc5 ("NFSD add COPY_NOTIFY operation")
> Assisted-by: LLM
> Signed-off-by: Hyunsol Mun <muumthf@gmail.com>
> ---
> Changes in v2:
> - Describe current timing-assisted baseline and patched results.
> - Add full-build and NFSv4.2/NFSv3 smoke-test results.
> - Keep nfsd4_get_client() local to nfs4state.c.
> 
>  fs/nfsd/nfs4proc.c  | 31 +++++++++++++++++++++----------
>  fs/nfsd/nfs4state.c | 34 +++++++++++++++++++++++++++-------
>  fs/nfsd/state.h     |  2 +-
>  3 files changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 7df60abfb..d5b5f59f1 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1144,7 +1144,7 @@ nfsd4_read(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	/* check stateid */
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>  					&read->rd_stateid, RD_STATE,
> -					&read->rd_nf, NULL);
> +					&read->rd_nf, NULL, NULL);
>  
>  	read->rd_rqstp = rqstp;
>  	read->rd_fhp = &cstate->current_fh;
> @@ -1342,6 +1342,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		.na_dpacl	= posix_acl_dup(setattr->sa_dpacl),
>  	};
>  	bool save_no_wcc, deleg_attrs;
> +	struct nfs4_client *stid_clp = NULL;
>  	struct nfs4_stid *st = NULL;
>  	struct inode *inode;
>  	__be32 status = nfs_ok;
> @@ -1358,7 +1359,7 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  		status = nfs4_preprocess_stateid_op(rqstp, cstate,
>  				&cstate->current_fh, &setattr->sa_stateid,
> -				flags, NULL, &st);
> +				flags, NULL, &st, &stid_clp);
>  		if (status)
>  			goto out_err;
>  	}
> @@ -1375,8 +1376,11 @@ nfsd4_setattr(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  			}
>  		}
>  	}
> -	if (st)
> +	if (st) {
>  		nfs4_put_stid(st);
> +		if (stid_clp)
> +			nfsd4_put_client(stid_clp);
> +	}
>  	if (status)
>  		goto out_err;
>  
> @@ -1439,6 +1443,7 @@ nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	struct nfsd4_write *write = &u->write;
>  	stateid_t *stateid = &write->wr_stateid;
>  	struct nfs4_stid *stid = NULL;
> +	struct nfs4_client *stid_clp = NULL;
>  	struct nfsd_file *nf = NULL;
>  	__be32 status = nfs_ok;
>  	unsigned long cnt;
> @@ -1451,13 +1456,16 @@ nfsd4_write(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	trace_nfsd_write_start(rqstp, &cstate->current_fh,
>  			       write->wr_offset, cnt);
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> -						stateid, WR_STATE, &nf, &stid);
> +						stateid, WR_STATE, &nf, &stid,
> +						&stid_clp);
>  	if (status)
>  		return status;
>  
>  	if (stid) {
>  		nfsd4_file_mark_deleg_written(stid->sc_file);
>  		nfs4_put_stid(stid);
> +		if (stid_clp)
> +			nfsd4_put_client(stid_clp);
>  	}
>  
>  	write->wr_how_written = write->wr_stable_how;
> @@ -1484,12 +1492,12 @@ nfsd4_verify_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  		return nfserr_nofilehandle;
>  
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->save_fh,
> -					    src_stateid, RD_STATE, src, NULL);
> +					    src_stateid, RD_STATE, src, NULL, NULL);
>  	if (status)
>  		goto out;
>  
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
> -					    dst_stateid, WR_STATE, dst, NULL);
> +					    dst_stateid, WR_STATE, dst, NULL, NULL);
>  	if (status)
>  		goto out_put_src;
>  
> @@ -1954,7 +1962,7 @@ nfsd4_setup_inter_ssc(struct svc_rqst *rqstp,
>  	/* Verify the destination stateid and set dst struct file*/
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>  					    &copy->cp_dst_stateid,
> -					    WR_STATE, &copy->nf_dst, NULL);
> +					    WR_STATE, &copy->nf_dst, NULL, NULL);
>  	if (status)
>  		goto out;
>  
> @@ -2498,12 +2506,13 @@ nfsd4_copy_notify(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	struct nfsd4_copy_notify *cn = &u->copy_notify;
>  	__be32 status;
>  	struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
> +	struct nfs4_client *stid_clp = NULL;
>  	struct nfs4_stid *stid = NULL;
>  	struct nfs4_cpntf_state *cps;
>  
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>  					&cn->cpn_src_stateid, RD_STATE, NULL,
> -					&stid);
> +					&stid, &stid_clp);
>  	if (status)
>  		return status;
>  	if (!stid)
> @@ -2536,6 +2545,8 @@ nfsd4_copy_notify(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  	nfs4_put_cpntf_state(nn, cps);
>  out:
>  	nfs4_put_stid(stid);
> +	if (stid_clp)
> +		nfsd4_put_client(stid_clp);
>  	return status;
>  }
>  
> @@ -2548,7 +2559,7 @@ nfsd4_fallocate(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>  					    &fallocate->falloc_stateid,
> -					    WR_STATE, &nf, NULL);
> +					    WR_STATE, &nf, NULL, NULL);
>  	if (status != nfs_ok)
>  		return status;
>  
> @@ -2612,7 +2623,7 @@ nfsd4_seek(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  
>  	status = nfs4_preprocess_stateid_op(rqstp, cstate, &cstate->current_fh,
>  					    &seek->seek_stateid,
> -					    RD_STATE, &nf, NULL);
> +					    RD_STATE, &nf, NULL, NULL);
>  	if (status)
>  		return status;
>  
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index bbc16dd22..43906f87e 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -2805,6 +2805,15 @@ static void __free_client(struct kref *k)
>  	kmem_cache_free(client_slab, clp);
>  }
>  
> +/**
> + * nfsd4_get_client - acquire a reference on an nfs4_client
> + * @clp: the client to be acquired
> + */
> +static void nfsd4_get_client(struct nfs4_client *clp)
> +{
> +	kref_get(&clp->cl_nfsdfs.cl_ref);
> +}
> +

If you're going to add this helper, then you should convert all of the
other places that call kref_get() on this object to use it, ideally in
a separate patch.

>  /**
>   * nfsd4_put_client - release a reference on an nfs4_client
>   * @clp: the client to be released
> @@ -8504,7 +8513,8 @@ __be32 manage_cpntf_state(struct nfsd_net *nn, stateid_t *st,
>  }
>  
>  static __be32 find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
> -			       struct nfs4_stid **stid)
> +			       struct nfs4_stid **stid,
> +			       struct nfs4_client **stid_clp)
>  {
>  	__be32 status;
>  	struct nfs4_cpntf_state *cps = NULL;
> @@ -8524,10 +8534,13 @@ static __be32 find_cpntf_state(struct nfsd_net *nn, stateid_t *st,
>  	*stid = find_stateid_by_type(found, &cps->cp_p_stateid,
>  				     SC_TYPE_DELEG|SC_TYPE_OPEN|SC_TYPE_LOCK,
>  				     0);
> -	if (*stid)
> +	if (*stid) {
> +		nfsd4_get_client(found);
> +		*stid_clp = found;
>  		status = nfs_ok;
> -	else
> +	} else {
>  		status = nfserr_bad_stateid;
> +	}
>  
>  	put_client_renew(found);
>  out:
> @@ -8551,6 +8564,7 @@ void nfs4_put_cpntf_state(struct nfsd_net *nn, struct nfs4_cpntf_state *cps)
>   * @flags: flags describing type of operation to be done
>   * @nfp: optional nfsd_file return pointer (may be NULL)
>   * @cstid: optional returned nfs4_stid pointer (may be NULL)
> + * @cstid_clp: client reference paired with @cstid (required with @cstid)
>   *
>   * Given info from the client, look up a nfs4_stid for the operation. On
>   * success, it returns a reference to the nfs4_stid and/or the nfsd_file
> @@ -8560,10 +8574,11 @@ __be32
>  nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>  		struct nfsd4_compound_state *cstate, struct svc_fh *fhp,
>  		stateid_t *stateid, int flags, struct nfsd_file **nfp,
> -		struct nfs4_stid **cstid)
> +		struct nfs4_stid **cstid, struct nfs4_client **cstid_clp)

The bug seems possible (esp given the fault-injected reproducer), but
this function is really devolving into a big mess. It would be very
nice to move the COPY-specific code into a dedicated helper that is
only called from the COPY-related codepaths. Consider reorganizing this
code along those lines.

>  {
>  	struct net *net = SVC_NET(rqstp);
>  	struct nfsd_net *nn = net_generic(net, nfsd_net_id);
> +	struct nfs4_client *stid_clp = NULL;
>  	struct nfs4_stid *s = NULL;
>  	__be32 status;
>  
> @@ -8579,7 +8594,7 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>  				SC_TYPE_DELEG|SC_TYPE_OPEN|SC_TYPE_LOCK,
>  				0, &s, nn);
>  	if (status == nfserr_bad_stateid)
> -		status = find_cpntf_state(nn, stateid, &s);
> +		status = find_cpntf_state(nn, stateid, &s, &stid_clp);
>  	if (status)
>  		return status;
>  	status = nfsd4_stid_check_stateid_generation(stateid, s,
> @@ -8605,11 +8620,16 @@ nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>  		status = nfs4_check_file(rqstp, fhp, s, nfp, flags);
>  out:
>  	if (s) {
> -		if (!status && cstid)
> +		if (!status && cstid) {
>  			*cstid = s;
> -		else
> +			*cstid_clp = stid_clp;
> +			stid_clp = NULL;
> +		} else {
>  			nfs4_put_stid(s);
> +		}
>  	}
> +	if (stid_clp)
> +		nfsd4_put_client(stid_clp);
>  	return status;
>  }
>  
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index 1a0da82cb..7910dabb4 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -911,7 +911,7 @@ struct nfsd4_async_copy;
>  extern __be32 nfs4_preprocess_stateid_op(struct svc_rqst *rqstp,
>  		struct nfsd4_compound_state *cstate, struct svc_fh *fhp,
>  		stateid_t *stateid, int flags, struct nfsd_file **filp,
> -		struct nfs4_stid **cstid);
> +		struct nfs4_stid **cstid, struct nfs4_client **cstid_clp);
>  __be32 nfsd4_lookup_stateid(struct nfsd4_compound_state *cstate,
>  			    stateid_t *stateid, unsigned short typemask,
>  			    unsigned short statusmask,
> 
> base-commit: 32eb1a60b456980761cf7a9cee8f907fdc08afb8

-- 
Jeff Layton <jlayton@kernel.org>