Re: [PATCH net-next v6 10/12] 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: 0 · Medium: 1 · Low: 0

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

- [Medium] The rationale attached to the new reference pin in
  rds_rdma_cm_event_handler_cmn() (net/rds/rdma_transport.c:66-72) and…

--

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

This takes a reference on the connection found via cm_id->context for the
duration of rds_rdma_cm_event_handler_cmn(), ignoring the event if the
connection is already being freed, and re-checks rds_destroy_pending()
under c_cm_lock in rds_ib_cm_handle_connect() so a connect request cannot
install a new cm_id and QP on a connection that is only waiting for its
last reference.

> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 786ddcb45bcbf..1b5491598433b 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c

[ ... ]

> @@ -928,8 +935,8 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id,
>  		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.
> +		 * reference of its own; rds_rdma_cm_event_handler_cmn()
> +		 * takes one for the duration of each event it handles.
>  		 */
>  		rds_conn_put(conn);
>  	}
> 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;
>  
> +	/* 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: Medium]

The get_unless_zero/put pair itself looks balanced, but is the stated
reason for it accurate?  The comment says "what the callbacks below do may
drop the last reference other than ours", and the changelog says the same:

    Now that a connection is freed by its last reference, the remaining
    exposure is a callback that drops the last reference other than the
    handler's

Walking the callbacks dispatched from the switch below, none of them puts
a reference on the connection the handler just pinned.

cm_connect_complete -> rds_ib_cm_connect_complete() only drops the
connection on a version mismatch, and after commit f97d8c7bab78 that is
rds_conn_drop(), which takes no reference action:

net/rds/connection.c:rds_conn_path_drop() {
	atomic_set(&cp->cp_state, RDS_CONN_ERROR);

	rcu_read_lock();
	if (!destroy && rds_destroy_pending(cp->cp_conn)) {
		rcu_read_unlock();
		return;
	}
	queue_work(cp->cp_wq, &cp->cp_down_w);
	rcu_read_unlock();
}

cm_initiate_connect -> rds_ib_cm_initiate_connect() contains no put, and
the remaining switch arms only call rds_conn_drop().

cm_handle_connect -> rds_ib_cm_handle_connect() does call rds_conn_put(),
but that releases the reference rds_conn_create() handed to that same
function, and it runs after its own mutex_unlock(&conn->c_cm_lock).  On
RDMA_CM_EVENT_CONNECT_REQUEST the handler's conn is NULL anyway, since the
listener cm_id is created with a NULL context:

net/rds/rdma_transport.c:rds_rdma_listen() {
	cm_id = rdma_create_id(&init_net, handler, NULL,
			       RDMA_PS_TCP, IB_QPT_RC);
}

which is what BUG_ON(cm_id->context) in rds_ib_cm_handle_connect()
asserts, so nothing was pinned on that path.

That leaves holders outside the handler as the ones that can reach zero
concurrently - rds_conn_destroy() dropping the initial reference, a socket
dropping rs_conn, a c_passive drop, or an inc drop - which is a different
invariant than the one written down.  Could the comment and the changelog
name the holder that can actually race here, or say plainly that the pin
is defensive?  As written, a later reader auditing the callbacks will find
no put and may conclude the pin is dead code, and the same wording is what
a backporter would use to decide whether this is a fix or hardening.

I also checked the end of the series (fe1de9d527be84f2ab7fed636cc4915635b09f0d)
and the comment is unchanged there, so no later patch adjusts it.

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