Re: [RFC PATCH 6.6.y] xprtrdma: Fix endpoint leak after address change and device removal

Re: [RFC PATCH 6.6.y] xprtrdma: Fix endpoint leak after address change and device removal

[sashiko-bot]

> Commit a562485c359d ("xprtrdma: Fix ep kref imbalance on ADDR_CHANGE")
> is the linux-6.6.y backport of af9b65b29af34. It leaves the ESTABLISHED
> reference outstanding after ADDR_CHANGE and relies on a subsequent
> DISCONNECTED event to release it.
> 
> However, linux-6.6.y still handles DEVICE_REMOVAL in the CM event
> handler. Consider the following sequence on an established connection:
> 
>   ESTABLISHED:    acquire the connection reference, status = 1
>   ADDR_CHANGE:    set status = -ENODEV and request asynchronous close
>   DEVICE_REMOVAL: observe status = -ENODEV, skip the disconnected path
> 
> Before invoking the DEVICE_REMOVAL callback, the CMA sets the CM ID state
> to RDMA_CM_DEVICE_REMOVAL. cma_ib_handler() then filters subsequent
> IB CM disconnect events. The DEVICE_REMOVAL handler has skipped the
> [ ... ]
> The flag is initially clear in the zero-initialized endpoint, and
> all accesses are serialized by the CM ID's handler mutex.
> 
> Fixes: a562485c359d ("xprtrdma: Fix ep kref imbalance on ADDR_CHANGE")
> Signed-off-by: Li Xiasong <lixiasong1@huawei.com>

Sashiko has reviewed this patch and found no issues.

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

[RFC PATCH 6.6.y] xprtrdma: Fix endpoint leak after address change and device removal

[Li Xiasong]

Commit a562485c359d ("xprtrdma: Fix ep kref imbalance on ADDR_CHANGE")
is the linux-6.6.y backport of af9b65b29af34. It leaves the ESTABLISHED
reference outstanding after ADDR_CHANGE and relies on a subsequent
DISCONNECTED event to release it.

However, linux-6.6.y still handles DEVICE_REMOVAL in the CM event
handler. Consider the following sequence on an established connection:

  ESTABLISHED:    acquire the connection reference, status = 1
  ADDR_CHANGE:    set status = -ENODEV and request asynchronous close
  DEVICE_REMOVAL: observe status = -ENODEV, skip the disconnected path

Before invoking the DEVICE_REMOVAL callback, the CMA sets the CM ID state
to RDMA_CM_DEVICE_REMOVAL. cma_ib_handler() then filters subsequent
IB CM disconnect events. The DEVICE_REMOVAL handler has skipped the
put, and no DISCONNECTED notification is delivered to RPC/RDMA to
release the ESTABLISHED reference.

The drain and owner puts leave one endpoint reference outstanding.
rpcrdma_xprt_disconnect() clears rx_ep without destroying the endpoint
or its CM ID, leaking the endpoint and its remaining RDMA resources.
This can also prevent device removal from completing.

This is a linux-6.6.y-specific backport regression: this branch
combines the updated ADDR_CHANGE handling with the older CM-based
device removal path. Earlier stable branches have not received this
ADDR_CHANGE change, while upstream and stable branches based on
v6.11 or later handle device removal outside the CM event handler.
There is therefore no corresponding upstream commit for this fix.

Track the outstanding ESTABLISHED reference independently of the
connection status. On DEVICE_REMOVAL or DISCONNECTED, clear the
flag before dropping the reference so that it is released at most
once, even if earlier events have changed the connection status.
The flag is initially clear in the zero-initialized endpoint, and
all accesses are serialized by the CM ID's handler mutex.

Fixes: a562485c359d ("xprtrdma: Fix ep kref imbalance on ADDR_CHANGE")
Signed-off-by: Li Xiasong <lixiasong1@huawei.com>
---

This RFC is based on linux-6.6.y at v6.6.161.

