Re: [PATCH net-next v3 11/13] net/rds: pin the connection across RDMA-CM event handling

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Self-deadlock: rds_ib_cm_connect_complete() destroys the
  connection synchronously from inside the RDMA-CM event callback, while…

Pre-existing issues:
- [High] Uninitialized function-pointer-bearing struct pointer
  dereference in rds_rdma_cm_event_handler_cmn()…

--

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

The patch takes a reference on the connection found in cm_id->context for
the duration of rds_rdma_cm_event_handler_cmn(), ignoring the event when
the connection is already on its way out, and adds an rds_destroy_pending()
re-check under c_cm_lock in rds_ib_cm_handle_connect().  A new helper,
rds_conn_get_unless_zero(), wraps kref_get_unless_zero() on c_refcount.

> diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> index b15cf316b23a2..584e9867810f4 100644
> --- a/net/rds/rdma_transport.c
> +++ b/net/rds/rdma_transport.c
> @@ -63,6 +63,18 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
>  	if (cm_id->device->node_type == RDMA_NODE_IB_CA)
>  		trans = &rds_ib_transport;
                        ^^^^^

[Severity: High]

This isn't a bug introduced by this patch, but since the new hunk lands
directly underneath it: is trans guaranteed to be initialized here?  In
rds_rdma_cm_event_handler_cmn() the declaration is a plain

	struct rds_transport *trans;

and the only assignment is the RDMA_NODE_IB_CA test above, with no else
branch and no bail-out for other node types.  The CONNECT_REQUEST case then
dereferences it unconditionally:

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

On that path conn is NULL (the child cm_id inherits the listener's NULL
context), so none of the "if (conn)" guards cover the use.

Can a non-IB-CA device reach this handler?  RDS's own comment in
rds_ib_laddr_check() says so:

	/* rdma_bind_addr will only succeed for IB & iWARP devices */
	...
	/* due to this, we will claim to support iWARP devices unless we
	   check node_type. */

and rds_ib_add_one() rejects anything that is not RDMA_NODE_IB_CA, while
rds_rdma_listen_init() listens on INADDR_ANY, so the CM fans connect
requests out over every registered device.  With an iWARP or usnic device
present, would a remote connect request to RDS_PORT end up making an
indirect call through an indeterminate stack value?  The kernel build
disables -Wmaybe-uninitialized, so nothing flags it at compile time.

Still present unchanged at the end of the series.

>  
> +	/* 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;
> +	}
> +

[Severity: High]

The commit message says "a callback that drops the last reference other
than the handler's implicit one leaves the final mutex_unlock() running on
freed memory".  Which callback is that?

The one callback that destroys the connection is the ESTABLISHED path, and
on that path control does not seem to reach the new rds_conn_put() at all,
because the destroy blocks on the mutex the handler is still holding:

rds_rdma_cm_event_handler_cmn()
  mutex_lock(&conn->c_cm_lock);	/* == conn->c_path[0].cp_cm_lock */
  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)	/* always 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 caller */

Is this a circular wait?  The handler thread waits in flush_work() for
cp_down_w, and the flushed worker waits for cp_cm_lock, which the handler
thread holds.

rds_conn_shutdown() does enter that block, since rds_conn_path_drop() has
just moved the state to RDS_CONN_ERROR, so the DOWN -> DOWN transition
fails:

net/rds/connection.c:rds_conn_shutdown() {
	if (!rds_conn_path_transition(cp, RDS_CONN_DOWN, RDS_CONN_DOWN)) {
		/*
		 * ... We don't hold the mutex for the entire
		 * duration of the shutdown operation, else we may be
		 * deadlocking with the CM handler. Instead, the CM event
		 * handler is supposed to check for state DISCONNECTING
		 */
		mutex_lock(&cp->cp_cm_lock);
	...
}

The existing comment there states the assumption that the CM handler only
checks DISCONNECTING and returns.  Does the synchronous rds_conn_destroy()
from inside rds_ib_cm_connect_complete() break that assumption?

The trigger looks remote-controlled.  In 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;
		}
	}

A peer accepting with an older advertised version reaches it, and so does a
peer accepting with absent or short private data: the connection is
allocated with kmem_cache_zalloc() and only rds_ib_set_protocol() ever
writes c_version, so c_version stays 0 and compares below
RDS_PROTOCOL_VERSION.

There is a second effect worth checking: the RDMA core holds
id_priv->handler_mutex across the callback, so while the handler thread is
parked in flush_work(), CM events for that cm_id stop and a later
rdma_destroy_id() on it would block as well - and the shutdown worker's
rds_ib_conn_path_shutdown() needs handler_mutex itself.

The synchronous destroy from the callback is older than this patch, but
since that is the very callback the new reference is meant to make safe,
would deferring the destroy out of the CM callback be the change that
actually covers it?  Pinning the connection does not help if the
mutex_unlock() and rds_conn_put() at out: are never reached.

Verified still present at the end of the series.

>  	/* Prevent shutdown from tearing down the connection
>  	 * while we're executing. */
>  	if (conn) {

[ ... ]

> @@ -171,8 +183,10 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
>  	}
>  
>  out:
> -	if (conn)
> +	if (conn) {
>  		mutex_unlock(&conn->c_cm_lock);
> +		rds_conn_put(conn);
> +	}
>  
>  	rdsdebug("id %p event %u (%s) handling ret %d\n", cm_id, event->event,
>  		 rdma_event_msg(event->event), ret);

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