Re: [PATCH net-next v3 04/13] net/rds: make rds_destroy_pending() cover single-connection destroy

[netdev-bot+sashiko@kernel.org]

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

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

- [Medium] The new c_destroy_in_prog + synchronize_rcu() guard only
  covers requeue sites that acquire the struct rds_connection pointer…
- [Low] The two comments this patch adds overstate the invariant they
  document.

Pre-existing issues:
- [High] AB-BA deadlock on the single-connection destroy path that this
  patch is written to protect.
- [Low] The header comment above rds_conn_destroy()
  (net/rds/connection.c, immediately above the function this patch…

--

net/rds: make rds_destroy_pending() cover single-connection destroy

This adds a per-connection c_destroy_in_prog flag, set by
rds_conn_destroy() before the unhash and synchronize_rcu(), and tests it
from rds_destroy_pending() so that the predicate also covers the destroy
of a single connection.  The now-unreferenced RDS_DESTROY_PENDING cp_flags
bit and its dead test in rds_ib_is_unloading() are removed.

No Link: tags in the commit header.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index a96569a3ee9ad..242ca0570a47e 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -579,6 +579,14 @@ void rds_conn_destroy(struct rds_connection *conn)
>  		 "%pI4\n", conn, &conn->c_laddr,
>  		 &conn->c_faddr);
>  
> +	/* Make rds_destroy_pending() true for this conn.  Together with
> +	 * the synchronize_rcu() below this stops the work-requeueing
> +	 * sites (which all test rds_destroy_pending() under
> +	 * rcu_read_lock()) from queueing new work on the path
> +	 * workqueues once we start cancelling and destroying them.
> +	 */
> +	WRITE_ONCE(conn->c_destroy_in_prog, true);
> +

