[PATCH v2] xprtrdma: fix race between frwr_unmap_sync() and transport disconnect

[PATCH v2] xprtrdma: fix race between frwr_unmap_sync() and transport disconnect

[Jorge Mora]

From: Rakesh Bhargava <Rakesh.Bhargava@netapp.com>

frwr_unmap_sync() is called from xprt_rdma_free() via ->buf_free, which
xprt_release() invokes without holding the transport send lock and
without XPRT_LOCKED. rpcrdma_xprt_disconnect() runs from xprt_autoclose()
under XPRT_LOCKED and, after draining the QP, resets every rpcrdma_req
(rpcrdma_reqs_reset) and destroys every MR (rpcrdma_mrs_destroy). Nothing
orders the two, so both can walk and list_del() the same req's
rl_registered list and the same MRs. Observed on RHEL 10.2
(6.12.0-211.7.3.el10_2) NFS/RDMA FlexFiles clients as:

  kernel BUG at lib/list_debug.c:65!
  frwr_unmap_sync+0x75 [rpcrdma]
  xprt_release+... [sunrpc]

In a vmcore, the panicking task was in frwr_unmap_sync() for r_xprt X
while another kworker was in xprt_autoclose -> rpcrdma_xprt_disconnect
-> rpcrdma_reqs_reset -> frwr_mr_release for the same r_xprt X. The
window was large because 256 transports were being torn down at once,
each blocked in mlx5_core_destroy_mkey waiting on firmware.

Fix it by making frwr_unmap_sync() and disconnect cooperate through
rb_lock:

 - frwr_unmap_sync() reads rx_ep, checks rb_closing and that
   rl_registered is non-empty, and claims all of the req's MRs under
   rb_lock. It bumps rb_unmap_users while it still references the MRs
   and the ep, and drops it (waking rb_unmap_wq) when done.
 - If rb_closing is set, frwr_unmap_sync() returns and leaves the MRs on
   rl_registered. rb_closing is only set after rdma_disconnect(), when
   the QP is in the error state and the peer cannot reach the MRs;
   rpcrdma_reqs_reset() then releases them.
 - rpcrdma_xprt_disconnect() sets rb_closing after rdma_disconnect(),
   waits for rb_unmap_users to reach zero after draining the QP and
   before releasing reps, reqs and MRs, and clears rx_ep and rb_closing
   together under rb_lock after dropping the ep reference.

Signed-off-by: Rakesh Bhargava <Rakesh.Bhargava@netapp.com>
Signed-off-by: Jorge Mora <mora@netapp.com>
---
 net/sunrpc/xprtrdma/frwr_ops.c  | 43 +++++++++++++++++++++++++++------
 net/sunrpc/xprtrdma/verbs.c     | 31 ++++++++++++++++++++++++
 net/sunrpc/xprtrdma/xprt_rdma.h | 14 +++++++++++
 3 files changed, 81 insertions(+), 7 deletions(-)

