mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Jeff Layton <jlayton@kernel.org>
To: Chuck Lever <cel@kernel.org>, NeilBrown <neil@brown.name>,
	 Olga Kornievskaia <okorniev@redhat.com>,
	Dai Ngo <Dai.Ngo@oracle.com>,  Tom Talpey <tom@talpey.com>,
	Andy Adamson <andros@netapp.com>
Cc: Chris Mason <clm@meta.com>,
	linux-nfs@vger.kernel.org,  linux-kernel@vger.kernel.org,
	Jeff Layton <jlayton@kernel.org>
Subject: [PATCH 3/3] nfsd: fix UAF in async copy cancel and shutdown
Date: Wed, 24 Jun 2026 13:51:45 -0400	[thread overview]
Message-ID: <20260624-nfsd-testing-v1-3-b8853eb22e45@kernel.org> (raw)
In-Reply-To: <20260624-nfsd-testing-v1-0-b8853eb22e45@kernel.org>

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


  parent reply	other threads:[~2026-06-24 17:52 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-06-24 17:51 [PATCH 0/3] nfsd: inter-server copy fixes Jeff Layton
2026-06-24 17:51 ` [PATCH 1/3] nfsd: fix cpntf publish race in nfs4_init_cp_state Jeff Layton
2026-06-26  3:46   ` NeilBrown
2026-06-24 17:51 ` [PATCH 2/3] nfsd: fix UAF in cpntf statelist drain Jeff Layton
2026-06-26  4:02   ` NeilBrown
2026-06-24 17:51 ` Jeff Layton [this message]
2026-06-26  4:37   ` [PATCH 3/3] nfsd: fix UAF in async copy cancel and shutdown NeilBrown

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260624-nfsd-testing-v1-3-b8853eb22e45@kernel.org \
    --to=jlayton@kernel.org \
    --cc=Dai.Ngo@oracle.com \
    --cc=andros@netapp.com \
    --cc=cel@kernel.org \
    --cc=clm@meta.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=neil@brown.name \
    --cc=okorniev@redhat.com \
    --cc=tom@talpey.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

Powered by JetHome