Re: [PATCH v2 1/3] rpcrdma: Serialize notification registration with device removal

Re: [PATCH v2 1/3] rpcrdma: Serialize notification registration with device removal

[sashiko-bot]

> rpcrdma_rn_register() checks RPCRDMA_RD_F_REMOVING before publishing the
> notification and taking a reference on the rpcrdma_device. Nothing
> serializes these operations with rpcrdma_remove_one(). Device removal
> can therefore free the private data after the check but before xa_alloc()
> or kref_get(), resulting in a use-after-free. It can also finish its
> notification walk before a new registration is published, leaving the
> removal path waiting for a registrant that was never notified.
> 
> Use a module-wide mutex to protect the client-data lookup, removal-state
> check, notification publication and reference acquisition against the
> transition to REMOVING. Clear client-data under the same mutex before
> destroying the XArray and freeing the private data, so a later lookup
> cannot return a stale pointer. Keep notification callbacks and completion
> waits outside the critical sections.
> 
> Fixes: 7e86845a0346 ("rpcrdma: Implement generic device removal")
> Signed-off-by: Li Xiasong <lixiasong1@huawei.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103032758.1302493-1-lixiasong1@huawei.com?part=1

Re: [PATCH v2 2/3] rpcrdma: Pin notifications during removal callbacks

[sashiko-bot]

> rpcrdma_remove_one() can race with unregister and invoke a notification
> callback after its owner is freed. XArray RCU does not pin stored objects.
> 
> Pin notification owners under rpcrdma_device_mutex and serialize
> unregister with the same lock. Also pin client RPC transports. Skip
> owners in final release with kref_get_unless_zero().
> 
> Invoke rn_done and release references outside the mutex. Track references
> separately so partial acquisition can be unwound without accessing freed
> owners or waiting for XPRT_LOCKED under the device mutex.
> 
> Fixes: 7e86845a0346 ("rpcrdma: Implement generic device removal")
> Signed-off-by: Li Xiasong <lixiasong1@huawei.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103032758.1302493-1-lixiasong1@huawei.com?part=2

[PATCH v2 0/3] rpcrdma: Fix device removal races and endpoint cleanup

[Li Xiasong]

This series fixes races and endpoint cleanup issues in the RPC-over-RDMA
device removal paths.

Notification registration is not serialized with device removal. Removal
can free the per-device private data while registration is still using it,
or finish walking the notification array before a new registration is
published. In addition, the notification walk does not hold references to
the objects containing the notifications, allowing concurrent unregister
and teardown to free an object before its removal callback is invoked.

On the client side, the device removal callback only forces a transport
disconnect. A connect worker waiting for re_connect_status to change can
remain asleep, preventing endpoint cleanup. Removal that races with
connection establishment can also be overwritten by a late ESTABLISHED
event, and releasing the endpoint from a removal callback or address
change can free its CM ID context before CMA has stopped dispatching
events to it.

Patch 1 serializes notification registration with the transition to device
removal and protects access to the per-device private data.

Patch 2 uses the same mutex to serialize notification reference acquisition
with unregister, and holds references across each removal callback. Each
notification records the references it acquires, so a partially completed
acquisition can be unwound without touching a freed owner or blocking on
XPRT_LOCKED under the device mutex. The client endpoint pin also takes a
reference on the RPC transport. Callbacks and reference release run outside
the mutex to allow teardown to unregister notifications.

Patch 3 records device removal in the endpoint connection status and wakes
a waiting connect worker. Atomic state transitions ensure the first path to
leave the established state forces a disconnect exactly once. It drops the
connection-management callback get/put pairs and destroys the CM ID in the
endpoint destructor, before freeing its context, so that active and future
callbacks are fenced. It also handles connection establishment racing with
removal.

The patches are ordered by dependency: patch 2 uses the serialization
introduced by patch 1, and patch 3 relies on the callback lifetime
protection added by patch 2.

Changes since v1:
- Pin the client RPC transport alongside the endpoint owner, and track
  acquired references separately so a partial acquisition can be unwound
  without accessing a freed owner or waiting for XPRT_LOCKED under the
  device mutex.
- Add kernel-doc for the new get/put callbacks and Context: sections to
  the notification registration interfaces.
- Rework client endpoint lifetime: remove the connection-management
  callback get/put pairs and destroy the CM ID in the endpoint destructor
  before freeing its context, closing the use-after-free window on
  teardown.
- Give UNREACHABLE and REJECTED the same atomic status handling as
  ADDR_CHANGE so a transition on an established connection forces a
  disconnect exactly once.
- Link to v1: https://lore.kernel.org/all/20261023125033.528835-1-lixiasong1@huawei.com/

Li Xiasong (3):
  rpcrdma: Serialize notification registration with device removal
  rpcrdma: Pin notifications during removal callbacks
  xprtrdma: Wake connect worker on device removal

 include/linux/sunrpc/rdma_rn.h           |  29 +++++-
 net/sunrpc/xprtrdma/ib_client.c          |  84 +++++++++++++++--
 net/sunrpc/xprtrdma/svc_rdma_transport.c |  21 ++++-
 net/sunrpc/xprtrdma/verbs.c              | 114 ++++++++++++++++-------
 4 files changed, 203 insertions(+), 45 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] rpcrdma: Serialize notification registration with device removal

[Li Xiasong]

rpcrdma_rn_register() checks RPCRDMA_RD_F_REMOVING before publishing the
notification and taking a reference on the rpcrdma_device. Nothing
serializes these operations with rpcrdma_remove_one(). Device removal
can therefore free the private data after the check but before xa_alloc()
or kref_get(), resulting in a use-after-free. It can also finish its
notification walk before a new registration is published, leaving the
removal path waiting for a registrant that was never notified.

