Re: [PATCH net-next v4 10/12] net/rds: pin the connection across RDMA-CM event handling

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 1 (1) · Medium: 0 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The commit message's central premise is false for this tree: it
  claims that rds_ib_cm_connect_complete()'s protocol-version-mismatch…
- [Low] Stale forward-reference comment: net/rds/ib_cm.c:936-940 (out:
  block of rds_ib_cm_handle_connect()) still says the conn "stays…

Pre-existing issues:
- [High] rds_rdma_cm_event_handler_cmn() declares `struct rds_transport
  *trans;` with no initializer and assigns it only for InfiniBand…

--

net/rds: pin the connection across RDMA-CM event handling

The RDMA-CM event handler takes a reference on the connection it picked
up from cm_id->context for the duration of the callback dispatch, and
ignores the event when the connection is already being freed.
rds_ib_cm_handle_connect() additionally re-checks rds_destroy_pending()
under c_cm_lock and rejects the incoming request in that case.

Two questions on the code and one on a quoted paragraph of the commit
message below.

net/rds: pin the connection across RDMA-CM event handling

rds_rdma_cm_event_handler_cmn() picks the connection up from
cm_id->context, which carries no reference, and holds c_cm_lock - a
mutex that lives in the connection's path array - across the transport
callbacks.  Before this series that was already a use-after-free
whenever a callback destroyed the connection, since rds_conn_destroy()
freed it synchronously and the handler's mutex_unlock() ran on freed
memory; the one such callback, rds_ib_cm_connect_complete() on a
protocol version below 3.1, has meanwhile been switched to
rds_conn_drop() by commit f97d8c7bab78 ("rds: ib: use rds_conn_drop()
on protocol version mismatch"), which also removed the deadlock that
destroy took on c_cm_lock.

[Severity: High]
Which tree does commit f97d8c7bab78 ("rds: ib: use rds_conn_drop() on
protocol version mismatch") come from?  I cannot find it in the baseline
or anywhere in this series, and rds_ib_cm_connect_complete() still calls
rds_conn_destroy() on the version-mismatch path, both at this commit and
at the end of the series:

net/rds/ib_cm.c:rds_ib_cm_connect_complete() {
	...
	if (conn->c_version < RDS_PROTOCOL_VERSION) {
		if (conn->c_version != RDS_PROTOCOL_COMPAT_VERSION) {
			pr_notice("RDS/IB: Connection <%pI6c,%pI6c> version %u.%u no longer supported\n",
				  ...);
			rds_conn_destroy(conn);
			return;
		}
	}
	...
}

If that call site is still there, is the deadlock the paragraph says was
removed still reachable?  The handler dispatches this callback with
c_cm_lock held:

rds_rdma_cm_event_handler_cmn()
    mutex_lock(&conn->c_cm_lock);
    case RDMA_CM_EVENT_ESTABLISHED:
        trans->cm_connect_complete(conn, event);   /* rds_ib_cm_connect_complete() */
            rds_conn_destroy(conn)
                rds_conn_path_quiesce()
                    rds_conn_path_drop(cp, true);  /* forces RDS_CONN_ERROR, queues cp_down_w */
                    flush_work(&cp->cp_down_w);
                        rds_shutdown_worker() -> rds_conn_shutdown()
                            mutex_lock(&cp->cp_cm_lock);   /* held by the flushing thread */

Since c_cm_lock is c_path[0].cp_cm_lock, does the flush_work() in
rds_conn_path_quiesce() wait for a shutdown worker that blocks on the
mutex the same thread is holding?  rds_conn_shutdown() cannot take the
RDS_CONN_DOWN -> RDS_CONN_DOWN shortcut here because
rds_conn_path_drop(cp, true) has just set the state to RDS_CONN_ERROR.

The version bytes come from the peer's CM private data, so a remote
endpoint advertising a version below 3.1 (and different from the compat
version) selects this branch.  Would that hang the RDMA-CM event thread
with c_cm_lock held, leave the connection unfreed, and make rds_ib
unload block in rds_conn_wait_conns_freed()?

If the referenced commit is a prerequisite that is not part of this
posting, could the ordering be stated, or could the destroy-from-callback
site be converted in this series?

[Severity: Low]
This isn't a bug, but the out: block a bit further down in the same
function still points at this change as future work:

net/rds/ib_cm.c:rds_ib_cm_handle_connect() {
	...
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.
		 */
		rds_conn_put(conn);
	}
	...
}

This commit is that following patch, and it does not give cm_id->context
a reference of its own - rds_rdma_cm_event_handler_cmn() takes a
per-event reference and drops it at out:.  Could that comment be updated
to describe the per-event pinning?  It is unchanged at the end of the
series.

[Severity: High]
This isn't a bug introduced by this patch - it reads the same way in the
baseline - but while we are in rds_rdma_cm_event_handler_cmn(): can trans
be used uninitialized here?  It is declared as

	struct rds_transport *trans;

with no initializer, assigned only for RDMA_NODE_IB_CA, and the
CONNECT_REQUEST arm dereferences it unconditionally:

	case RDMA_CM_EVENT_CONNECT_REQUEST:
		ret = trans->cm_handle_connect(cm_id, event, isv6);

The listener is created without a node-type restriction -
rds_rdma_listen_init_common() calls

	cm_id = rdma_create_id(&init_net, handler, NULL,
			       RDMA_PS_TCP, IB_QPT_RC);

and RDS never calls rdma_restrict_node_type(), so id_priv->
restricted_node_type stays RDMA_NODE_UNSPECIFIED and cma_listen_on_dev()
installs the listener on iWARP devices too.  An incoming iWARP request
then goes:

iw_conn_req_handler()          /* event.event = RDMA_CM_EVENT_CONNECT_REQUEST */
    cma_listen_handler()
        rds_rdma_cm_event_handler()
            rds_rdma_cm_event_handler_cmn()

with cm_id->device->node_type == RDMA_NODE_RNIC and cm_id->context NULL,
so the assignment above is skipped, conn stays NULL (the new
rds_conn_get_unless_zero() branch and the c_cm_lock block are both
skipped), and the indirect call goes through an uninitialized stack
value.  The other two arms happen to be guarded by conn.

Would an explicit else that rejects non-IB node types, or initialising
trans to NULL plus a check, be worth adding while this function is being
touched?

+	/* cm_id->context carries no reference of its own.  Pin the
+	 * connection for the duration of the handler: what the callbacks
+	 * below do may drop the last reference other than ours, and the
+	 * mutex released at out: lives in the connection's path array.
+	 * A connection already being freed gets no events handled.
+	 */
+	if (conn && !rds_conn_get_unless_zero(conn)) {
+		rdsdebug("conn %p id %p is being freed, ignoring event\n",
+			 conn, cm_id);
+		return 0;
+	}
+
 	/* Prevent shutdown from tearing down the connection
 	 * while we're executing. */
 	if (conn) {

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260917073958.174056-1-achender%40kernel.org