diff --git a/net/sunrpc/xprtrdma/frwr_ops.c b/net/sunrpc/xprtrdma/frwr_ops.c
index e83cef19e656..03e0e6c475b1 100644
--- a/net/sunrpc/xprtrdma/frwr_ops.c
+++ b/net/sunrpc/xprtrdma/frwr_ops.c
@@ -567,12 +567,33 @@ static void frwr_wc_localinv_wake(struct ib_cq *cq, struct ib_wc *wc)
  */
 void frwr_unmap_sync(struct rpcrdma_xprt *r_xprt, struct rpcrdma_req *req)
 {
+	struct rpcrdma_buffer *buf = &r_xprt->rx_buf;
 	struct ib_send_wr *first, **prev, *last;
-	struct rpcrdma_ep *ep = r_xprt->rx_ep;
 	const struct ib_send_wr *bad_wr;
+	struct rpcrdma_ep *ep;
 	struct rpcrdma_mr *mr;
 	int rc;
 
+	/* This function is invoked via ->buf_free, which does not
+	 * hold the transport send lock. It therefore races with
+	 * rpcrdma_xprt_disconnect(), which resets every req and
+	 * destroys every MR.
+	 *
+	 * Claim the req's MRs, and pin the transport's ep, under
+	 * rb_lock. If the disconnect path has already put the QP
+	 * into the error state, leave the MRs on rl_registered:
+	 * the peer can no longer reach them, and
+	 * rpcrdma_reqs_reset() releases them.
+	 */
+	spin_lock(&buf->rb_lock);
+	ep = r_xprt->rx_ep;
+	if (buf->rb_closing || !ep ||
+	    list_empty(&req->rl_registered)) {
+		spin_unlock(&buf->rb_lock);
+		return;
+	}
+	buf->rb_unmap_users++;
+
 	/* ORDER: Invalidate all of the MRs first
 	 *
 	 * Chain the LOCAL_INV Work Requests and post them with
@@ -598,6 +619,7 @@ void frwr_unmap_sync(struct rpcrdma_xprt *r_xprt, struct rpcrdma_req *req)
 		*prev = last;
 		prev = &last->next;
 	} while ((mr = rpcrdma_mr_pop(&req->rl_registered)));
+	spin_unlock(&buf->rb_lock);
 
 	mr = container_of(last, struct rpcrdma_mr, mr_invwr);
 
@@ -621,15 +643,22 @@ void frwr_unmap_sync(struct rpcrdma_xprt *r_xprt, struct rpcrdma_req *req)
 	 */
 	if (bad_wr != first)
 		wait_for_completion(&mr->mr_linv_done);
-	if (!rc)
-		return;
+	if (rc) {
+		/* On error, the MRs get destroyed once the QP has drained. */
+		trace_xprtrdma_post_linv_err(req, rc);
 
-	/* On error, the MRs get destroyed once the QP has drained. */
-	trace_xprtrdma_post_linv_err(req, rc);
+		/* Force a connection loss to ensure complete recovery.
+		 */
+		rpcrdma_force_disconnect(ep);
+	}
 
-	/* Force a connection loss to ensure complete recovery.
+	/* The MRs and the ep are no longer referenced. Allow
+	 * rpcrdma_xprt_disconnect() to proceed.
 	 */
-	rpcrdma_force_disconnect(ep);
+	spin_lock(&buf->rb_lock);
+	if (!--buf->rb_unmap_users)
+		wake_up(&buf->rb_unmap_wq);
+	spin_unlock(&buf->rb_lock);
 }
 
 /**
diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
index 54957295a631..b0c42cbe38ec 100644
--- a/net/sunrpc/xprtrdma/verbs.c
+++ b/net/sunrpc/xprtrdma/verbs.c
@@ -580,6 +580,7 @@ int rpcrdma_xprt_connect(struct rpcrdma_xprt *r_xprt)
  */
 void rpcrdma_xprt_disconnect(struct rpcrdma_xprt *r_xprt)
 {
+	struct rpcrdma_buffer *buf = &r_xprt->rx_buf;
 	struct rpcrdma_ep *ep = r_xprt->rx_ep;
 	struct rdma_cm_id *id;
 	int rc;
@@ -594,7 +595,27 @@ void rpcrdma_xprt_disconnect(struct rpcrdma_xprt *r_xprt)
 	rc = rdma_disconnect(id);
 	trace_xprtrdma_disconnect(r_xprt, rc);
 
+	/* The QP is in the error state now, so the peer can no
+	 * longer access any registered MR. From here on, a
+	 * frwr_unmap_sync() caller that has not yet claimed a req's
+	 * MRs leaves them on rl_registered: rpcrdma_reqs_reset()
+	 * releases them below.
+	 */
+	spin_lock(&buf->rb_lock);
+	buf->rb_closing = true;
+	spin_unlock(&buf->rb_lock);
+
 	rpcrdma_xprt_drain(r_xprt);
+
+	/* frwr_unmap_sync() runs without the transport send lock.
+	 * Callers that claimed MRs before rb_closing was set may
+	 * still be posting or waiting on LOCAL_INV Work Requests.
+	 * Those complete (or flush) because the QP has been
+	 * drained, but the callers still reference the MRs and the
+	 * ep, so wait for them before anything is released.
+	 */
+	wait_event(buf->rb_unmap_wq, !READ_ONCE(buf->rb_unmap_users));
+
 	rpcrdma_reps_unmap(r_xprt);
 	rpcrdma_sendctxs_destroy(r_xprt);
 	rpcrdma_reqs_reset(r_xprt);
@@ -603,7 +624,14 @@ void rpcrdma_xprt_disconnect(struct rpcrdma_xprt *r_xprt)
 	if (rpcrdma_ep_put(ep))
 		rdma_destroy_id(id);
 
+	/* rb_closing stays set until rx_ep is cleared, so a late
+	 * frwr_unmap_sync() caller returns before it can use a stale
+	 * rx_ep.
+	 */
+	spin_lock(&buf->rb_lock);
 	r_xprt->rx_ep = NULL;
+	buf->rb_closing = false;
+	spin_unlock(&buf->rb_lock);
 }
 
 /* Fixed-size circular FIFO queue. This implementation is wait-free and
@@ -1171,6 +1199,9 @@ int rpcrdma_buffer_create(struct rpcrdma_xprt *r_xprt)
 
 	buf->rb_bc_srv_max_requests = 0;
 	spin_lock_init(&buf->rb_lock);
+	buf->rb_closing = false;
+	buf->rb_unmap_users = 0;
+	init_waitqueue_head(&buf->rb_unmap_wq);
 	INIT_LIST_HEAD(&buf->rb_mrs);
 	INIT_LIST_HEAD(&buf->rb_all_mrs);
 	INIT_WORK(&buf->rb_refresh_worker, rpcrdma_mr_refresh_worker);
diff --git a/net/sunrpc/xprtrdma/xprt_rdma.h b/net/sunrpc/xprtrdma/xprt_rdma.h
index 4cbc941e4a3e..0161317e4916 100644
--- a/net/sunrpc/xprtrdma/xprt_rdma.h
+++ b/net/sunrpc/xprtrdma/xprt_rdma.h
@@ -402,6 +402,20 @@ struct rpcrdma_buffer {
 	u32			rb_bc_max_requests;
 
 	struct work_struct	rb_refresh_worker;
+
+	/* Serializes frwr_unmap_sync(), which runs from ->buf_free
+	 * without holding the transport send lock, against
+	 * rpcrdma_xprt_disconnect(). Protected by rb_lock.
+	 *
+	 * rb_closing: the QP is in the error state and the disconnect
+	 *	path is about to reset all reqs and destroy all MRs.
+	 * rb_unmap_users: number of frwr_unmap_sync() callers that
+	 *	own MRs taken from a req's rl_registered list, or have
+	 *	LOCAL_INV Work Requests outstanding on the QP.
+	 */
+	bool			rb_closing;
+	unsigned int		rb_unmap_users;
+	wait_queue_head_t	rb_unmap_wq;
 };
 
 /*

base-commit: ef30b3efb7e8bae94d790a6503a595e1051323e7
-- 
2.52.0

Re: [PATCH v2] xprtrdma: fix race between frwr_unmap_sync() and transport disconnect

[Chuck Lever]

On 11/5/26 6:21 PM, Jorge Mora wrote:
> frwr_unmap_sync() is called from xprt_rdma_free() via ->buf_free, which
> xprt_release() invokes without holding the transport send lock and
> without XPRT_LOCKED. rpcrdma_xprt_disconnect() runs from xprt_autoclose()
> under XPRT_LOCKED and, after draining the QP, resets every rpcrdma_req
> (rpcrdma_reqs_reset) and destroys every MR (rpcrdma_mrs_destroy). Nothing
> orders the two, so both can walk and list_del() the same req's
> rl_registered list and the same MRs.

Thanks for the vmcore analysis, and for the detail about the
destroy_mkey stall widening the window. That explains why the
list corruption is what shows up on your systems.

Tim Menninger has been working the same race from the other
symptom, a hung task in frwr_unmap_sync() when the LOCAL_INV chain
is posted after the QP drain. His v5 went out this morning:

  https://lore.kernel.org/linux-nfs/20261106142039.1066437-1-tmenninger@everpuredata.com/

> + - If rb_closing is set, frwr_unmap_sync() returns and leaves the MRs on
> +   rl_registered. rb_closing is only set after rdma_disconnect(), when
> +   the QP is in the error state and the peer cannot reach the MRs;
> +   rpcrdma_reqs_reset() then releases them.

When frwr_unmap_sync() returns here, xprt_rdma_free() returns and
the RPC's pages go back to the consumer while the MRs are still
DMA-mapped. frwr_mr_unmap() does not run until rpcrdma_req_reset()
reaches that req. On a platform with swiotlb or a bounce-buffering
IOMMU, the deferred dma_unmap_sg() for a FROM_DEVICE mapping copies
the bounce buffer into a page that may already be freed and reused.
Tim's version always unmaps before frwr_unmap_sync() returns,
either in the LOCAL_INV completion handler or because the reset
already ran.


> +	/* frwr_unmap_sync() runs without the transport send lock.
> +	 * Callers that claimed MRs before rb_closing was set may
> +	 * still be posting or waiting on LOCAL_INV Work Requests.
> +	 * Those complete (or flush) because the QP has been
> +	 * drained, but the callers still reference the MRs and the
> +	 * ep, so wait for them before anything is released.
> +	 */
> +	wait_event(buf->rb_unmap_wq, !READ_ONCE(buf->rb_unmap_users));

