Re: [PATCH net-next v11 11/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 patch adds a pin in rds_rdma_cm_event_handler_cmn()
  (net/rds/rdma_transport.c): `if (conn &&…

--

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

This patch has rds_rdma_cm_event_handler_cmn() take a reference on the
connection found in cm_id->context for each event it handles. If the
reference count is already zero, the event is ignored. It also adds
rds_destroy_pending() checks to rds_ib_cm_initiate_connect() and
rds_ib_cm_handle_connect(), so neither of them sets up a QP or a new
cm_id on a connection whose destroy has begun.

> diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> index 75532734c054b..03c21ce2324c9 100644
> --- a/net/rds/rdma_transport.c
> +++ b/net/rds/rdma_transport.c
> @@ -60,6 +60,21 @@ 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 carries no reference of its own.  Pin the
> +	 * connection for the duration of the handler, since the mutex
> +	 * released at out: lives in the connection's path array.  None
> +	 * of the callbacks below drops a reference on this connection,
> +	 * and the shutdown destroys the cm_id - waiting for a running
> +	 * handler - before the last reference can go, so this is
> +	 * defensive.  A connection already at zero references 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]

Can this zero-reference check actually catch a connection that is going
away?

The conn pointer comes from cm_id->context, which owns no reference.
rds_ib_conn_path_connect() passes conn to rdma_create_id() without taking
a reference. rds_ib_cm_handle_connect() drops its own reference after it
sets cm_id->context.

The final put frees the memory straight away, with no grace period:

net/rds/connection.c:rds_conn_destroy_fini() {
    ...
	kfree(conn->c_path);
	kmem_cache_free(rds_conn_slab, conn);
    ...
}

The slab is also not SLAB_TYPESAFE_BY_RCU:

net/rds/connection.c:rds_conn_init() {
    ...
	rds_conn_slab = KMEM_CACHE(rds_connection, 0);
    ...
}

That means a zero count can only be seen inside rds_conn_destroy_fini(),
between the count reaching zero and the kmem_cache_free() call. After
that point, kref_get_unless_zero() reads conn->c_refcount from freed
memory.

If another rds_connection has already reused the slot, the get succeeds.
The handler would then take that connection's c_cm_lock and apply the
peer's ESTABLISHED, REJECTED, DISCONNECTED or error events to the wrong
connection.

I could not find a current path that frees the connection while its
cm_id can still deliver events. The shutdown pass calls rdma_destroy_id(),
which waits for a running handler, before the initial reference is
dropped. That matches what the commit message says. Still, memory safety
here depends entirely on that ordering. The later patches in the series
don't appear to change this: the handler and the synchronous
kmem_cache_free() are left as they are.

Given that, are the new comment ("A connection already at zero references
gets no events handled") and this part of the commit message accurate?

  "the pin is defensive, keeping the handler correct without leaning on
  that ordering."

If the goal is to stop depending on the destroy-before-free ordering,
would cm_id->context need to own a real reference, released when the
cm_id is destroyed? Another option would be RCU-deferred freeing, or
SLAB_TYPESAFE_BY_RCU with an identity re-check after the get, such as
ic->i_cm_id == cm_id.

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