Re: [PATCH net-next v5 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 more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Stale forward-referencing comment: the ownership comment at the
  `out:` label of rds_ib_cm_handle_connect() (net/rds/ib_cm.c:936-940)…
- [Low] Misattributed trigger in both the commit message and the new in-
  code comment: the message states "the remaining exposure is a callback…

--

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

The RDMA-CM event handler takes a reference on the connection it picks up
from cm_id->context and drops it at the out: label, ignoring the event
entirely if the connection is already on its way out.
rds_ib_cm_handle_connect() also re-checks rds_destroy_pending() under
c_cm_lock and rejects the connect request instead of claiming the
DOWN -> CONNECTING transition.

A couple of questions about the commit message and about a comment that
this patch appears to invalidate.

>     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 - which has none - and the reference handed out by
>     rds_conn_create() to rds_ib_cm_handle_connect() and dropped at its
>     end.

[Severity: Low]
Is there really a callback below the handler that drops a connection
reference?

For most events the handler calls rds_conn_drop(), which ends in
rds_conn_path_drop() in net/rds/connection.c:

	atomic_set(&cp->cp_state, RDS_CONN_ERROR);
	...
	queue_work(cp->cp_wq, &cp->cp_down_w);

That only flips the state and queues the shutdown work, it releases no
reference.  rds_ib_cm_initiate_connect() and rds_ib_cm_connect_complete()
have no rds_conn_put() or rds_conn_destroy() either.  The only
rds_conn_put() in net/rds/ib_cm.c is the one at the end of
rds_ib_cm_handle_connect(), and that path only runs when cm_id->context
was NULL (BUG_ON(cm_id->context) just above it), so the handler's own conn
is NULL there and neither the new reference nor c_cm_lock is in play.

The pin still looks necessary, but for a different reason: a concurrent
rds_conn_destroy() from transport or device teardown can drop the last
reference while the handler is still inside the switch holding c_cm_lock.
For example rds_ib_destroy_nodev_conns() in net/rds/ib_rdma.c:

	rds_conn_destroy(conn);
	rds_conn_put(conn);

rds_conn_shutdown() only takes cp_cm_lock around the state transition, and
skips it when the path is already DOWN, so that thread is not serialized
against a handler sitting in the switch.  Without the new reference the
handler's mutex_unlock(&conn->c_cm_lock) would run on freed memory.

Would it be clearer to describe that path instead?  As written a reader
could look for the callback that drops the reference, not find one, and
conclude the pin is not needed.

> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 786ddcb45bcb..abe5919c368a 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -874,6 +874,13 @@ 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);
> +	/* A destroy that has already quiesced this conn leaves it in
> +	 * RDS_CONN_DOWN with no cm_id, exactly what the transition
> +	 * below would happily claim; nothing would tear the new cm_id
> +	 * and QP down again before the conn is freed.  Reject instead.
> +	 */
> +	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");

[Severity: Low]
Should the comment at the out: label further down in
rds_ib_cm_handle_connect() be updated by this patch?  It currently reads:

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 patch is the "following patch" that comment points at, so from here
on the statement that the CM event handler holds no reference of its own
is no longer true, and the forward reference has nothing left to look
forward to.  The comment is still unchanged at the tip of the series, so
the two files end up describing contradictory reference rules for CM
event handling.

> diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> index b15cf316b23a..584e9867810f 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
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Low]
Same question here as on the changelog: which callback below drops a
connection reference?  rds_conn_drop() only queues cp_down_w, and neither
rds_ib_cm_initiate_connect() nor rds_ib_cm_connect_complete() calls
rds_conn_put().  Would naming the concurrent rds_conn_destroy() path,
such as rds_ib_destroy_nodev_conns(), describe what this pin protects
against?

> +	 * 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/20260919061149.250658-1-achender%40kernel.org