[PATCH 0/3] nfsd: inter-server copy fixes

[PATCH 0/3] nfsd: inter-server copy fixes

[Jeff Layton]

These patches fix bugs in inter-server copy offload code noticed by LLM
inspection.

Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
Chris Mason (2):
      nfsd: fix cpntf publish race in nfs4_init_cp_state
      nfsd: fix UAF in cpntf statelist drain

Jeff Layton (1):
      nfsd: fix UAF in async copy cancel and shutdown

 fs/nfsd/nfs4proc.c  | 100 +++++++++++++++++++++++++++++++++++++++----------
 fs/nfsd/nfs4state.c | 105 +++++++++++++++++++++++++++++++++++++++++-----------
 2 files changed, 164 insertions(+), 41 deletions(-)
---
base-commit: 2bb83225da8ee0383d17783b5c903589696faf90
change-id: 20260624-nfsd-testing-8439f0163312

Best regards,
-- 
Jeff Layton <jlayton@kernel.org>

[PATCH 1/3] nfsd: fix cpntf publish race in nfs4_init_cp_state

[Jeff Layton]

From: Chris Mason <clm@meta.com>

nfs4_alloc_init_cpntf_state() splits IDR publication and cp_list
linkage across two separate s2s_cp_lock critical sections. The first
installs the new nfs4_cpntf_state into nn->s2s_cp_stateids and then
assigns cs_type = NFS4_COPYNOTIFY_STID; the second acquires the lock
again to list_add() the entry onto p_stid->sc_cp_list. Between the
two, the entry is reachable by so_id with the correct cs_type, but
cp_list is still {NULL, NULL} from kzalloc.

A second NFSv4.2 request can race in that gap:

    CPU 0                              CPU 1
    -----                              -----
    nfs4_alloc_init_cpntf_state()
      nfs4_init_cp_state()
        spin_lock(&s2s_cp_lock)
        idr_alloc_cyclic()
        spin_unlock(&s2s_cp_lock)
      cps->cp_stateid.cs_type =
          NFS4_COPYNOTIFY_STID
                                       nfsd4_offload_cancel()
                                         find_async_copy() -> NULL
                                         manage_cpntf_state(clp!=NULL)
                                           spin_lock(&s2s_cp_lock)
                                           idr_find() -> cps
                                           cs_type check passes
                                           _free_cpntf_state_locked()
                                             refcount_dec_and_test
                                             list_del(&cps->cp_list)
      spin_lock(&s2s_cp_lock)             /* {NULL,NULL} -> oops */
      list_add(&cps->cp_list, ...)

The so_id returned to the client by COPY_NOTIFY is echoed back as
cnr_stateid, so any authenticated NFSv4.2 client can drive
OFFLOAD_CANCEL into manage_cpntf_state() against an entry still in
the window. list_del() on a zeroed list_head dereferences NULL and
oopses the server.

Fold cs_type assignment and the list_add() onto p_stid->sc_cp_list
into the same s2s_cp_lock critical section that runs idr_alloc_cyclic.
A concurrent manage_cpntf_state() now either fails idr_find() (entry
not yet visible) or observes a fully linked cp_list. Initialize
cp_list with INIT_LIST_HEAD() after allocation, and switch
_free_cpntf_state_locked() to list_del_init() so that any stale
unlink on an already-unlinked entry is a benign no-op rather than a
NULL dereference. nfs4_init_copy_state() passes NULL for p_stid and
skips the list_add branch, preserving NFS4_COPY_STID semantics.

