[PATCH net-next v12 11/12] net/rds: give cm_id->context a reference on its connection

[Allison Henderson <achender@kernel.org>]

rds_rdma_cm_event_handler_cmn() picks the connection up from
cm_id->context and holds c_cm_lock - a mutex that lives in the
connection's path array - across the transport callbacks, but the
context pointer owns no reference.  What keeps the connection alive
under the handler is ordering: rds_ib_conn_path_shutdown() destroys the
id, and rdma_destroy_id() waits for a running handler, before the
destroy drops the connection's initial reference, so no event is
handled on a connection that is being freed.  Before this series that
ordering was broken from inside the handler, since rds_conn_destroy()
freed synchronously and the one callback that destroyed its
connection, rds_ib_cm_connect_complete() on a protocol version below
3.1, left the handler's mutex_unlock() running on freed memory;
commit f97d8c7bab78 ("rds: ib: use rds_conn_drop() on protocol version
mismatch") has since switched that path to rds_conn_drop(), which also
removed the deadlock that destroy took on c_cm_lock.

Now that connections are reference counted, give the id a reference of
its own rather than rely on that ordering.  rds_ib_conn_path_connect()
takes it once rdma_create_id() has installed the connection as the
context, rds_ib_cm_handle_connect() takes it when it installs the id on
the passive side, and each is dropped after rdma_destroy_id() has
returned: in rds_ib_conn_path_shutdown(), and on the two failure paths
of rds_ib_conn_path_connect() that destroy the id straight away.  With
that the handler cannot find its connection freed under it, whatever
the callbacks do to the other holders - the destroy's initial
reference, a socket's cache, a parent's c_passive, an inc.  None of the
new puts can be the last reference: the shutdown runs on behalf of a
destroy, or of a drop on a connection still in the hash, and the
connect worker runs on a connection still in the hash, and each of
those holds a reference of its own.

rds_ib_cm_initiate_connect() and rds_ib_cm_handle_connect() get a
defensive check of the same kind.  A destroy that began while the
address and route were resolving either waits on c_cm_lock behind the
ROUTE_RESOLVED handler - and its shutdown pass then tears down
whatever the handler set up, QP, cm_id and device reference alike - or
has already destroyed the id, in which case no event for it reaches
the handler.  On the passive side a quiesced connection is unhashed,
so rds_conn_create() cannot hand it to a connect request, and the
listeners are stopped before any IB connection is destroyed at unload.
Neither path is reachable today, then; both functions nevertheless
decline to set up a QP, or install a new cm_id, on a connection whose
destroy has begun, so that the contract does not rest on that
ordering.  rds_ib_cm_initiate_connect() returns success when it
declines: the id is still ic->i_cm_id, for the shutdown to destroy,
and a non-zero return would have the rdma_cm destroy it instead.  An
unload that begins after the check has passed is covered by
rds_ib_add_conn() refusing a device that rds_ib_dev_shutdown() has
marked.

Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/ib_cm.c          | 41 ++++++++++++++++++++++++++++++++++++----
 net/rds/rdma_transport.c |  7 +++++++
 2 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 5bf6b22c10d6..a580a8c667ca 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -876,6 +876,15 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id,
 	 * see the comment above rds_queue_reconnect()
 	 */
 	mutex_lock(&conn->c_cm_lock);
+	/* Defensive: a destroy that has quiesced this conn also unhashed
+	 * it, so rds_conn_create() cannot have returned it, and the
+	 * listeners are stopped before any IB connection is destroyed at
+	 * unload.  Should a request reach a conn in RDS_CONN_DOWN with
+	 * its destroy begun all the same, do not install a new cm_id and
+	 * QP that nothing would tear down.
+	 */
+	if (rds_destroy_pending(conn))
+		goto out;
 	if (!rds_conn_transition(conn, RDS_CONN_DOWN, RDS_CONN_CONNECTING)) {
 		if (rds_conn_state(conn) == RDS_CONN_UP) {
 			rdsdebug("incoming connect while connecting\n");
@@ -904,6 +913,10 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id,
 	BUG_ON(ic->i_cm_id);
 
 	ic->i_cm_id = cm_id;
+	/* The id owns a reference on the connection it points at, put
+	 * after rdma_destroy_id() in rds_ib_conn_path_shutdown().
+	 */
+	rds_conn_get(conn);
 	cm_id->context = conn;
 
 	/* We got halfway through setting up the ib_connection, if we
@@ -928,10 +941,8 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id,
 out:
 	if (conn) {
 		mutex_unlock(&conn->c_cm_lock);
-		/* Drop the reference rds_conn_create() handed us.  The
-		 * conn stays reachable through cm_id->context without a
-		 * reference of its own for now; the CM event handler is
-		 * given one of its own by a following patch.
+		/* Drop the reference rds_conn_create() handed us; the
+		 * cm_id holds its own from here on.
 		 */
 		rds_conn_put(conn);
 	}
@@ -950,6 +961,18 @@ int rds_ib_cm_initiate_connect(struct rdma_cm_id *cm_id, bool isv6)
 	union rds_ib_conn_priv dp;
 	int ret;
 
+	/* Defensive: a destroy that began while the address and route
+	 * were being resolved either waits on c_cm_lock behind this
+	 * handler, and its shutdown pass then tears down whatever is
+	 * set up here, or has already destroyed the id this event
+	 * arrived on, in which case the event never gets here.  Decline
+	 * to set up a QP all the same, and return success: the id is
+	 * still ic->i_cm_id, for that shutdown to destroy, and a
+	 * non-zero return would have the rdma_cm destroy it instead.
+	 */
+	if (rds_destroy_pending(conn))
+		return 0;
+
 	/* If the peer doesn't do protocol negotiation, we must
 	 * default to RDSv3.0 */
 	rds_ib_set_protocol(conn, RDS_PROTOCOL_4_1);
@@ -1007,6 +1030,10 @@ int rds_ib_conn_path_connect(struct rds_conn_path *cp)
 		rdsdebug("rdma_create_id() failed: %d\n", ret);
 		goto out;
 	}
+	/* The id owns a reference on the connection it points at, put
+	 * after rdma_destroy_id() wherever the id is destroyed.
+	 */
+	rds_conn_get(conn);
 
 	/* rds_ib_laddr_check() only vouched for the local address being
 	 * on an IB device; the address resolution below picks the device
@@ -1017,6 +1044,7 @@ int rds_ib_conn_path_connect(struct rds_conn_path *cp)
 		rdsdebug("rdma_restrict_node_type() failed: %d\n", ret);
 		rdma_destroy_id(ic->i_cm_id);
 		ic->i_cm_id = NULL;
+		rds_conn_put(conn);
 		goto out;
 	}
 
@@ -1058,6 +1086,7 @@ int rds_ib_conn_path_connect(struct rds_conn_path *cp)
 			 ret);
 		rdma_destroy_id(ic->i_cm_id);
 		ic->i_cm_id = NULL;
+		rds_conn_put(conn);
 	}
 
 out:
@@ -1188,6 +1217,10 @@ void rds_ib_conn_path_shutdown(struct rds_conn_path *cp)
 			rds_ib_recv_clear_ring(ic);
 
 		rdma_destroy_id(ic->i_cm_id);
+		/* The id's reference on the connection.  Never the last
+		 * one: whoever runs this shutdown holds its own.
+		 */
+		rds_conn_put(conn);
 
 		/*
 		 * Move connection back to the nodev list.
diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
index 75532734c054..5d7d41c2db29 100644
--- a/net/rds/rdma_transport.c
+++ b/net/rds/rdma_transport.c
@@ -60,6 +60,13 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
 	rdsdebug("conn %p id %p handling event %u (%s)\n", conn, cm_id,
 		 event->event, rdma_event_msg(event->event));
 
+	/* cm_id->context owns a reference on the connection, dropped
+	 * only after rdma_destroy_id() has returned, and that waits for
+	 * a running handler.  The connection, and the mutex released at
+	 * out: that lives in its path array, therefore outlive this
+	 * handler whatever the callbacks below do to the other holders.
+	 */
+
 	/* Prevent shutdown from tearing down the connection
 	 * while we're executing. */
 	if (conn) {
-- 
2.50.1