Use a module-wide mutex to protect the client-data lookup, removal-state
check, notification publication and reference acquisition against the
transition to REMOVING. Clear client-data under the same mutex before
destroying the XArray and freeing the private data, so a later lookup
cannot return a stale pointer. Keep notification callbacks and completion
waits outside the critical sections.

Fixes: 7e86845a0346 ("rpcrdma: Implement generic device removal")
Signed-off-by: Li Xiasong <lixiasong1@huawei.com>
---
 net/sunrpc/xprtrdma/ib_client.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/net/sunrpc/xprtrdma/ib_client.c b/net/sunrpc/xprtrdma/ib_client.c
index 188f7a13397f..23ab1dfcb230 100644
--- a/net/sunrpc/xprtrdma/ib_client.c
+++ b/net/sunrpc/xprtrdma/ib_client.c
@@ -9,6 +9,7 @@
 #include <linux/types.h>
 #include <linux/kref.h>
 #include <linux/completion.h>
+#include <linux/mutex.h>
 
 #include <linux/sunrpc/svc_rdma.h>
 #include <linux/sunrpc/rdma_rn.h>
@@ -28,6 +29,11 @@ struct rpcrdma_device {
 #define RPCRDMA_RD_F_REMOVING	(0)
 
 static struct ib_client rpcrdma_ib_client;
+/*
+ * Serialize registration with the removal transition and client-data teardown.
+ * Invoke rn_done callbacks and wait for completion outside this mutex.
+ */
+static DEFINE_MUTEX(rpcrdma_device_mutex);
 
 /*
  * Listeners have no associated device, so we never register them.
@@ -56,15 +62,20 @@ static struct rpcrdma_device *rpcrdma_get_client_data(struct ib_device *device)
  * cleared again if xa_alloc fails), so the @rn may safely be
  * passed to rpcrdma_rn_unregister() without a separate
  * registered/unregistered flag in the caller.
+ *
+ * Context: May sleep. Takes rpcrdma_device_mutex.
  */
 int rpcrdma_rn_register(struct ib_device *device,
 			struct rpcrdma_notification *rn,
 			void (*done)(struct rpcrdma_notification *rn))
 {
-	struct rpcrdma_device *rd = rpcrdma_get_client_data(device);
+	struct rpcrdma_device *rd;
+	int ret = -ENETUNREACH;
 
+	mutex_lock(&rpcrdma_device_mutex);
+	rd = rpcrdma_get_client_data(device);
 	if (!rd || test_bit(RPCRDMA_RD_F_REMOVING, &rd->rd_flags))
-		return -ENETUNREACH;
+		goto out_unlock;
 
 	/*
 	 * Arm rn_done before xa_alloc() publishes @rn: once @rn is
@@ -78,11 +89,15 @@ int rpcrdma_rn_register(struct ib_device *device,
 	rn->rn_done = done;
 	if (xa_alloc(&rd->rd_xa, &rn->rn_index, rn, xa_limit_32b, GFP_KERNEL) < 0) {
 		rn->rn_done = NULL;
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto out_unlock;
 	}
 	kref_get(&rd->rd_kref);
 	trace_rpcrdma_client_register(device, rn);
-	return 0;
+	ret = 0;
+out_unlock:
+	mutex_unlock(&rpcrdma_device_mutex);
+	return ret;
 }
 
 static void rpcrdma_rn_release(struct kref *kref)
@@ -163,6 +178,8 @@ static int rpcrdma_add_one(struct ib_device *device)
  *
  * Upon return, all transports associated with @device have divested
  * themselves from IB hardware resources.
+ *
+ * Context: May sleep. Takes rpcrdma_device_mutex.
  */
 static void rpcrdma_remove_one(struct ib_device *device,
 			       void *client_data)
@@ -173,7 +190,9 @@ static void rpcrdma_remove_one(struct ib_device *device,
 
 	trace_rpcrdma_client_remove_one(device);
 
+	mutex_lock(&rpcrdma_device_mutex);
 	set_bit(RPCRDMA_RD_F_REMOVING, &rd->rd_flags);
+	mutex_unlock(&rpcrdma_device_mutex);
 	xa_for_each(&rd->rd_xa, index, rn)
 		rn->rn_done(rn);
 
@@ -187,8 +206,11 @@ static void rpcrdma_remove_one(struct ib_device *device,
 	}
 
 	trace_rpcrdma_client_remove_one_done(device);
+	mutex_lock(&rpcrdma_device_mutex);
+	ib_set_client_data(device, &rpcrdma_ib_client, NULL);
 	xa_destroy(&rd->rd_xa);
 	kfree(rd);
+	mutex_unlock(&rpcrdma_device_mutex);
 }
 
 static struct ib_client rpcrdma_ib_client = {
-- 
2.34.1

[PATCH v2 2/3] rpcrdma: Pin notifications during removal callbacks

[Li Xiasong]

rpcrdma_remove_one() can race with unregister and invoke a notification
callback after its owner is freed. XArray RCU does not pin stored objects.

Pin notification owners under rpcrdma_device_mutex and serialize
unregister with the same lock. Also pin client RPC transports. Skip
owners in final release with kref_get_unless_zero().

Invoke rn_done and release references outside the mutex. Track references
separately so partial acquisition can be unwound without accessing freed
owners or waiting for XPRT_LOCKED under the device mutex.

Fixes: 7e86845a0346 ("rpcrdma: Implement generic device removal")
Signed-off-by: Li Xiasong <lixiasong1@huawei.com>
---
 include/linux/sunrpc/rdma_rn.h           | 29 ++++++++++++-
 net/sunrpc/xprtrdma/ib_client.c          | 54 +++++++++++++++++++++---
 net/sunrpc/xprtrdma/svc_rdma_transport.c | 21 ++++++++-
 net/sunrpc/xprtrdma/verbs.c              | 34 ++++++++++++++-
 4 files changed, 128 insertions(+), 10 deletions(-)

diff --git a/include/linux/sunrpc/rdma_rn.h b/include/linux/sunrpc/rdma_rn.h
index 7d032ca057af..1b3e0989b3a5 100644
--- a/include/linux/sunrpc/rdma_rn.h
+++ b/include/linux/sunrpc/rdma_rn.h
@@ -8,17 +8,42 @@
 
 #include <rdma/ib_verbs.h>
 
+struct rpc_xprt;
+
+/**
+ * struct rpcrdma_notification_ref - references acquired for a notification
+ * @rr_owner: pinned notification owner, or NULL if not acquired
+ * @rr_xprt: optional RPC transport reference, or NULL if not acquired
+ *
+ * The record is initialized to zero before rn_get(). Record all acquired
+ * references here, including on partial failure, for release by rn_put().
+ */
+struct rpcrdma_notification_ref {
+	void			*rr_owner;
+	struct rpc_xprt		*rr_xprt;
+};
+
 /**
- * rpcrdma_notification - request removal notification
+ * struct rpcrdma_notification - request removal notification
+ * @rn_done: device removal callback
+ * @rn_get: acquire references as described in rpcrdma_rn_register()
+ * @rn_put: release references as described in rpcrdma_rn_register()
+ * @rn_index: index of this notification in the device's XArray
  */
 struct rpcrdma_notification {
 	void			(*rn_done)(struct rpcrdma_notification *rn);
+	bool			(*rn_get)(struct rpcrdma_notification *rn,
+					  struct rpcrdma_notification_ref *ref);
+	void			(*rn_put)(struct rpcrdma_notification_ref *ref);
 	u32			rn_index;
 };
 
 int rpcrdma_rn_register(struct ib_device *device,
 			struct rpcrdma_notification *rn,
-			void (*done)(struct rpcrdma_notification *rn));
+			void (*done)(struct rpcrdma_notification *rn),
+			bool (*get)(struct rpcrdma_notification *rn,
+				    struct rpcrdma_notification_ref *ref),
+			void (*put)(struct rpcrdma_notification_ref *ref));
 void rpcrdma_rn_unregister(struct ib_device *device,
 			   struct rpcrdma_notification *rn);
 int rpcrdma_ib_client_register(void);
diff --git a/net/sunrpc/xprtrdma/ib_client.c b/net/sunrpc/xprtrdma/ib_client.c
index 23ab1dfcb230..6fa72996209a 100644
--- a/net/sunrpc/xprtrdma/ib_client.c
+++ b/net/sunrpc/xprtrdma/ib_client.c
@@ -52,6 +52,18 @@ static struct rpcrdma_device *rpcrdma_get_client_data(struct ib_device *device)
  * @device: device to monitor
  * @rn: notification object that wishes to be notified
  * @done: callback to notify caller of device removal
+ * @get: callback to acquire a notification object reference
+ * @put: callback to release references acquired by @get
+ *
+ * @get runs under rpcrdma_device_mutex. On success, it must pin both
+ * the object embedding @rn and its registration until the returned
+ * reference is released. The owner must not call rpcrdma_rn_unregister()
+ * while that pin is held. @get must record all acquired references,
+ * including on partial failure, in &struct rpcrdma_notification_ref.
+ *
+ * @get returns true if @done can be invoked. @put releases any acquired
+ * references outside rpcrdma_device_mutex, even if @get returns false,
+ * and must not access an unpinned @rn.
  *
  * Returns zero on success. The callback in rn_done is guaranteed
  * to be invoked when the device is removed, unless this notification
@@ -67,7 +79,10 @@ static struct rpcrdma_device *rpcrdma_get_client_data(struct ib_device *device)
  */
 int rpcrdma_rn_register(struct ib_device *device,
 			struct rpcrdma_notification *rn,
-			void (*done)(struct rpcrdma_notification *rn))
+			void (*done)(struct rpcrdma_notification *rn),
+			bool (*get)(struct rpcrdma_notification *rn,
+				    struct rpcrdma_notification_ref *ref),
+			void (*put)(struct rpcrdma_notification_ref *ref))
 {
 	struct rpcrdma_device *rd;
 	int ret = -ENETUNREACH;
@@ -87,8 +102,12 @@ int rpcrdma_rn_register(struct ib_device *device,
 	 * value would unregister an @rn that was never inserted.
 	 */
 	rn->rn_done = done;
+	rn->rn_get = get;
+	rn->rn_put = put;
 	if (xa_alloc(&rd->rd_xa, &rn->rn_index, rn, xa_limit_32b, GFP_KERNEL) < 0) {
 		rn->rn_done = NULL;
+		rn->rn_get = NULL;
+		rn->rn_put = NULL;
 		ret = -ENOMEM;
 		goto out_unlock;
 	}
@@ -117,14 +136,18 @@ static void rpcrdma_rn_release(struct kref *kref)
  * It is safe to call this on an @rn whose registration never
  * completed or failed; rn_done == NULL is treated as
  * never-registered and the call is a no-op.
+ *
+ * Context: May sleep. Takes rpcrdma_device_mutex.
  */
 void rpcrdma_rn_unregister(struct ib_device *device,
 			   struct rpcrdma_notification *rn)
 {
-	struct rpcrdma_device *rd = rpcrdma_get_client_data(device);
+	struct rpcrdma_device *rd;
 
+	mutex_lock(&rpcrdma_device_mutex);
+	rd = rpcrdma_get_client_data(device);
 	if (!rd)
-		return;
+		goto out_unlock;
 
 	/*
 	 * rn_done is the registration sentinel: rpcrdma_rn_register
@@ -138,12 +161,16 @@ void rpcrdma_rn_unregister(struct ib_device *device,
 	 * rd_kref.
 	 */
 	if (!rn->rn_done)
-		return;
+		goto out_unlock;
 	rn->rn_done = NULL;
+	rn->rn_get = NULL;
+	rn->rn_put = NULL;
 
 	trace_rpcrdma_client_unregister(device, rn);
 	xa_erase(&rd->rd_xa, rn->rn_index);
 	kref_put(&rd->rd_kref, rpcrdma_rn_release);
+out_unlock:
+	mutex_unlock(&rpcrdma_device_mutex);
 }
 
 /**
@@ -186,15 +213,30 @@ static void rpcrdma_remove_one(struct ib_device *device,
 {
 	struct rpcrdma_device *rd = client_data;
 	struct rpcrdma_notification *rn;
+	void (*done)(struct rpcrdma_notification *rn);
+	void (*put)(struct rpcrdma_notification_ref *ref);
 	unsigned long index;
 
 	trace_rpcrdma_client_remove_one(device);
 
 	mutex_lock(&rpcrdma_device_mutex);
 	set_bit(RPCRDMA_RD_F_REMOVING, &rd->rd_flags);
+	xa_for_each(&rd->rd_xa, index, rn) {
+		struct rpcrdma_notification_ref ref = {};
+		bool notify;
+
+		notify = rn->rn_get(rn, &ref);
+		if (!ref.rr_owner && !ref.rr_xprt)
+			continue;
+		done = notify ? rn->rn_done : NULL;
+		put = rn->rn_put;
+		mutex_unlock(&rpcrdma_device_mutex);
+		if (done)
+			done(rn);
+		put(&ref);
+		mutex_lock(&rpcrdma_device_mutex);
+	}
 	mutex_unlock(&rpcrdma_device_mutex);
-	xa_for_each(&rd->rd_xa, index, rn)
-		rn->rn_done(rn);
 
 	/*
 	 * Wait only if there are still outstanding notification
diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index f949601b2144..ff3545570859 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -424,6 +424,24 @@ static void svc_rdma_xprt_done(struct rpcrdma_notification *rn)
 	svc_xprt_close(&rdma->sc_xprt);
 }
 
+static void svc_rdma_xprt_put(struct rpcrdma_notification_ref *ref)
+{
+	if (ref->rr_owner)
+		svc_xprt_put(ref->rr_owner);
+}
+
+static bool svc_rdma_xprt_get(struct rpcrdma_notification *rn,
+			      struct rpcrdma_notification_ref *ref)
+{
+	struct svcxprt_rdma *rdma = container_of(rn, struct svcxprt_rdma,
+						 sc_rn);
+
+	if (!kref_get_unless_zero(&rdma->sc_xprt.xpt_ref))
+		return false;
+	ref->rr_owner = &rdma->sc_xprt;
+	return true;
+}
+
 /*
  * This is the xpo_recvfrom function for listening endpoints. Its
  * purpose is to accept incoming connections. The CMA callback handler
@@ -464,7 +482,8 @@ static struct svc_xprt *svc_rdma_accept(struct svc_xprt *xprt)
 	dev = newxprt->sc_cm_id->device;
 	newxprt->sc_port_num = newxprt->sc_cm_id->port_num;
 
-	if (rpcrdma_rn_register(dev, &newxprt->sc_rn, svc_rdma_xprt_done))
+	if (rpcrdma_rn_register(dev, &newxprt->sc_rn, svc_rdma_xprt_done,
+				svc_rdma_xprt_get, svc_rdma_xprt_put))
 		goto errout;
 
 	newxprt->sc_max_req_size = svcrdma_max_req_size;
diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
index 54957295a631..69d1796bb4f3 100644
--- a/net/sunrpc/xprtrdma/verbs.c
+++ b/net/sunrpc/xprtrdma/verbs.c
@@ -297,6 +297,35 @@ static void rpcrdma_ep_removal_done(struct rpcrdma_notification *rn)
 	xprt_force_disconnect(ep->re_xprt);
 }
 
+static void rpcrdma_ep_removal_put(struct rpcrdma_notification_ref *ref)
+{
+	struct rpcrdma_ep *ep = ref->rr_owner;
+	struct rpc_xprt *xprt = ref->rr_xprt;
+
+	if (ep) {
+		struct rdma_cm_id *id = ep->re_id;
+
+		if (rpcrdma_ep_put(ep))
+			rdma_destroy_id(id);
+	}
+	xprt_put(xprt);
+}
+
+static bool rpcrdma_ep_removal_get(struct rpcrdma_notification *rn,
+				   struct rpcrdma_notification_ref *ref)
+{
+	struct rpcrdma_ep *ep = container_of(rn, struct rpcrdma_ep, re_rn);
+	struct rpc_xprt *xprt = ep->re_xprt;
+
+	if (!xprt_get(xprt))
+		return false;
+	ref->rr_xprt = xprt;
+	if (!kref_get_unless_zero(&ep->re_kref))
+		return false;
+	ref->rr_owner = ep;
+	return true;
+}
+
 static struct rdma_cm_id *rpcrdma_create_id(struct rpcrdma_xprt *r_xprt,
 					    struct rpcrdma_ep *ep)
 {
@@ -337,7 +366,10 @@ static struct rdma_cm_id *rpcrdma_create_id(struct rpcrdma_xprt *r_xprt,
 		goto out;
 
 	ep->re_id = id;
-	rc = rpcrdma_rn_register(id->device, &ep->re_rn, rpcrdma_ep_removal_done);
+	rc = rpcrdma_rn_register(id->device, &ep->re_rn,
+				 rpcrdma_ep_removal_done,
+				 rpcrdma_ep_removal_get,
+				 rpcrdma_ep_removal_put);
 	if (rc)
 		goto out;
 
-- 
2.34.1

[PATCH v2 3/3] xprtrdma: Wake connect worker on device removal

[Li Xiasong]

Device removal only forces a transport disconnect, leaving a connect
worker asleep while re_connect_status is zero and blocking endpoint
cleanup.

Set -ENODEV and wake pending connects on removal. Force established
connections to disconnect and use atomic transitions to prevent late
ESTABLISHED events from overwriting errors. Handle REJECTED after
establishment with an atomic status update and forced disconnect, retaining
its errno and unconditional wakeup without relying on CQ flushes.

The final endpoint put can free id->context before rdma_destroy_id()
synchronizes with CM callbacks. Destroy the CM ID in the endpoint
destructor before freeing its context to close this window. Remove the
CM callback get/put pairs so final puts occur outside CM callbacks.
Initialize the connect waitqueue before notification registration.

If transport pinning fails, fail and wake a pending connect under the
device mutex before skipping the notification. The final transport put
can be waiting for that worker to release XPRT_LOCKED.

Fixes: 3f4eb9ff9234 ("xprtrdma: Handle device removal outside of the CM event handler")
Signed-off-by: Li Xiasong <lixiasong1@huawei.com>
---
 net/sunrpc/xprtrdma/verbs.c | 94 ++++++++++++++++++++++---------------
 1 file changed, 56 insertions(+), 38 deletions(-)

diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
index 69d1796bb4f3..9eec462cffd9 100644
--- a/net/sunrpc/xprtrdma/verbs.c
+++ b/net/sunrpc/xprtrdma/verbs.c
@@ -75,7 +75,7 @@ static void rpcrdma_reps_unmap(struct rpcrdma_xprt *r_xprt);
 static void rpcrdma_mrs_create(struct rpcrdma_xprt *r_xprt);
 static void rpcrdma_mrs_destroy(struct rpcrdma_xprt *r_xprt);
 static void rpcrdma_ep_get(struct rpcrdma_ep *ep);
-static int rpcrdma_ep_put(struct rpcrdma_ep *ep);
+static void rpcrdma_ep_put(struct rpcrdma_ep *ep);
 static struct rpcrdma_regbuf *
 rpcrdma_regbuf_alloc_node(size_t size, enum dma_data_direction direction,
 			  int node);
@@ -124,6 +124,20 @@ void rpcrdma_force_disconnect(struct rpcrdma_ep *ep)
 		xprt_force_disconnect(ep->re_xprt);
 }
 
+static void rpcrdma_ep_disconnect(struct rpcrdma_ep *ep, int status)
+{
+	switch (cmpxchg(&ep->re_connect_status, 0, status)) {
+	case 0:
+		wake_up_all(&ep->re_connect_wait);
+		break;
+	case 1:
+		rpcrdma_force_disconnect(ep);
+		break;
+	default:
+		break;
+	}
+}
+
 /**
  * rpcrdma_flush_disconnect - Disconnect on flushed completion
  * @r_xprt: transport to disconnect
@@ -222,13 +236,14 @@ static void rpcrdma_update_cm_private(struct rpcrdma_ep *ep,
  * @id: rdma_cm_id on which an event has occurred
  * @event: details of the event
  *
- * Called with @id's mutex held. Returns 1 if caller should
- * destroy @id, otherwise 0.
+ * Called with @id's mutex held. Always returns 0: the endpoint owner
+ * destroys @id and waits for these callbacks before freeing @id's context.
  */
 static int
 rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 {
 	struct rpcrdma_ep *ep = id->context;
+	int status;
 
 	might_sleep();
 
@@ -247,20 +262,18 @@ rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 		complete(&ep->re_done);
 		return 0;
 	case RDMA_CM_EVENT_ADDR_CHANGE:
-		switch (xchg(&ep->re_connect_status, -ENODEV)) {
+		rpcrdma_ep_disconnect(ep, -ENODEV);
+		return 0;
+	case RDMA_CM_EVENT_ESTABLISHED:
+		switch (cmpxchg(&ep->re_connect_status, 0, 1)) {
 		case 0:
-			goto wake_connect_worker;
-		case 1:
-			/* The later DISCONNECTED event balances the
-			 * ESTABLISHED get; do not put here.
-			 */
+			break;
+		case -ENODEV:
 			rpcrdma_force_disconnect(ep);
 			return 0;
+		default:
+			return 0;
 		}
-		return 0;
-	case RDMA_CM_EVENT_ESTABLISHED:
-		rpcrdma_ep_get(ep);
-		ep->re_connect_status = 1;
 		rpcrdma_update_cm_private(ep, &event->param.conn);
 		trace_xprtrdma_inline_thresh(ep);
 		wake_up_all(&ep->re_connect_wait);
@@ -269,19 +282,21 @@ rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 		ep->re_connect_status = -ENOTCONN;
 		goto wake_connect_worker;
 	case RDMA_CM_EVENT_UNREACHABLE:
-		ep->re_connect_status = -ENETUNREACH;
-		goto wake_connect_worker;
+		rpcrdma_ep_disconnect(ep, -ENETUNREACH);
+		return 0;
 	case RDMA_CM_EVENT_REJECTED:
-		ep->re_connect_status = -ECONNREFUSED;
+		status = -ECONNREFUSED;
 		if (event->status == IB_CM_REJ_STALE_CONN)
-			ep->re_connect_status = -ENOTCONN;
+			status = -ENOTCONN;
+		if (xchg(&ep->re_connect_status, status) == 1)
+			rpcrdma_force_disconnect(ep);
 wake_connect_worker:
 		wake_up_all(&ep->re_connect_wait);
 		return 0;
 	case RDMA_CM_EVENT_DISCONNECTED:
-		ep->re_connect_status = -ECONNABORTED;
+		xchg(&ep->re_connect_status, -ECONNABORTED);
 		rpcrdma_force_disconnect(ep);
-		return rpcrdma_ep_put(ep);
+		return 0;
 	default:
 		break;
 	}
@@ -294,7 +309,7 @@ static void rpcrdma_ep_removal_done(struct rpcrdma_notification *rn)
 	struct rpcrdma_ep *ep = container_of(rn, struct rpcrdma_ep, re_rn);
 
 	trace_xprtrdma_device_removal(ep->re_id);
-	xprt_force_disconnect(ep->re_xprt);
+	rpcrdma_ep_disconnect(ep, -ENODEV);
 }
 
 static void rpcrdma_ep_removal_put(struct rpcrdma_notification_ref *ref)
@@ -302,12 +317,8 @@ static void rpcrdma_ep_removal_put(struct rpcrdma_notification_ref *ref)
 	struct rpcrdma_ep *ep = ref->rr_owner;
 	struct rpc_xprt *xprt = ref->rr_xprt;
 
-	if (ep) {
-		struct rdma_cm_id *id = ep->re_id;
-
-		if (rpcrdma_ep_put(ep))
-			rdma_destroy_id(id);
-	}
+	if (ep)
+		rpcrdma_ep_put(ep);
 	xprt_put(xprt);
 }
 
@@ -317,8 +328,15 @@ static bool rpcrdma_ep_removal_get(struct rpcrdma_notification *rn,
 	struct rpcrdma_ep *ep = container_of(rn, struct rpcrdma_ep, re_rn);
 	struct rpc_xprt *xprt = ep->re_xprt;
 
-	if (!xprt_get(xprt))
+	if (!xprt_get(xprt)) {
+		/* The final xprt_put() can be waiting for the connect worker's
+		 * XPRT_LOCKED bit. The device mutex protects @ep until
+		 * unregister. Wake pending connects without using the transport.
+		 */
+		if (cmpxchg(&ep->re_connect_status, 0, -ENODEV) == 0)
+			wake_up_all(&ep->re_connect_wait);
 		return false;
+	}
 	ref->rr_xprt = xprt;
 	if (!kref_get_unless_zero(&ep->re_kref))
 		return false;
@@ -383,6 +401,7 @@ static struct rdma_cm_id *rpcrdma_create_id(struct rpcrdma_xprt *r_xprt,
 static void rpcrdma_ep_destroy(struct kref *kref)
 {
 	struct rpcrdma_ep *ep = container_of(kref, struct rpcrdma_ep, re_kref);
+	struct ib_device *device = ep->re_id->device;
 
 	if (ep->re_id->qp) {
 		rdma_destroy_qp(ep->re_id);
@@ -400,7 +419,11 @@ static void rpcrdma_ep_destroy(struct kref *kref)
 		ib_dealloc_pd(ep->re_pd);
 	ep->re_pd = NULL;
 
-	rpcrdma_rn_unregister(ep->re_id->device, &ep->re_rn);
+	/* All puts occur outside CM callbacks. Fence both queued and running
+	 * callbacks before freeing the context, even if no connection formed.
+	 */
+	rdma_destroy_id(ep->re_id);
+	rpcrdma_rn_unregister(device, &ep->re_rn);
 
 	kfree(ep);
 	module_put(THIS_MODULE);
@@ -411,13 +434,10 @@ static noinline void rpcrdma_ep_get(struct rpcrdma_ep *ep)
 	kref_get(&ep->re_kref);
 }
 
-/* Returns:
- *     %0 if @ep still has a positive kref count, or
- *     %1 if @ep was destroyed successfully.
- */
-static noinline int rpcrdma_ep_put(struct rpcrdma_ep *ep)
+/* Must not be called from an RDMA CM event handler. */
+static noinline void rpcrdma_ep_put(struct rpcrdma_ep *ep)
 {
-	return kref_put(&ep->re_kref, rpcrdma_ep_destroy);
+	kref_put(&ep->re_kref, rpcrdma_ep_destroy);
 }
 
 static int rpcrdma_ep_create(struct rpcrdma_xprt *r_xprt)
@@ -433,6 +453,7 @@ static int rpcrdma_ep_create(struct rpcrdma_xprt *r_xprt)
 		return -ENOTCONN;
 	ep->re_xprt = &r_xprt->rx_xprt;
 	kref_init(&ep->re_kref);
+	init_waitqueue_head(&ep->re_connect_wait);
 
 	id = rpcrdma_create_id(r_xprt, ep);
 	if (IS_ERR(id)) {
@@ -460,7 +481,6 @@ static int rpcrdma_ep_create(struct rpcrdma_xprt *r_xprt)
 
 	ep->re_send_batch = ep->re_max_requests >> 3;
 	ep->re_send_count = ep->re_send_batch;
-	init_waitqueue_head(&ep->re_connect_wait);
 
 	ep->re_attr.send_cq = ib_alloc_cq_any(device, r_xprt,
 					      ep->re_attr.cap.max_send_wr,
@@ -528,7 +548,6 @@ static int rpcrdma_ep_create(struct rpcrdma_xprt *r_xprt)
 
 out_destroy:
 	rpcrdma_ep_put(ep);
-	rdma_destroy_id(id);
 	return rc;
 }
 
@@ -632,8 +651,7 @@ void rpcrdma_xprt_disconnect(struct rpcrdma_xprt *r_xprt)
 	rpcrdma_reqs_reset(r_xprt);
 	rpcrdma_mrs_destroy(r_xprt);
 
-	if (rpcrdma_ep_put(ep))
-		rdma_destroy_id(id);
+	rpcrdma_ep_put(ep);
 
 	r_xprt->rx_ep = NULL;
 }
-- 
2.34.1

Re: [PATCH v2 3/3] xprtrdma: Wake connect worker on device removal

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] xprtrdma: rpcrdma_cm_event_handler() forces disconnect on unpinned xprt
- [High] xprtrdma: rpcrdma_ep_disconnect() fails to update established status
--

commit d1bbd79b2434442573f7ee7831bc287ea9096c3a
Author: Li Xiasong <lixiasong1@huawei.com>

xprtrdma: Wake connect worker on device removal

This commit updates endpoint connection status transitions and CM ID teardown
during RDMA device removal and CM events so that pending connect workers are
woken and endpoint destruction synchronizes with CM callbacks.

> diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
> index 69d1796bb4f3b..9eec462cffd92 100644
> --- a/net/sunrpc/xprtrdma/verbs.c
> +++ b/net/sunrpc/xprtrdma/verbs.c
> @@ -75,7 +75,7 @@ static void rpcrdma_reps_unmap(struct rpcrdma_xprt *r_xprt);
>  static void rpcrdma_mrs_create(struct rpcrdma_xprt *r_xprt);
>  static void rpcrdma_mrs_destroy(struct rpcrdma_xprt *r_xprt);
>  static void rpcrdma_ep_get(struct rpcrdma_ep *ep);
> -static int rpcrdma_ep_put(struct rpcrdma_ep *ep);
> +static void rpcrdma_ep_put(struct rpcrdma_ep *ep);
>  static struct rpcrdma_regbuf *
>  rpcrdma_regbuf_alloc_node(size_t size, enum dma_data_direction direction,
>  			  int node);
> @@ -124,6 +124,20 @@ void rpcrdma_force_disconnect(struct rpcrdma_ep *ep)
>  		xprt_force_disconnect(ep->re_xprt);
>  }
>  
> +static void rpcrdma_ep_disconnect(struct rpcrdma_ep *ep, int status)
> +{
> +	switch (cmpxchg(&ep->re_connect_status, 0, status)) {
> +	case 0:
> +		wake_up_all(&ep->re_connect_wait);
> +		break;
> +	case 1:
> +		rpcrdma_force_disconnect(ep);
> +		break;
> +	default:
> +		break;
> +	}
> +}

[Severity: High]
When ep->re_connect_status is already 1 (established), cmpxchg(&ep->re_connect_status, 0, status) in rpcrdma_ep_disconnect() returns 1 without updating ep->re_connect_status to status.

Unlike the previous xchg(&ep->re_connect_status, -ENODEV) in RDMA_CM_EVENT_ADDR_CHANGE (and the xchg() calls used for RDMA_CM_EVENT_REJECTED and RDMA_CM_EVENT_DISCONNECTED), calling rpcrdma_ep_disconnect() from rpcrdma_ep_removal_done(), RDMA_CM_EVENT_ADDR_CHANGE, or RDMA_CM_EVENT_UNREACHABLE leaves ep->re_connect_status set to 1.

If RDMA_CM_EVENT_ESTABLISHED transitions ep->re_connect_status to 1 and wakes ep->re_connect_wait, and then rpcrdma_ep_disconnect() (or rpcrdma_ep_removal_get() when xprt_get() fails) runs before rpcrdma_xprt_connect() checks the status:

net/sunrpc/xprtrdma/verbs.c:rpcrdma_xprt_connect() {
	...
	wait_event_interruptible(ep->re_connect_wait,
				 ep->re_connect_status != 0);
	if (ep->re_connect_status <= 0) {
		rc = ep->re_connect_status;
		goto out;
	}
	...
}

won't rpcrdma_xprt_connect() observe ep->re_connect_status == 1 and proceed to allocate and post send contexts, receive buffers, and MRs on the disconnecting or removing device while holding XPRT_LOCKED?

Leaving ep->re_connect_status == 1 also allows rpcrdma_mrs_refresh() to continue queueing buf->rb_refresh_worker until xprt_autoclose() runs:

net/sunrpc/xprtrdma/verbs.c:rpcrdma_mrs_refresh() {
	...
	if (ep->re_connect_status != 1)
		return;
	queue_work(system_highpri_wq, &buf->rb_refresh_worker);
}

Should rpcrdma_ep_disconnect() update ep->re_connect_status when transitioning from 1 as well (for example, using xchg() or a second transition from 1 to status)?

[ ... ]

> @@ -247,20 +262,18 @@ rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
>  		complete(&ep->re_done);
>  		return 0;
>  	case RDMA_CM_EVENT_ADDR_CHANGE:
> -		switch (xchg(&ep->re_connect_status, -ENODEV)) {
> +		rpcrdma_ep_disconnect(ep, -ENODEV);
> +		return 0;
> +	case RDMA_CM_EVENT_ESTABLISHED:
> +		switch (cmpxchg(&ep->re_connect_status, 0, 1)) {
>  		case 0:
> -			goto wake_connect_worker;
> -		case 1:
> -			/* The later DISCONNECTED event balances the
> -			 * ESTABLISHED get; do not put here.
> -			 */
> +			break;
> +		case -ENODEV:
>  			rpcrdma_force_disconnect(ep);
>  			return 0;
> +		default:
> +			return 0;
>  		}

[Severity: Critical]
As reported by Sashiko on v1 [1], when the final xprt_put() has already dropped xprt->kref to 0 and xprt_destroy() is waiting on XPRT_LOCKED, rpcrdma_ep_removal_get() sees !xprt_get(xprt) and sets ep->re_connect_status to -ENODEV via cmpxchg() to wake the connect worker without touching the unpinned xprt:

net/sunrpc/xprtrdma/verbs.c:rpcrdma_ep_removal_get() {
	if (!xprt_get(xprt)) {
		if (cmpxchg(&ep->re_connect_status, 0, -ENODEV) == 0)
			wake_up_all(&ep->re_connect_wait);
		return false;
	}
}

If RDMA_CM_EVENT_ESTABLISHED arrives in rpcrdma_cm_event_handler() before the connect worker finishes rpcrdma_xprt_disconnect(), cmpxchg(&ep->re_connect_status, 0, 1) returns -ENODEV and executes:

	rpcrdma_force_disconnect(ep);
	  -> xprt_force_disconnect(ep->re_xprt);
	    -> xprt_schedule_autoclose_locked(xprt);

which sets XPRT_CLOSE_WAIT on the unpinned xprt whose kref is already 0.

When xprt_rdma_connect_worker() subsequently unlocks the transport via xprt_unlock_connect() -> xprt_release_xprt_cong() -> xprt_clear_locked(), seeing XPRT_CLOSE_WAIT causes xprt_clear_locked() to queue xprt->task_cleanup (xprt_autoclose) instead of clearing XPRT_LOCKED. Once xprt_autoclose() calls xprt_release_write() and clears XPRT_LOCKED, xprt_destroy() wakes from wait_on_bit_lock(), reinitializes xprt->task_cleanup with INIT_WORK(&xprt->task_cleanup, xprt_destroy_cb), and frees xprt while xprt_autoclose() is still finishing.

Since ep->re_connect_status was never 1 when cmpxchg(&ep->re_connect_status, 0, 1) observes -ENODEV, rpcrdma_xprt_connect() is already guaranteed to see ep->re_connect_status <= 0 and trigger rpcrdma_xprt_disconnect() from xprt_rdma_connect_worker(). Can the -ENODEV case in RDMA_CM_EVENT_ESTABLISHED simply return 0 without calling rpcrdma_force_disconnect(ep)?

[1] https://lore.kernel.org/r/20261023125033.528835-4-lixiasong1@huawei.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103032758.1302493-1-lixiasong1@huawei.com?part=3

Re: [PATCH v2 1/3] rpcrdma: Serialize notification registration with device removal

[Chuck Lever]

On Tue, 03 Nov 2026 11:27:56 +0800, Li Xiasong wrote:

> +	mutex_lock(&rpcrdma_device_mutex);
> +	rd = rpcrdma_get_client_data(device);
>  	if (!rd || test_bit(RPCRDMA_RD_F_REMOVING, &rd->rd_flags))
> -		return -ENETUNREACH;
> +		goto out_unlock;
>
>  	/*
>  	 * Arm rn_done before xa_alloc() publishes @rn: once @rn is
> @@ -78,11 +89,15 @@ int rpcrdma_rn_register(struct ib_device *device,
>  	rn->rn_done = done;
>  	if (xa_alloc(&rd->rd_xa, &rn->rn_index, rn, xa_limit_32b, GFP_KERNEL) < 0) {

This xa_alloc() now runs with rpcrdma_device_mutex held, and the
mutex is module-wide. Every xprtrdma connect, every svcrdma accept,
and rpcrdma_remove_one() take it. GFP_KERNEL here can enter direct
reclaim with the mutex held.

Before this patch, a connect worker that blocked in that wait
could wait only on its own transport, and every other
transport reconnected independently. With the mutex held across
the allocation, a connect worker for transport A that is waiting in
reclaim on an RPC over disconnected transport B blocks transport
B's reconnect in rpcrdma_rn_register(). The same holds for an nfsd
thread in svc_rdma_accept() on a host that also runs an NFS client.
Device removal queues behind the same mutex.

The mutex does not need to cover the allocation. xa_reserve() or
xa_insert() with a slot reserved before mutex_lock(), or
memalloc_noio_save() around the critical section, keeps the
serialization this patch needs without holding the lock across
reclaim.


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

Re: [PATCH v2 2/3] rpcrdma: Pin notifications during removal callbacks

[Chuck Lever]

On Tue, 03 Nov 2026 11:27:57 +0800, Li Xiasong wrote:
> rpcrdma_remove_one() can race with unregister and invoke a notification
> callback after its owner is freed. XArray RCU does not pin stored objects.
>
> Pin notification owners under rpcrdma_device_mutex and serialize
> unregister with the same lock. Also pin client RPC transports. Skip
> owners in final release with kref_get_unless_zero().

> +	xa_for_each(&rd->rd_xa, index, rn) {
> +		struct rpcrdma_notification_ref ref = {};
> +		bool notify;
> +
> +		notify = rn->rn_get(rn, &ref);
> +		if (!ref.rr_owner && !ref.rr_xprt)
> +			continue;
> +		done = notify ? rn->rn_done : NULL;
> +		put = rn->rn_put;
> +		mutex_unlock(&rpcrdma_device_mutex);
> +		if (done)
> +			done(rn);
> +		put(&ref);
> +		mutex_lock(&rpcrdma_device_mutex);
> +	}

I wonder whether the owner pin is needed once unregister takes
the mutex. Both owners free their object only after
rpcrdma_rn_unregister() returns, so a callback invoked with the
mutex held cannot see a freed owner. Neither rn_done sleeps or
takes the mutex: svc_rdma_xprt_done() sets XPT_CLOSE and
enqueues, and after patch 3 rpcrdma_ep_removal_done() does a
cmpxchg and a wake, or xprt_force_disconnect() under
transport_lock. Invoking rn_done under the mutex would remove
rn_get, rn_put, kref_get_unless_zero() on the owner, the
rr_owner half of struct rpcrdma_notification_ref, and the
unlock/relock around each callback. The contract would then be
that rn_done must not sleep and must not call
rpcrdma_rn_unregister().

The transport pin is a different matter. xprt_force_disconnect()
on a transport that is inside xprt_destroy() queues task_cleanup
as autoclose while xprt_destroy() re-initializes that work for
xprt_destroy_cb(), so rpcrdma_ep_removal_done() does need to know
whether the transport is in its final put, and the matching
xprt_put() has to happen outside the mutex. Neither requirement
involves svcrdma, which leaves rr_xprt NULL, and the header now forward-
declares struct rpc_xprt for one user. Could the transport pin
live in verbs.c, with the generic API carrying only rn_done?


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