Fixes: 624322f1adc5 ("NFSD add COPY_NOTIFY operation")
Assisted-by: kres:claude-opus-4-7
Signed-off-by: Chris Mason <clm@meta.com>
---
 fs/nfsd/nfs4state.c | 42 ++++++++++++++++++++++++++++++++----------
 1 file changed, 32 insertions(+), 10 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index a4398dc861a5..b8946db3ebaa 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -944,7 +944,7 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
  * Create a unique stateid_t to represent each COPY.
  */
 static int nfs4_init_cp_state(struct nfsd_net *nn, copy_stateid_t *stid,
-			      unsigned char cs_type)
+			      unsigned char cs_type, struct nfs4_stid *p_stid)
 {
 	int new_id;
 
@@ -954,19 +954,37 @@ static int nfs4_init_cp_state(struct nfsd_net *nn, copy_stateid_t *stid,
 	idr_preload(GFP_KERNEL);
 	spin_lock(&nn->s2s_cp_lock);
 	new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, stid, 0, 0, GFP_NOWAIT);
-	stid->cs_stid.si_opaque.so_id = new_id;
-	stid->cs_stid.si_generation = 1;
+	if (new_id >= 0) {
+		stid->cs_stid.si_opaque.so_id = new_id;
+		stid->cs_stid.si_generation = 1;
+		/*
+		 * Publish cs_type and link onto the parent stid's
+		 * sc_cp_list inside the same critical section that
+		 * installed the entry into nn->s2s_cp_stateids. A
+		 * concurrent manage_cpntf_state() either fails the
+		 * idr_find() (entry not yet visible) or observes a
+		 * fully linked cp_list, so list_del_init() in
+		 * _free_cpntf_state_locked() is always well-defined.
+		 */
+		stid->cs_type = cs_type;
+		if (p_stid) {
+			struct nfs4_cpntf_state *cps =
+				container_of(stid, struct nfs4_cpntf_state,
+					     cp_stateid);
+
+			list_add(&cps->cp_list, &p_stid->sc_cp_list);
+		}
+	}
 	spin_unlock(&nn->s2s_cp_lock);
 	idr_preload_end();
 	if (new_id < 0)
 		return 0;
-	stid->cs_type = cs_type;
 	return 1;
 }
 
 int nfs4_init_copy_state(struct nfsd_net *nn, struct nfsd4_copy *copy)
 {
-	return nfs4_init_cp_state(nn, &copy->cp_stateid, NFS4_COPY_STID);
+	return nfs4_init_cp_state(nn, &copy->cp_stateid, NFS4_COPY_STID, NULL);
 }
 
 struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
@@ -977,13 +995,17 @@ struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
 	cps = kzalloc_obj(struct nfs4_cpntf_state);
 	if (!cps)
 		return NULL;
+	/*
+	 * Initialize cp_list so any stale unlink (e.g. on an
+	 * entry that never reached its parent's sc_cp_list)
+	 * degrades to a benign self-unlink via list_del_init().
+	 */
+	INIT_LIST_HEAD(&cps->cp_list);
 	cps->cpntf_time = ktime_get_boottime_seconds();
 	refcount_set(&cps->cp_stateid.cs_count, 1);
-	if (!nfs4_init_cp_state(nn, &cps->cp_stateid, NFS4_COPYNOTIFY_STID))
+	if (!nfs4_init_cp_state(nn, &cps->cp_stateid, NFS4_COPYNOTIFY_STID,
+				p_stid))
 		goto out_free;
-	spin_lock(&nn->s2s_cp_lock);
-	list_add(&cps->cp_list, &p_stid->sc_cp_list);
-	spin_unlock(&nn->s2s_cp_lock);
 	return cps;
 out_free:
 	kfree(cps);
@@ -7854,7 +7876,7 @@ _free_cpntf_state_locked(struct nfsd_net *nn, struct nfs4_cpntf_state *cps)
 	WARN_ON_ONCE(cps->cp_stateid.cs_type != NFS4_COPYNOTIFY_STID);
 	if (!refcount_dec_and_test(&cps->cp_stateid.cs_count))
 		return;
-	list_del(&cps->cp_list);
+	list_del_init(&cps->cp_list);
 	idr_remove(&nn->s2s_cp_stateids,
 		   cps->cp_stateid.cs_stid.si_opaque.so_id);
 	kfree(cps);

-- 
2.54.0

Re: [PATCH 1/3] nfsd: fix cpntf publish race in nfs4_init_cp_state

[NeilBrown]

