Re: [PATCH 3/3] nfsd: fix UAF in async copy cancel and shutdown

[NeilBrown]

On Thu, 25 Jun 2026, Jeff Layton wrote:
> find_async_copy() bumps copy->refcount under clp->async_lock but
> leaves the copy on clp->async_copies, violating the precondition of
> nfsd4_stop_copy(). The async copy reaper walks clp->async_copies,
> moves OFFLOAD_DONE copies to a private reaplist, then calls
> cleanup_async_copy() outside the lock, racing the cancel path:
> 
>     CPU 0 (OFFLOAD_CANCEL)         CPU 1 (reaper)
>     -----                          -----
>     find_async_copy()
>       refcount_inc(&copy->refcount)
>       /* copy still on async_copies */
>                                    spin_lock(&clp->async_lock)
>                                    move copy to reaplist
>                                    spin_unlock(&clp->async_lock)
>     nfsd4_stop_copy(copy)
>       release_copy_files(copy)
>         nfsd_file_put(nf_src)
>                                    cleanup_async_copy(copy)
>                                      release_copy_files(copy)
>                                        nfsd_file_put(nf_src)
>                                        /* double put -> UAF */
> 
> release_copy_files() reads, puts, then NULLs nf_src/nf_dst, but the
> two callers hold no common lock, so both can observe a non-NULL
> pointer and each call nfsd_file_put() on the same struct nfsd_file.
> That decrements nf_ref twice; nfsd_file_slab is created with
> KMEM_CACHE(nfsd_file, 0) (no SLAB_TYPESAFE_BY_RCU), so the premature
> free leads to a use-after-free of the filecache object.
> 
> Unlinking the copy in find_async_copy() exposes a second hazard.
> nfsd4_stop_copy() only joins via kthread_stop() when the teardown
> caller wins test_and_set_bit(NFSD4_COPY_F_STOPPED). If the kthread
> sets the bit first, kthread_stop() is skipped and the teardown
> caller's nfs4_put_copy() can free copy while the kthread runs on:
> 
>     CPU 0 (teardown)               CPU 1 (copy kthread)
>     -----                          -----
>                                    set_bit(STOPPED, &cp_flags)
>                                    set_bit(COMPLETED, &cp_flags)
>     nfsd4_stop_copy(copy)
>       test_and_set_bit(STOPPED)
>         /* returns 1, skip stop */
>       release_copy_files(copy)
>       nfs4_put_copy(copy) /* 1->0 */
>       kfree(copy)
>                                    nfsd4_send_cb_offload(copy)
>                                    /* UAF */
> 
> In find_async_copy(), after refcount_inc() under async_lock, clear
> copy->cp_clp and list_del_init(&copy->copies), mirroring
> nfsd4_unhash_copy() so the reaper can no longer observe the copy.
> nfsd4_offload_cancel(), nfsd4_shutdown_copy(), and
> nfsd4_cancel_copy_by_sb() then call nfs4_put_copy() after
> nfsd4_stop_copy() to release the list-membership reference the
> reaper previously dropped via cleanup_async_copy(). Because the copy
> is always unlinked before cleanup_async_copy() now runs, drop the
> list_del fixup (and the cp_clp deref) from cleanup_async_copy().
> 
> Give the kthread its own reference: refcount_inc() in nfsd4_copy()
> before wake_up_process(), paired with nfs4_put_copy() as the final
> act of nfsd4_do_async_copy(). Call wake_up_process() before
> list_add() so the publisher has no use of async_copy after
> spin_unlock; a concurrent find_async_copy() + nfsd4_stop_copy() can
> then drain all references safely.
> 
> Pair the cp_clp writers (find_async_copy(), nfsd4_unhash_copy())
> with smp_load_acquire() in nfsd4_send_cb_offload(). set_bit() and
> clear_bit() are unordered atomics per
> Documentation/atomic_bitops.rst and provide no ordering fence, so
> a plain load of cp_clp could be reordered past the subsequent clp
> dereference on weakly-ordered hardware. smp_load_acquire() supplies
> the required barrier.

This patch seems to make two changes.
One involves extra refcount on the "copy" structure, which is probably
sensible but hard for me to follow because the patch is so noisy.

The other is barriers around assignment of ->cp_clp which doesn't make
any sense to me at all, and the copious comments don't help with.
A barrier is always between two things.  Maybe we assign NULL to
something before testing something else, or maybe we test something
before an assignment.  The comments should make clear what the something
else is.  They don't (at least not to me).

I would rather two patches with less noise and more useful comments.

