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

[Jorge Mora <jmora1300@gmail.com>]

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