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 --- 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