The upstream device removal mechanism was introduced by:

  7e86845a0346 ("rpcrdma: Implement generic device removal")
  3f4eb9ff9234 ("xprtrdma: Handle device removal outside of the CM event handler")

I have kept this RFC limited to reference accounting in the existing
linux-6.6.y teardown paths. Backporting the generic mechanism would
also change per-device registration, module initialization and
teardown synchronization, and would require reviewing its dependencies
and follow-up fixes. Keeping the fix local avoids introducing those
additional lifecycle changes into the stable branch.

Would this localized fix be appropriate for linux-6.6.y? I'd appreciate
your feedback on the reference accounting, or any suggestions for a
simpler way to address the regression.

 net/sunrpc/xprtrdma/verbs.c     | 15 ++++++++++-----
 net/sunrpc/xprtrdma/xprt_rdma.h |  2 ++
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/net/sunrpc/xprtrdma/verbs.c b/net/sunrpc/xprtrdma/verbs.c
index 4990e37bcdc0..5b5179aa2adf 100644
--- a/net/sunrpc/xprtrdma/verbs.c
+++ b/net/sunrpc/xprtrdma/verbs.c
@@ -250,17 +250,18 @@ rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 	case RDMA_CM_EVENT_DEVICE_REMOVAL:
 		pr_info("rpcrdma: removing device %s for %pISpc\n",
 			ep->re_id->device->name, sap);
-		switch (xchg(&ep->re_connect_status, -ENODEV)) {
-		case 0: goto wake_connect_worker;
-		case 1: goto disconnected;
-		}
+		if (xchg(&ep->re_connect_status, -ENODEV) == 0)
+			goto wake_connect_worker;
+		/* CMA suppresses subsequent DISCONNECTED events after removal. */
+		if (ep->re_connect_ref)
+			goto disconnected;
 		return 0;
 	case RDMA_CM_EVENT_ADDR_CHANGE:
 		switch (xchg(&ep->re_connect_status, -ENODEV)) {
 		case 0:
 			goto wake_connect_worker;
 		case 1:
-			/* The later DISCONNECTED event balances the
+			/* DISCONNECTED or DEVICE_REMOVAL balances the
 			 * ESTABLISHED get; do not put here.
 			 */
 			rpcrdma_force_disconnect(ep);
@@ -269,6 +270,7 @@ rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 		return 0;
 	case RDMA_CM_EVENT_ESTABLISHED:
 		rpcrdma_ep_get(ep);
+		ep->re_connect_ref = true;
 		ep->re_connect_status = 1;
 		rpcrdma_update_cm_private(ep, &event->param.conn);
 		trace_xprtrdma_inline_thresh(ep);
@@ -291,6 +293,9 @@ rpcrdma_cm_event_handler(struct rdma_cm_id *id, struct rdma_cm_event *event)
 		ep->re_connect_status = -ECONNABORTED;
 disconnected:
 		rpcrdma_force_disconnect(ep);
+		if (!ep->re_connect_ref)
+			return 0;
+		ep->re_connect_ref = false;
 		return rpcrdma_ep_put(ep);
 	default:
 		break;
diff --git a/net/sunrpc/xprtrdma/xprt_rdma.h b/net/sunrpc/xprtrdma/xprt_rdma.h
index c78734e93c71..4a3ce78bb90c 100644
--- a/net/sunrpc/xprtrdma/xprt_rdma.h
+++ b/net/sunrpc/xprtrdma/xprt_rdma.h
@@ -84,6 +84,8 @@ struct rpcrdma_ep {
 	unsigned int		re_max_inline_recv;
 	int			re_async_rc;
 	int			re_connect_status;
+	/* Outstanding ESTABLISHED reference; protected by CM handler mutex */
+	bool			re_connect_ref;
 	atomic_t		re_receiving;
 	atomic_t		re_force_disconnect;
 	struct ib_qp_init_attr	re_attr;
-- 
2.53.0