[Severity: Low]
Is the parenthetical "which all test rds_destroy_pending() under
rcu_read_lock()" accurate?  The same claim appears in the new
c_destroy_in_prog comment in net/rds/rds.h ("A site that arms a path work
must test the predicate and queue the work inside one rcu_read_lock()
section") and in the commit message ("all of those sites guard the queueing
with rds_destroy_pending() under rcu_read_lock() (the last stragglers were
converted by the previous patch)").

There appear to be four arming sites in net/rds/threads.c that do neither.
rds_send_worker() re-arms cp_send_w with no predicate and no RCU section:

	case -EAGAIN:
		rds_stats_inc(s_send_immediate_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 0);
		break;
	case -ENOMEM:
		rds_stats_inc(s_send_delayed_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 2);

and rds_recv_worker() does the same for cp_recv_w.

rds_conn_path_drop() is a fifth site, and it deliberately bypasses the
predicate:

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

For the threads.c self-requeues the thing that keeps them off a destroyed
workqueue looks like the cancel/disable semantics of
cancel_delayed_work_sync() rejecting a requeue issued from inside the work
being cancelled, not the RCU and predicate pairing these comments name.
Would it make sense to either name these exceptions in the two comments, or
convert those sites like the others?

I checked the end of the series and both comments and all four threads.c
sites are unchanged there.

[Severity: Low]
This isn't a bug introduced by this patch, but the header comment a dozen
lines above the new store still reads:

	/*
	 * Stop and free a connection.
	 *
	 * This can only be used in very limited circumstances.  It assumes that once
	 * the conn has been shutdown that no one else is referencing the connection.
	 * We can only ensure this in the rmmod path in the current code.
	 */

That contract is contradicted by rds_ib_cm_connect_complete() in
net/rds/ib_cm.c, which destroys a single connection from an RDMA CM event
on a protocol version mismatch, and by the comment this patch adds just
below, which documents the function as fencing off concurrent requeuers for
exactly that non-rmmod case.  Should the stale header comment be refreshed
in the same patch?

For the record, the next patch in the series, "net/rds: split connection
destroy into quiesce and kref-governed free", rewrites this comment block,
so the contradiction does not survive the series.

[Severity: High]
This is a pre-existing issue, but since this is the per-connection destroy
path the commit message relies on, is there an AB-BA deadlock on it?

rds_rdma_cm_event_handler_cmn() in net/rds/rdma_transport.c takes
mutex_lock(&conn->c_cm_lock) and holds it across the whole event switch:

	if (conn) {
		mutex_lock(&conn->c_cm_lock);
	...
	case RDMA_CM_EVENT_ESTABLISHED:
		if (conn)
			trans->cm_connect_complete(conn, event);
	...
	out:
		if (conn)
			mutex_unlock(&conn->c_cm_lock);

The ESTABLISHED case reaches rds_ib_cm_connect_complete(), which for a peer
whose negotiated version is neither RDS_PROTOCOL_VERSION nor
RDS_PROTOCOL_COMPAT_VERSION calls rds_conn_destroy(conn).  The chain is:

rds_conn_destroy() -> rds_conn_path_destroy() ->
	rds_conn_path_drop(cp, true);
	flush_work(&cp->cp_down_w);

rds_conn_path_drop() with destroy == true sets cp_state to RDS_CONN_ERROR
and unconditionally queues cp_down_w, then the flush blocks the CM handler
thread while it still holds c_cm_lock.

The flushed worker runs rds_conn_shutdown(), where the DOWN to DOWN
transition fails because the state is RDS_CONN_ERROR, so it proceeds to:

	if (!rds_conn_path_transition(cp, RDS_CONN_DOWN, RDS_CONN_DOWN)) {
		/* ... else we may be deadlocking with the CM handler ... */
		mutex_lock(&cp->cp_cm_lock);

IB is not t_mp_capable, and net/rds/rds_single_path.h has:

	#define c_cm_lock		c_path[0].cp_cm_lock

so cp->cp_cm_lock is the very mutex the blocked CM handler thread holds.
Can either side make progress here?  Only the shutdown worker sets
RDS_CONN_DOWN, and its transition cannot succeed after the
atomic_set(RDS_CONN_ERROR), so mutex_lock() looks unavoidable.

The protocol major and minor come from the peer's rdma_cm private data via
rds_ib_set_protocol(), so a remote IB peer advertising an unsupported RDS
version reaches this.

As a secondary note, if the flush ever did return, the trailing
mutex_unlock(&conn->c_cm_lock) in rds_rdma_cm_event_handler_cmn() would
touch a conn that rds_conn_destroy() already kmem_cache_free()d.

The drop plus flush and the shutdown-side mutex_lock() are identical at the
baseline, and both are still present at the end of the series.

>  	/* Ensure conn will not be scheduled for reconnect */
>  	spin_lock_irq(&rds_conn_lock);
>  	hlist_del_init_rcu(&conn->c_hash_node);

[ ... ]

> diff --git a/net/rds/rds.h b/net/rds/rds.h
> index 2db49573dacd5..50b08c28ab865 100644
> --- a/net/rds/rds.h
> +++ b/net/rds/rds.h

[ ... ]

> @@ -994,7 +1001,8 @@ void __rds_put_mr_final(struct kref *kref);
>  
>  static inline bool rds_destroy_pending(struct rds_connection *conn)
>  {
> -	return !check_net(rds_conn_net(conn)) ||
> +	return READ_ONCE(conn->c_destroy_in_prog) ||
> +	       !check_net(rds_conn_net(conn)) ||
>  	       (conn->c_trans->t_unloading && conn->c_trans->t_unloading(conn));
>  }

[Severity: Medium]
Does the READ_ONCE() here rely on the caller having obtained the conn
pointer inside the same rcu_read_lock() section as the check and the queue?

At this commit struct rds_connection has no reference count, and
rds_conn_destroy() runs destroy_workqueue(cp->cp_wq), kfree(conn->c_path)
and kmem_cache_free(rds_conn_slab, conn) synchronously after the
synchronize_rcu().  Callers that hold a long-lived, non-refcounted pointer
instead of looking the conn up under RCU do exist: rs->rs_conn cached by
rds_sendmsg(), and ic->conn used by the IB completion handlers.

For those holders the deferred-send requeue in net/rds/send.c looks like it
can read the flag from freed slab memory and still queue onto a freed
workqueue:

	rcu_read_lock();
	if (rds_destroy_pending(cpath->cp_conn))
		ret = -ENETUNREACH;
	else
		queue_delayed_work(cpath->cp_wq, &cpath->cp_send_w, 1);
	rcu_read_unlock();

The single-connection destroy this patch targets,
rds_ib_cm_connect_complete() on a version mismatch, is exactly where such
stale pointers exist, so does the RCU flag alone close the
requeue-onto-freed-workqueue class the commit message describes, or does
that need the connection refcounting?

Later patches in this series do close it: "net/rds: split connection
destroy into quiesce and kref-governed free" moves destroy_workqueue(),
kfree(c_path) and kmem_cache_free() into rds_conn_destroy_fini() driven by
kref_put(), and "net/rds: hold connection references in lookup, sockets and
c_passive" makes rds_conn_lookup(), __rds_conn_create() and the rs_conn
cache own references.  Since this commit carries a Fixes: tag and may be
backported on its own, would it be worth noting in the commit message that
the guard is complete only with those follow-ups?

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