Thanks,
NeilBrown


> 
> Fixes: ac0514f4d198 ("NFSD: Add a laundromat reaper for async copy state")
> Assisted-by: kres:claude-opus-4-8
> Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> ---
>  fs/nfsd/nfs4proc.c | 100 +++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 81 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 669896be08b6..a16a33d0ed00 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1536,11 +1536,10 @@ static void nfsd4_stop_copy(struct nfsd4_copy *copy)
>  	}
>  
>  	/*
> -	 * The copy was removed from async_copies before this function
> -	 * was called, so the reaper cannot clean it up. Release files
> -	 * here regardless of who won the STOPPED race. If the thread
> -	 * set STOPPED, it has finished using the files. If STOPPED
> -	 * was set here, kthread_stop() waited for the thread to exit.
> +	 * Caller has already removed the copy from clp->async_copies, so
> +	 * the reaper cannot reach it. Release files regardless of who won
> +	 * STOPPED; if STOPPED was set here, kthread_stop() joined the
> +	 * kthread.
>  	 */
>  	release_copy_files(copy);
>  	nfs4_put_copy(copy);
> @@ -1555,7 +1554,11 @@ static struct nfsd4_copy *nfsd4_unhash_copy(struct nfs4_client *clp)
>  		copy = list_first_entry(&clp->async_copies, struct nfsd4_copy,
>  					copies);
>  		refcount_inc(&copy->refcount);
> -		copy->cp_clp = NULL;
> +		/*
> +		 * Pairs with smp_load_acquire in nfsd4_send_cb_offload();
> +		 * see find_async_copy() for rationale.
> +		 */
> +		smp_store_release(&copy->cp_clp, NULL);
>  		if (!list_empty(&copy->copies))
>  			list_del_init(&copy->copies);
>  	}
> @@ -1567,8 +1570,16 @@ void nfsd4_shutdown_copy(struct nfs4_client *clp)
>  {
>  	struct nfsd4_copy *copy;
>  
> -	while ((copy = nfsd4_unhash_copy(clp)) != NULL)
> +	while ((copy = nfsd4_unhash_copy(clp)) != NULL) {
>  		nfsd4_stop_copy(copy);
> +		/*
> +		 * nfsd4_unhash_copy() removed the copy from
> +		 * clp->async_copies and cleared cp_clp, so the reaper
> +		 * can no longer reach it and drop the list-membership
> +		 * reference via cleanup_async_copy(). Drop it here.
> +		 */
> +		nfs4_put_copy(copy);
> +	}
>  }
>  
>  static bool nfsd4_copy_on_sb(const struct nfsd4_copy *copy,
> @@ -1637,6 +1648,14 @@ void nfsd4_cancel_copy_by_sb(struct net *net, struct super_block *sb)
>  
>  		list_del_init(&copy->copies);
>  		nfsd4_stop_copy(copy);
> +		/*
> +		 * The copy was moved off clp->async_copies under
> +		 * async_lock above, so the reaper can no longer reach
> +		 * it and drop the list-membership reference via
> +		 * cleanup_async_copy(). Drop it here, mirroring
> +		 * nfsd4_offload_cancel() and nfsd4_shutdown_copy().
> +		 */
> +		nfs4_put_copy(copy);
>  		nfsd4_put_client(clp);
>  	}
>  }
> @@ -2062,28 +2081,38 @@ static void release_copy_files(struct nfsd4_copy *copy)
>  	}
>  }
>  
> +/*
> + * Called from the async copy reaper (after unlinking from async_copies
> + * under async_lock) and from nfsd4_copy()'s out_err path (where the copy
> + * was never list_add'd). In both cases the copy is unreachable from
> + * clp->async_copies.
> + */
>  static void cleanup_async_copy(struct nfsd4_copy *copy)
>  {
>  	nfs4_free_copy_state(copy);
>  	release_copy_files(copy);
> -	if (copy->cp_clp) {
> -		spin_lock(&copy->cp_clp->async_lock);
> -		if (!list_empty(&copy->copies))
> -			list_del_init(&copy->copies);
> -		spin_unlock(&copy->cp_clp->async_lock);
> -	}
>  	nfs4_put_copy(copy);
>  }
>  
>  static void nfsd4_send_cb_offload(struct nfsd4_copy *copy)
>  {
>  	struct nfsd4_cb_offload *cbo = &copy->cp_cb_offload;
> -	struct nfs4_client *clp = copy->cp_clp;
> +	struct nfs4_client *clp;
>  
>  	/*
> -	 * cp_clp is NULL when called via nfsd4_shutdown_copy() during
> -	 * client destruction. Skip the callback; the client is gone.
> +	 * Pairs with smp_store_release(&copy->cp_clp) in find_async_copy()
> +	 * and nfsd4_unhash_copy(). set_bit/clear_bit are unordered atomics
> +	 * (Documentation/atomic_bitops.rst), so the acquire is needed to
> +	 * prevent the cp_clp load being reordered past the clp dereference
> +	 * below on weakly-ordered hardware. The kthread holds its own
> +	 * reference across this call (taken before wake_up_process in
> +	 * nfsd4_copy()); see commit log for per-path client lifetime.
> +	 *
> +	 * cp_clp is NULL when the copy was canceled (find_async_copy,
> +	 * nfsd4_unhash_copy) before the kthread reached this point. Skip
> +	 * the callback; the canceling path owns the notification.
>  	 */
> +	clp = smp_load_acquire(&copy->cp_clp);
>  	if (!clp) {
>  		set_bit(NFSD4_COPY_F_OFFLOAD_DONE, &copy->cp_flags);
>  		return;
> @@ -2160,6 +2189,13 @@ static int nfsd4_do_async_copy(void *data)
>  	if (copy->cp_res.wr_bytes_written > 0 && copy->attr_update)
>  		nfsd_update_cmtime_attr(copy->nf_dst->nf_file, 0);
>  	nfsd4_send_cb_offload(copy);
> +	/*
> +	 * Drop the kthread's own reference (taken before
> +	 * wake_up_process() in nfsd4_copy()). After this point, copy
> +	 * may be freed by a concurrent teardown caller's pending
> +	 * nfs4_put_copy().
> +	 */
> +	nfs4_put_copy(copy);
>  	return 0;
>  }
>  
> @@ -2229,11 +2265,18 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
>  				async_copy, "%s", "copy thread");
>  		if (IS_ERR(async_copy->copy_task))
>  			goto out_dec_async_copy_err;
> +		/*
> +		 * Take the kthread's reference and wake it before publishing
> +		 * on async_copies, so the publisher does not touch async_copy
> +		 * after spin_unlock and a concurrent teardown caller can drain
> +		 * all remaining references safely. See commit log for details.
> +		 */
> +		refcount_inc(&async_copy->refcount);
> +		wake_up_process(async_copy->copy_task);
>  		spin_lock(&async_copy->cp_clp->async_lock);
>  		list_add(&async_copy->copies,
>  				&async_copy->cp_clp->async_copies);
>  		spin_unlock(&async_copy->cp_clp->async_lock);
> -		wake_up_process(async_copy->copy_task);
>  		status = nfs_ok;
>  	} else {
>  		status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> @@ -2287,8 +2330,19 @@ find_async_copy(struct nfs4_client *clp, stateid_t *stateid)
>  
>  	spin_lock(&clp->async_lock);
>  	copy = find_async_copy_locked(clp, stateid);
> -	if (copy)
> +	if (copy) {
>  		refcount_inc(&copy->refcount);
> +		/*
> +		 * Mirror nfsd4_unhash_copy(): unlink and clear cp_clp under
> +		 * async_lock so the reaper cannot reach the copy. Caller drops
> +		 * the list-membership reference via nfs4_put_copy() after
> +		 * nfsd4_stop_copy(). smp_store_release() pairs with
> +		 * smp_load_acquire() in nfsd4_send_cb_offload().
> +		 */
> +		smp_store_release(&copy->cp_clp, NULL);
> +		if (!list_empty(&copy->copies))
> +			list_del_init(&copy->copies);
> +	}
>  	spin_unlock(&clp->async_lock);
>  	return copy;
>  }
> @@ -2307,8 +2361,16 @@ nfsd4_offload_cancel(struct svc_rqst *rqstp,
>  		struct nfsd_net *nn = net_generic(SVC_NET(rqstp), nfsd_net_id);
>  
>  		return manage_cpntf_state(nn, &os->stateid, clp, NULL);
> -	} else
> +	} else {
>  		nfsd4_stop_copy(copy);
> +		/*
> +		 * find_async_copy() removed the copy from
> +		 * clp->async_copies, so the reaper can no longer
> +		 * reach it and drop the list-membership reference
> +		 * via cleanup_async_copy(). Drop it here.
> +		 */
> +		nfs4_put_copy(copy);
> +	}
>  
>  	return nfs_ok;
>  }
> 
> -- 
> 2.54.0
> 
>