On Thu, 25 Jun 2026, Jeff Layton wrote:
> From: Chris Mason <clm@meta.com>
> 
> nfs4_alloc_init_cpntf_state() splits IDR publication and cp_list
> linkage across two separate s2s_cp_lock critical sections. The first
> installs the new nfs4_cpntf_state into nn->s2s_cp_stateids and then
> assigns cs_type = NFS4_COPYNOTIFY_STID; the second acquires the lock
> again to list_add() the entry onto p_stid->sc_cp_list. Between the
> two, the entry is reachable by so_id with the correct cs_type, but
> cp_list is still {NULL, NULL} from kzalloc.
> 
> A second NFSv4.2 request can race in that gap:
> 
>     CPU 0                              CPU 1
>     -----                              -----
>     nfs4_alloc_init_cpntf_state()
>       nfs4_init_cp_state()
>         spin_lock(&s2s_cp_lock)
>         idr_alloc_cyclic()
>         spin_unlock(&s2s_cp_lock)
>       cps->cp_stateid.cs_type =
>           NFS4_COPYNOTIFY_STID
>                                        nfsd4_offload_cancel()
>                                          find_async_copy() -> NULL
>                                          manage_cpntf_state(clp!=NULL)
>                                            spin_lock(&s2s_cp_lock)
>                                            idr_find() -> cps
>                                            cs_type check passes
>                                            _free_cpntf_state_locked()
>                                              refcount_dec_and_test
>                                              list_del(&cps->cp_list)
>       spin_lock(&s2s_cp_lock)             /* {NULL,NULL} -> oops */
>       list_add(&cps->cp_list, ...)
> 
> The so_id returned to the client by COPY_NOTIFY is echoed back as
> cnr_stateid, so any authenticated NFSv4.2 client can drive
> OFFLOAD_CANCEL into manage_cpntf_state() against an entry still in
> the window. list_del() on a zeroed list_head dereferences NULL and
> oopses the server.
> 
> Fold cs_type assignment and the list_add() onto p_stid->sc_cp_list
> into the same s2s_cp_lock critical section that runs idr_alloc_cyclic.
> A concurrent manage_cpntf_state() now either fails idr_find() (entry
> not yet visible) or observes a fully linked cp_list. Initialize
> cp_list with INIT_LIST_HEAD() after allocation, and switch
> _free_cpntf_state_locked() to list_del_init() so that any stale
> unlink on an already-unlinked entry is a benign no-op rather than a
> NULL dereference. nfs4_init_copy_state() passes NULL for p_stid and
> skips the list_add branch, preserving NFS4_COPY_STID semantics.
> 
> Fixes: 624322f1adc5 ("NFSD add COPY_NOTIFY operation")
> Assisted-by: kres:claude-opus-4-7
> Signed-off-by: Chris Mason <clm@meta.com>
> ---
>  fs/nfsd/nfs4state.c | 42 ++++++++++++++++++++++++++++++++----------
>  1 file changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index a4398dc861a5..b8946db3ebaa 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -944,7 +944,7 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
>   * Create a unique stateid_t to represent each COPY.
>   */
>  static int nfs4_init_cp_state(struct nfsd_net *nn, copy_stateid_t *stid,
> -			      unsigned char cs_type)
> +			      unsigned char cs_type, struct nfs4_stid *p_stid)
>  {
>  	int new_id;
>  
> @@ -954,19 +954,37 @@ static int nfs4_init_cp_state(struct nfsd_net *nn, copy_stateid_t *stid,
>  	idr_preload(GFP_KERNEL);
>  	spin_lock(&nn->s2s_cp_lock);
>  	new_id = idr_alloc_cyclic(&nn->s2s_cp_stateids, stid, 0, 0, GFP_NOWAIT);
> -	stid->cs_stid.si_opaque.so_id = new_id;
> -	stid->cs_stid.si_generation = 1;
> +	if (new_id >= 0) {
> +		stid->cs_stid.si_opaque.so_id = new_id;
> +		stid->cs_stid.si_generation = 1;
> +		/*
> +		 * Publish cs_type and link onto the parent stid's
> +		 * sc_cp_list inside the same critical section that
> +		 * installed the entry into nn->s2s_cp_stateids. A
> +		 * concurrent manage_cpntf_state() either fails the
> +		 * idr_find() (entry not yet visible) or observes a
> +		 * fully linked cp_list, so list_del_init() in
> +		 * _free_cpntf_state_locked() is always well-defined.
> +		 */

This comment seems excessive.  The explanation certainly should be in
the commit message, and is probably there already.  It doesn't need to
be here too.

> +		stid->cs_type = cs_type;
> +		if (p_stid) {

I would rather this switched on "cs_type == NFS4_COPYNOTIFY_STID"

A substantially simpler solution would be to *not* pass cs_type to
nfs4_init_cp_state() so the type remains initialised to zero, which is
invalid.
Anything which finds that stateid will ignore it because they already
check the type.
Then callers of nfs4_init_cp_state() set the type themselves.

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index f4d12dbcf97b..7a8010f450ea 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -943,8 +943,7 @@ struct nfs4_stid *nfs4_alloc_stid(struct nfs4_client *cl, struct kmem_cache *sla
 /*
  * Create a unique stateid_t to represent each COPY.
  */
-static int nfs4_init_cp_state(struct nfsd_net *nn, copy_stateid_t *stid,
-			      unsigned char cs_type)
+static int nfs4_init_cp_state(struct nfsd_net *nn, copy_stateid_t *stid)
 {
 	int new_id;
 
@@ -960,13 +959,16 @@ static int nfs4_init_cp_state(struct nfsd_net *nn, copy_stateid_t *stid,
 	idr_preload_end();
 	if (new_id < 0)
 		return 0;
-	stid->cs_type = cs_type;
 	return 1;
 }
 
 int nfs4_init_copy_state(struct nfsd_net *nn, struct nfsd4_copy *copy)
 {
-	return nfs4_init_cp_state(nn, &copy->cp_stateid, NFS4_COPY_STID);
+	if (nfs4_init_cp_state(nn, &copy->cp_stateid)){
+		stid->cs_type = NFS4_COPY_STID;
+		return 1;
+	}
+	return 0;
 }
 
 struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
@@ -979,10 +981,11 @@ struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
 		return NULL;
 	cps->cpntf_time = ktime_get_boottime_seconds();
 	refcount_set(&cps->cp_stateid.cs_count, 1);
-	if (!nfs4_init_cp_state(nn, &cps->cp_stateid, NFS4_COPYNOTIFY_STID))
+	if (!nfs4_init_cp_state(nn, &cps->cp_stateid))
 		goto out_free;
 	spin_lock(&nn->s2s_cp_lock);
 	list_add(&cps->cp_list, &p_stid->sc_cp_list);
+	stid->cs_type = NFS4_COPYNOTIFY_STID;
 	spin_unlock(&nn->s2s_cp_lock);
 	return cps;
 out_free:


Maybe not a "Better" solution, I'm not sure.  But certainly "simpler".
NeilBrown



> +			struct nfs4_cpntf_state *cps =
> +				container_of(stid, struct nfs4_cpntf_state,
> +					     cp_stateid);
> +
> +			list_add(&cps->cp_list, &p_stid->sc_cp_list);
> +		}
> +	}
>  	spin_unlock(&nn->s2s_cp_lock);
>  	idr_preload_end();
>  	if (new_id < 0)
>  		return 0;
> -	stid->cs_type = cs_type;
>  	return 1;
>  }
>  
>  int nfs4_init_copy_state(struct nfsd_net *nn, struct nfsd4_copy *copy)
>  {
> -	return nfs4_init_cp_state(nn, &copy->cp_stateid, NFS4_COPY_STID);
> +	return nfs4_init_cp_state(nn, &copy->cp_stateid, NFS4_COPY_STID, NULL);
>  }
>  
>  struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
> @@ -977,13 +995,17 @@ struct nfs4_cpntf_state *nfs4_alloc_init_cpntf_state(struct nfsd_net *nn,
>  	cps = kzalloc_obj(struct nfs4_cpntf_state);
>  	if (!cps)
>  		return NULL;
> +	/*
> +	 * Initialize cp_list so any stale unlink (e.g. on an
> +	 * entry that never reached its parent's sc_cp_list)
> +	 * degrades to a benign self-unlink via list_del_init().
> +	 */
> +	INIT_LIST_HEAD(&cps->cp_list);
>  	cps->cpntf_time = ktime_get_boottime_seconds();
>  	refcount_set(&cps->cp_stateid.cs_count, 1);
> -	if (!nfs4_init_cp_state(nn, &cps->cp_stateid, NFS4_COPYNOTIFY_STID))
> +	if (!nfs4_init_cp_state(nn, &cps->cp_stateid, NFS4_COPYNOTIFY_STID,
> +				p_stid))
>  		goto out_free;
> -	spin_lock(&nn->s2s_cp_lock);
> -	list_add(&cps->cp_list, &p_stid->sc_cp_list);
> -	spin_unlock(&nn->s2s_cp_lock);
>  	return cps;
>  out_free:
>  	kfree(cps);
> @@ -7854,7 +7876,7 @@ _free_cpntf_state_locked(struct nfsd_net *nn, struct nfs4_cpntf_state *cps)
>  	WARN_ON_ONCE(cps->cp_stateid.cs_type != NFS4_COPYNOTIFY_STID);
>  	if (!refcount_dec_and_test(&cps->cp_stateid.cs_count))
>  		return;
> -	list_del(&cps->cp_list);
> +	list_del_init(&cps->cp_list);
>  	idr_remove(&nn->s2s_cp_stateids,
>  		   cps->cp_stateid.cs_stid.si_opaque.so_id);
>  	kfree(cps);
> 
> -- 
> 2.54.0
> 
> 

[PATCH 2/3] nfsd: fix UAF in cpntf statelist drain

[Jeff Layton]

From: Chris Mason <clm@meta.com>

nfs4_free_cpntf_statelist() drains a parent stid's sc_cp_list by
repeatedly taking the first entry and calling
_free_cpntf_state_locked(), which only unlinks and frees the entry
when refcount_dec_and_test() drops cs_count to zero.  When a
concurrent holder has bumped cs_count via manage_cpntf_state(),
the helper returns early and leaves the entry on sc_cp_list, so
the drain re-decrements the same entry on the next iteration and
burns the reference that belonged to that concurrent holder.

    CPU 0                              CPU 1
    -----                              -----
    find_cpntf_state()
      manage_cpntf_state(clp=NULL)
        spin_lock(&nn->s2s_cp_lock)
        refcount_inc(cs_count) 1->2
        spin_unlock(&nn->s2s_cp_lock)
      /* caller now owns a ref */
                                       nfs4_free_cpntf_statelist()
                                         spin_lock(&s2s_cp_lock)
                                         iter 1: dec_and_test 2->1
                                                 (no unlink/free)
                                         iter 2: dec_and_test 1->0
                                                 list_del + idr_remove
                                                 kfree(cps)
                                         spin_unlock(&s2s_cp_lock)
    nfs4_put_cpntf_state(cps)
      spin_lock(&s2s_cp_lock)
      _free_cpntf_state_locked(cps)    /* cps is freed slab */

The late put writes into the freed slab object's refcount_t, a
KASAN-detectable use-after-free.

Fix by rewriting the drain to unconditionally revoke each entry
before dropping the parent's reference.  Walk sc_cp_list with
list_for_each_entry_safe(), list_del_init() the entry, remove it
from nn->s2s_cp_stateids, and only then drop the parent-owned
reference via a new put_cpntf_state_unlinked_locked() helper
that does refcount_dec_and_test() + kfree().  The drain now
terminates in one pass per entry regardless of cs_count.

Concurrent holders keep their own reference; their eventual
nfs4_put_cpntf_state() reaches _free_cpntf_state_locked() on an
entry the drain has already unlinked from sc_cp_list and from
nn->s2s_cp_stateids.  _free_cpntf_state_locked() gates its
list_del_init() and idr_remove() on !list_empty(&cps->cp_list);
the drain's list_del_init() leaves cp_list self-linked, so the
late put skips both calls and only the final refcount drop runs.

Gating idr_remove() on the same check is required because
idr_alloc_cyclic() may already have recycled the so_id for an
unrelated cpntf entry by the time the late put runs; an
unconditional idr_remove() would silently unhash that live entry
and corrupt subsequent find_cpntf_state() lookups.

Fixes: 624322f1adc5 ("NFSD add COPY_NOTIFY operation")
Assisted-by: kres:claude-opus-4-7
Signed-off-by: Chris Mason <clm@meta.com>
---
 fs/nfsd/nfs4state.c | 65 ++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 52 insertions(+), 13 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b8946db3ebaa..374155e57f3f 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1025,17 +1025,58 @@ void nfs4_free_copy_state(struct nfsd4_copy *copy)
 	spin_unlock(&nn->s2s_cp_lock);
 }
 
+/*
+ * Drop the parent stid's reference on a cpntf entry that has already been
+ * removed from sc_cp_list and the s2s_cp_stateids IDR. If a concurrent holder
+ * still owns a reference (acquired viamanage_cpntf_state() before the unlink),
+ * that holder's nfs4_put_cpntf_state() will perform the final free.
+ *
+ * The nn->s2s_cp_lock must be held!
+ */
+static void put_cpntf_state_unlinked_locked(struct nfs4_cpntf_state *cps)
+{
+	WARN_ON_ONCE(cps->cp_stateid.cs_type != NFS4_COPYNOTIFY_STID);
+	WARN_ON_ONCE(!list_empty(&cps->cp_list));
+
+	if (refcount_dec_and_test(&cps->cp_stateid.cs_count))
+		kfree(cps);
+}
+
+/*
+ * Unhash the stateid from the s2s stateid hash, and detach it from the sc_cp_list.
+ * Note that this is gated on a list_empty() check, to avoid problems with IDR
+ * hashval reuse.
+ */
+static void nfsd4_unhash_cpntf_state(struct nfsd_net *nn, struct nfs4_cpntf_state *cps)
+{
+	lockdep_assert_held(&nn->s2s_cp_lock);
+
+	if (!list_empty(&cps->cp_list)) {
+		list_del_init(&cps->cp_list);
+		idr_remove(&nn->s2s_cp_stateids, cps->cp_stateid.cs_stid.si_opaque.so_id);
+	}
+}
+
 static void nfs4_free_cpntf_statelist(struct net *net, struct nfs4_stid *stid)
 {
-	struct nfs4_cpntf_state *cps;
+	struct nfs4_cpntf_state *cps, *tmp;
 	struct nfsd_net *nn;
 
 	nn = net_generic(net, nfsd_net_id);
 	spin_lock(&nn->s2s_cp_lock);
-	while (!list_empty(&stid->sc_cp_list)) {
-		cps = list_first_entry(&stid->sc_cp_list,
-				       struct nfs4_cpntf_state, cp_list);
-		_free_cpntf_state_locked(nn, cps);
+	/*
+	 * Unlink every entry from sc_cp_list and the IDR before dropping
+	 * the parent's reference.  This makes the drain terminate in one
+	 * pass per entry regardless of cs_count: a concurrent holder that
+	 * obtained the entry via manage_cpntf_state() retains its own
+	 * reference, and its eventual nfs4_put_cpntf_state() will see the
+	 * entry already unlinked (list_del_init() in
+	 * _free_cpntf_state_locked makes that a no-op) and drive the final
+	 * kfree itself.
+	 */
+	list_for_each_entry_safe(cps, tmp, &stid->sc_cp_list, cp_list) {
+		nfsd4_unhash_cpntf_state(nn, cps);
+		put_cpntf_state_unlinked_locked(cps);
 	}
 	spin_unlock(&nn->s2s_cp_lock);
 }
@@ -7870,16 +7911,14 @@ nfs4_check_file(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfs4_stid *s,
 out:
 	return status;
 }
-static void
-_free_cpntf_state_locked(struct nfsd_net *nn, struct nfs4_cpntf_state *cps)
+
+static void _free_cpntf_state_locked(struct nfsd_net *nn, struct nfs4_cpntf_state *cps)
 {
 	WARN_ON_ONCE(cps->cp_stateid.cs_type != NFS4_COPYNOTIFY_STID);
-	if (!refcount_dec_and_test(&cps->cp_stateid.cs_count))
-		return;
-	list_del_init(&cps->cp_list);
-	idr_remove(&nn->s2s_cp_stateids,
-		   cps->cp_stateid.cs_stid.si_opaque.so_id);
-	kfree(cps);
+	if (refcount_dec_and_test(&cps->cp_stateid.cs_count)) {
+		nfsd4_unhash_cpntf_state(nn, cps);
+		kfree(cps);
+	}
 }
 /*
  * A READ from an inter server to server COPY will have a

-- 
2.54.0

Re: [PATCH 2/3] nfsd: fix UAF in cpntf statelist drain

[NeilBrown]

On Thu, 25 Jun 2026, Jeff Layton wrote:
> From: Chris Mason <clm@meta.com>
> 
> nfs4_free_cpntf_statelist() drains a parent stid's sc_cp_list by
> repeatedly taking the first entry and calling
> _free_cpntf_state_locked(), which only unlinks and frees the entry
> when refcount_dec_and_test() drops cs_count to zero.  When a
> concurrent holder has bumped cs_count via manage_cpntf_state(),
> the helper returns early and leaves the entry on sc_cp_list, so
> the drain re-decrements the same entry on the next iteration and
> burns the reference that belonged to that concurrent holder.
> 
>     CPU 0                              CPU 1
>     -----                              -----
>     find_cpntf_state()
>       manage_cpntf_state(clp=NULL)
>         spin_lock(&nn->s2s_cp_lock)
>         refcount_inc(cs_count) 1->2
>         spin_unlock(&nn->s2s_cp_lock)
>       /* caller now owns a ref */
>                                        nfs4_free_cpntf_statelist()
>                                          spin_lock(&s2s_cp_lock)
>                                          iter 1: dec_and_test 2->1
>                                                  (no unlink/free)
>                                          iter 2: dec_and_test 1->0
>                                                  list_del + idr_remove
>                                                  kfree(cps)
>                                          spin_unlock(&s2s_cp_lock)
>     nfs4_put_cpntf_state(cps)
>       spin_lock(&s2s_cp_lock)
>       _free_cpntf_state_locked(cps)    /* cps is freed slab */
> 
> The late put writes into the freed slab object's refcount_t, a
> KASAN-detectable use-after-free.
> 
> Fix by rewriting the drain to unconditionally revoke each entry
> before dropping the parent's reference.  Walk sc_cp_list with
> list_for_each_entry_safe(), list_del_init() the entry, remove it
> from nn->s2s_cp_stateids, and only then drop the parent-owned
> reference via a new put_cpntf_state_unlinked_locked() helper
> that does refcount_dec_and_test() + kfree().  The drain now
> terminates in one pass per entry regardless of cs_count.
> 
> Concurrent holders keep their own reference; their eventual
> nfs4_put_cpntf_state() reaches _free_cpntf_state_locked() on an
> entry the drain has already unlinked from sc_cp_list and from
> nn->s2s_cp_stateids.  _free_cpntf_state_locked() gates its
> list_del_init() and idr_remove() on !list_empty(&cps->cp_list);
> the drain's list_del_init() leaves cp_list self-linked, so the
> late put skips both calls and only the final refcount drop runs.
> 
> Gating idr_remove() on the same check is required because
> idr_alloc_cyclic() may already have recycled the so_id for an
> unrelated cpntf entry by the time the late put runs; an
> unconditional idr_remove() would silently unhash that live entry
> and corrupt subsequent find_cpntf_state() lookups.
> 
> Fixes: 624322f1adc5 ("NFSD add COPY_NOTIFY operation")
> Assisted-by: kres:claude-opus-4-7
> Signed-off-by: Chris Mason <clm@meta.com>
> ---
>  fs/nfsd/nfs4state.c | 65 ++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 52 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b8946db3ebaa..374155e57f3f 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1025,17 +1025,58 @@ void nfs4_free_copy_state(struct nfsd4_copy *copy)
>  	spin_unlock(&nn->s2s_cp_lock);
>  }
>  
> +/*
> + * Drop the parent stid's reference on a cpntf entry that has already been
> + * removed from sc_cp_list and the s2s_cp_stateids IDR. If a concurrent holder
> + * still owns a reference (acquired viamanage_cpntf_state() before the unlink),
> + * that holder's nfs4_put_cpntf_state() will perform the final free.
> + *
> + * The nn->s2s_cp_lock must be held!
> + */
> +static void put_cpntf_state_unlinked_locked(struct nfs4_cpntf_state *cps)
> +{
> +	WARN_ON_ONCE(cps->cp_stateid.cs_type != NFS4_COPYNOTIFY_STID);
> +	WARN_ON_ONCE(!list_empty(&cps->cp_list));
> +
> +	if (refcount_dec_and_test(&cps->cp_stateid.cs_count))
> +		kfree(cps);
> +}
> +
> +/*
> + * Unhash the stateid from the s2s stateid hash, and detach it from the sc_cp_list.
> + * Note that this is gated on a list_empty() check, to avoid problems with IDR
> + * hashval reuse.
> + */
> +static void nfsd4_unhash_cpntf_state(struct nfsd_net *nn, struct nfs4_cpntf_state *cps)
> +{
> +	lockdep_assert_held(&nn->s2s_cp_lock);
> +
> +	if (!list_empty(&cps->cp_list)) {
> +		list_del_init(&cps->cp_list);
> +		idr_remove(&nn->s2s_cp_stateids, cps->cp_stateid.cs_stid.si_opaque.so_id);
> +	}
> +}
> +
>  static void nfs4_free_cpntf_statelist(struct net *net, struct nfs4_stid *stid)
>  {
> -	struct nfs4_cpntf_state *cps;
> +	struct nfs4_cpntf_state *cps, *tmp;
>  	struct nfsd_net *nn;
>  
>  	nn = net_generic(net, nfsd_net_id);
>  	spin_lock(&nn->s2s_cp_lock);
> -	while (!list_empty(&stid->sc_cp_list)) {
> -		cps = list_first_entry(&stid->sc_cp_list,
> -				       struct nfs4_cpntf_state, cp_list);
> -		_free_cpntf_state_locked(nn, cps);
> +	/*
> +	 * Unlink every entry from sc_cp_list and the IDR before dropping
> +	 * the parent's reference.  This makes the drain terminate in one
> +	 * pass per entry regardless of cs_count: a concurrent holder that
> +	 * obtained the entry via manage_cpntf_state() retains its own
> +	 * reference, and its eventual nfs4_put_cpntf_state() will see the
> +	 * entry already unlinked (list_del_init() in
> +	 * _free_cpntf_state_locked makes that a no-op) and drive the final
> +	 * kfree itself.
> +	 */

Again, this comment doesn't belong in the final code.

> +	list_for_each_entry_safe(cps, tmp, &stid->sc_cp_list, cp_list) {

The switch from while() to list_for_each_entry_safe() isn't required for
this change, and isn't justified by the commit message.

I prefer while() is it makes it abundantly clear that the list will be
emptied.
I personally prefer

  while ((cps = list_first_entry_or_null(&stid->sc_cp_list,
				       struct nfs4_cpntf_state,
				       cp_list)) != NULL) {

but I understand that might not be everyone's favourite.

Apart from the comment, the code changes looks correct.

Reviewed-by: NeilBrown <neil@brown.name>

Thanks,
NeilBrown



> +		nfsd4_unhash_cpntf_state(nn, cps);
> +		put_cpntf_state_unlinked_locked(cps);
>  	}
>  	spin_unlock(&nn->s2s_cp_lock);
>  }
> @@ -7870,16 +7911,14 @@ nfs4_check_file(struct svc_rqst *rqstp, struct svc_fh *fhp, struct nfs4_stid *s,
>  out:
>  	return status;
>  }
> -static void
> -_free_cpntf_state_locked(struct nfsd_net *nn, struct nfs4_cpntf_state *cps)
> +
> +static void _free_cpntf_state_locked(struct nfsd_net *nn, struct nfs4_cpntf_state *cps)
>  {
>  	WARN_ON_ONCE(cps->cp_stateid.cs_type != NFS4_COPYNOTIFY_STID);
> -	if (!refcount_dec_and_test(&cps->cp_stateid.cs_count))
> -		return;
> -	list_del_init(&cps->cp_list);
> -	idr_remove(&nn->s2s_cp_stateids,
> -		   cps->cp_stateid.cs_stid.si_opaque.so_id);
> -	kfree(cps);
> +	if (refcount_dec_and_test(&cps->cp_stateid.cs_count)) {
> +		nfsd4_unhash_cpntf_state(nn, cps);
> +		kfree(cps);
> +	}
>  }
>  /*
>   * A READ from an inter server to server COPY will have a
> 
> -- 
> 2.54.0
> 
> 

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

[Jeff Layton]

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.

Fixes: ac0514f4d198 ("NFSD: Add a laundromat reaper for async copy state")
Assisted-by: kres:claude-opus-4-8
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 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

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@kernel.org>
> ---
>  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
> 
>