A caller that bumped rb_unmap_users just before rb_closing was set
can still reach ib_post_send() after rpcrdma_xprt_drain() has
returned. The comment relies on the provider flushing a WR posted
to an error-state QP. Tim's report is that no CQE arrived in that
case on his hardware. I can't tell from here which providers
guarantee the flush, but if his observation holds, this patch
leaves that hang in place and adds a second waiter: disconnect now
sleeps in wait_event() for the stuck caller, so the autoclose work
and the whole transport stop with it.

Tim's rwsem makes the post happen before the drain or not at all,
which removes the dependence on provider behavior. The counter,
flag, and waitqueue here are doing the same job as his rwsem by
hand, with that one ordering missing.

Would you be able to run Tim's v5 against your reproducer? If it
holds up, a Tested-by from NetApp, plus the vmcore details added
to his patch description, would make a stronger single submission
than either patch alone.

I'm a reviewer here, not the maintainer, so the final call on
which fix goes in is Anna's and Trond's. Tim's looks a little
more complete. Sorry I didn't realize earlier that both of you
were working the same issue.


-- 
Chuck Lever (Come to NFS bake-a-thon! https://nfsv4bat.org)

Re: [PATCH v2] xprtrdma: fix race between frwr_unmap_sync() and transport disconnect

[Mora, Jorge]

Hello Chuck,

I will take a look at Tim's patch.

Have a great weekend and thank you.

--Jorge

________________________________________
From: Chuck Lever <cel@kernel.org>
Sent: Friday, November 6, 2026 8:07 AM
To: Jorge Mora
Cc: linux-nfs@vger.kernel.org; trondmy@kernel.org; anna@kernel.org; Bhargava, Rakesh; Kumar, Mohit; tmenninger@everpuredata.com
Subject: Re: [PATCH v2] xprtrdma: fix race between frwr_unmap_sync() and transport disconnect

EXTERNAL EMAIL - USE CAUTION when clicking links or attachments




On 11/5/26 6:21 PM, Jorge Mora wrote:
> frwr_unmap_sync() is called from xprt_rdma_free() via ->buf_free, which
> xprt_release() invokes without holding the transport send lock and
> without XPRT_LOCKED. rpcrdma_xprt_disconnect() runs from xprt_autoclose()
> under XPRT_LOCKED and, after draining the QP, resets every rpcrdma_req
> (rpcrdma_reqs_reset) and destroys every MR (rpcrdma_mrs_destroy). Nothing
> orders the two, so both can walk and list_del() the same req's
> rl_registered list and the same MRs.

Thanks for the vmcore analysis, and for the detail about the
destroy_mkey stall widening the window. That explains why the
list corruption is what shows up on your systems.

Tim Menninger has been working the same race from the other
symptom, a hung task in frwr_unmap_sync() when the LOCAL_INV chain
is posted after the QP drain. His v5 went out this morning:

  https://urldefense.com/v3/__https://lore.kernel.org/linux-nfs/20261106142039.1066437-1-tmenninger@everpuredata.com/__;!!Nhn8V6BzJA!VKRwv5y7IYIdxq4TXPOHCY6wtyQ49nOa9F4m192VwlYSRqCQXJxF5lOKHGGCcTmvecPKnNR6x52l$

> + - If rb_closing is set, frwr_unmap_sync() returns and leaves the MRs on
> +   rl_registered. rb_closing is only set after rdma_disconnect(), when
> +   the QP is in the error state and the peer cannot reach the MRs;
> +   rpcrdma_reqs_reset() then releases them.

When frwr_unmap_sync() returns here, xprt_rdma_free() returns and
the RPC's pages go back to the consumer while the MRs are still
DMA-mapped. frwr_mr_unmap() does not run until rpcrdma_req_reset()
reaches that req. On a platform with swiotlb or a bounce-buffering
IOMMU, the deferred dma_unmap_sg() for a FROM_DEVICE mapping copies
the bounce buffer into a page that may already be freed and reused.
Tim's version always unmaps before frwr_unmap_sync() returns,
either in the LOCAL_INV completion handler or because the reset
already ran.


> +     /* frwr_unmap_sync() runs without the transport send lock.
> +      * Callers that claimed MRs before rb_closing was set may
> +      * still be posting or waiting on LOCAL_INV Work Requests.
> +      * Those complete (or flush) because the QP has been
> +      * drained, but the callers still reference the MRs and the
> +      * ep, so wait for them before anything is released.
> +      */
> +     wait_event(buf->rb_unmap_wq, !READ_ONCE(buf->rb_unmap_users));

A caller that bumped rb_unmap_users just before rb_closing was set
can still reach ib_post_send() after rpcrdma_xprt_drain() has
returned. The comment relies on the provider flushing a WR posted
to an error-state QP. Tim's report is that no CQE arrived in that
case on his hardware. I can't tell from here which providers
guarantee the flush, but if his observation holds, this patch
leaves that hang in place and adds a second waiter: disconnect now
sleeps in wait_event() for the stuck caller, so the autoclose work
and the whole transport stop with it.

Tim's rwsem makes the post happen before the drain or not at all,
which removes the dependence on provider behavior. The counter,
flag, and waitqueue here are doing the same job as his rwsem by
hand, with that one ordering missing.

Would you be able to run Tim's v5 against your reproducer? If it
holds up, a Tested-by from NetApp, plus the vmcore details added
to his patch description, would make a stronger single submission
than either patch alone.

I'm a reviewer here, not the maintainer, so the final call on
which fix goes in is Anna's and Trond's. Tim's looks a little
more complete. Sorry I didn't realize earlier that both of you
were working the same issue.


--
Chuck Lever (Come to NFS bake-a-thon! https://urldefense.com/v3/__https://nfsv4bat.org__;!!Nhn8V6BzJA!VKRwv5y7IYIdxq4TXPOHCY6wtyQ49nOa9F4m192VwlYSRqCQXJxF5lOKHGGCcTmvecPKnI-QvuUw$ )