Re: [PATCH net-next v4 05/12] net/rds: split connection destroy into quiesce and kref-governed free

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] The freeing half of teardown (destroy_workqueue() of the per-path
  workqueues in rds_conn_path_free(), net/rds/connection.c:565-576, plus…
- [Low] The destruction-protocol comments state that every site arming a
  path work tests rds_destroy_pending() inside one rcu_read_lock()…
- [Low] The new duplicate-call branch in rds_conn_destroy()
  (net/rds/connection.c:646-651) returns as soon as c_destroy_in_prog is…

Pre-existing issues:
- [High] Unsupported-protocol-version handling deadlocks the RDS/IB
  connection teardown: rds_rdma_cm_event_handler_cmn()…

--

net/rds: split connection destroy into quiesce and kref-governed free

This patch adds a kref to struct rds_connection, keeps the synchronous
quiesce work in rds_conn_destroy() and moves the actual frees (transport
conn_free, path workqueues, c_path array, slab object) into
rds_conn_destroy_fini(), which runs from the last rds_conn_put().
rds_conn_get()/rds_conn_put() plus an inline rds_conn_get_unless_zero()
are exported for the reference holders added in the following patches.

A few questions below.

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but since the quiesce (and its
flush_work()) stays synchronous inside rds_conn_destroy(), can this
teardown still deadlock when a peer negotiates an unsupported protocol
version?

The RDMA-CM handler calls the transport callback with the connection
management mutex held:

net/rds/rdma_transport.c:rds_rdma_cm_event_handler_cmn() {
	...
	mutex_lock(&conn->c_cm_lock);
	...
	trans->cm_connect_complete(conn, event);
}

and for a version mismatch that callback destroys the connection:

net/rds/ib_cm.c:rds_ib_cm_connect_complete() {
	...
	rds_conn_destroy(conn);
}

rds_conn_destroy() -> rds_conn_path_quiesce() then queues cp_down_w and
waits for it:

net/rds/connection.c:rds_conn_path_quiesce() {
	...
	rds_conn_path_drop(cp, true);
	flush_work(&cp->cp_down_w);
}

while the worker it waits for needs the same mutex:

net/rds/connection.c:rds_conn_shutdown() {
	...
	mutex_lock(&cp->cp_cm_lock);
}

For IB (single path) net/rds/rds_single_path.h aliases c_cm_lock to
c_path[0].cp_cm_lock, so the CM handler and the shutdown worker are
waiting on each other, and the version field comes from the peer's CM
private data.

Upstream commit f97d8c7bab78 ("rds: ib: use rds_conn_drop() on protocol
version mismatch", Cc: stable) addresses exactly this by not calling
rds_conn_destroy() there, but that change is not in this baseline -
rds_ib_cm_connect_complete() still calls rds_conn_destroy() both here and
at the tip of this series.  The later series patch "net/rds: pin the
connection across RDMA-CM event handling" states that the call was already
removed; is that accurate for this tree?

At the tip of the series this also turns transport unload into an
unbounded wait, since rds_conn_wait_conns_freed() in rds_ib_exit() polls
for a t_conn_count that a never-freed connection cannot drop.

[Severity: Low]
Would it be worth spelling out the calling-context contract that
rds_conn_put() now carries?

kref_put() runs the release in the caller's context, and
rds_conn_destroy_fini() -> rds_conn_path_free() sleeps:

	if (cp->cp_wq != rds_wq) {
		destroy_workqueue(cp->cp_wq);

destroy_workqueue() calls drain_workqueue(), mutex_lock(&wq->mutex) and
kthread_stop(), and in this commit rds_conn_destroy_fini() also takes
rds_conn_lock, so a last put issued under rds_conn_lock would recurse on
it.  That rds_conn_lock use is removed later in the series by "net/rds:
drop rds_conn_count in favor of t_conn_count", but the sleeping part
remains.

At this commit the only put site is the tail of rds_conn_destroy(), which
is process context and holds no lock, so nothing is broken today.  Should
rds_conn_put() carry a might_sleep() and a comment on the prototype, so
the future holders added by the follow-up patches have the requirement
stated in the header rather than only in commit messages?

 /*
  * Stop and free a connection.
  *

[ ... ]

[Severity: Low]
This isn't a bug, but is the "all test rds_destroy_pending() under
rcu_read_lock()" wording too broad?

The workers requeue themselves without the predicate and without an RCU
section:

net/rds/threads.c:rds_send_worker() {
	...
	case -EAGAIN:
		rds_stats_inc(s_send_immediate_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 0);
}

net/rds/threads.c:rds_recv_worker() {
	...
	case -EAGAIN:
		rds_stats_inc(s_recv_immediate_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_recv_w, 0);
}

The same claim also appears on c_destroy_in_prog 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").  The self-requeues are safe because
rds_conn_path_quiesce() cancels those works with
cancel_delayed_work_sync(), which is what the earlier series patch
"net/rds: guard every work-requeueing site with rds_destroy_pending()"
explains, and the over-broad clause came from "net/rds: make
rds_destroy_pending() cover single-connection destroy" rather than this
patch.  Could both comments be qualified with something like "apart from
the workers' own self-requeues"?

+	 *
+	 * Now that the transport state stays discoverable (e.g. on the
+	 * transports' connection lists) until the final rds_conn_put(),
+	 * a conn can be handed to rds_conn_destroy() more than once -
+	 * e.g. dropped for a protocol version mismatch and then found
+	 * again at module unload.  Only the first caller proceeds; the
+	 * unhash also happens under rds_conn_lock, so a looked-up conn
+	 * can never be quiesced twice.
 	 */
+	spin_lock_irq(&rds_conn_lock);
+	if (conn->c_destroy_in_prog) {
+		spin_unlock_irq(&rds_conn_lock);
+		return;
+	}
 	WRITE_ONCE(conn->c_destroy_in_prog, true);

[Severity: Low]
What does a second caller learn from this early return?  c_destroy_in_prog
is set here, before synchronize_rcu() and before any path is quiesced, so
on return the first caller may still be inside rds_conn_path_quiesce().

Meanwhile the new comment in net/rds/rds.h promises unconditionally:

	/* rds_conn_destroy() quiesces the connection synchronously;
	 * freeing it - the connection memory, the path workqueues and
	 * the transport's per-connection state - is deferred until the
	 * last reference is dropped via rds_conn_put().
	 */

The only real second destroyer today is rds_ib_cm_connect_complete()
racing the rds_ib_exit() sweep; the TCP and loopback sweeps unlink each
node under the transport lock first, and rds_ib_remove_one() only drops
the path.  The next patch in the series adds rds_conn_wait_conns_freed()
to rds_ib_exit(), which closes the window where the sweep returns while
another thread is still quiescing, so this is only about the wording.
Could the rds.h comment note that a duplicate rds_conn_destroy() call
returns without waiting for the in-flight quiesce?

[ ... ]

Thanks for looking at these.

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