Re: [PATCH net-next v4 06/12] net/rds: wait for connections to be freed on transport unload

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

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

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

- [High] rds_ib_exit()'s new wait can never complete for a connection
  whose rds_conn_destroy() is issued from the RDMA-CM event handler, so…
- [Medium] rds_conn_wait_conns_freed() (net/rds/connection.c) is an
  unbounded TASK_UNINTERRUPTIBLE poll loop executed from module exit…
- [Medium] At this commit, list_splice_init() empties ib_nodev_conns
  while the swept connections' ib_node entries sit on the rmmod thread's…
- [Low] Undocumented, load-bearing semantic change in net/rds/ib_rdma.c:
  rds_ib_destroy_nodev_conns() is switched from list_splice() to…
- [Low] The kernel-doc-style comment on rds_conn_wait_conns_freed()
  states a precondition its own IB caller deliberately violates: 'Called…

Pre-existing issues:
- [Medium] Queued incoming datagrams hold no connection reference at this
  commit, so t_conn_count can reach zero and permit transport unload…

--

net/rds: wait for connections to be freed on transport unload

This adds a per-transport live-connection counter (t_conn_count),
incremented when a connection is published in __rds_conn_create() and
decremented at the end of rds_conn_destroy_fini().  The transport exit
paths (rds_ib_exit(), rds_tcp_exit(), rds_loop_exit()) then poll that
count until it reaches zero before letting the module unload proceed.

A few questions below.

[ ... ]

[Severity: Medium]

This isn't a bug introduced by this patch, but at this commit a queued
incoming datagram holds no connection reference: rds_inc_init() and
rds_inc_path_init() in net/rds/recv.c store inc->i_conn without calling
rds_conn_get().

So t_conn_count can reach zero and let the transport unload while an
unread inc still carries inc->i_conn and dispatches through that
transport's callbacks:

net/rds/recv.c:rds_inc_put() {
	...
	inc->i_conn->c_trans->inc_free(inc);
}

The commit message is explicit that this only closes "once the following
patches make incs hold a connection reference", and the later patch
"net/rds: hold a connection reference from struct rds_incoming" does add
rds_conn_get() in rds_inc_init()/rds_inc_path_init().  Noting it here only
so the intermediate state in the series is on record - would it be worth
saying in the changelog that the guarantee is incomplete until that patch
lands?

+}
+
+/* Wait for all of @trans's connections to be freed; the free runs
+ * asynchronously once rds_conn_destroy() has quiesced a connection.
+ * Called on transport module unload, after the transport has destroyed
+ * all of its connections.  A connection reference can be held for an
+ * application-controlled time - once incs hold one, an unread datagram
+ * pins the inc that carries it, and thus the connection - so the wait
+ * is unbounded: the
+ * frees that run after unload call into this module's text (conn_free,
+ * inc_free) and free into its slabs, so proceeding while any remain
+ * would be a use-after-free, not a leak.  Warn periodically so a stuck
+ * count is diagnosable, but never stop waiting.  This matches the
+ * historical RDS contract that teardown does not discard queued data.
+ */

[Severity: Low]

Is the stated precondition the one the code actually has?  The comment
says the function is "Called on transport module unload, after the
transport has destroyed all of its connections", but rds_ib_exit() calls
it while connections are still attached to devices and migrating to
ib_nodev_conns asynchronously - which is why it passes
rds_ib_destroy_nodev_conns as the resweep callback and why the loop body
calls resweep() after every timeout.

Could the comment describe the real contract instead, i.e. that the caller
does an initial sweep and the resweep destroys late arrivals during the
wait?

Also, the sentence "so the wait is unbounded: the" / "frees that run after
unload call into this module's text" looks like a mangled line break.

+void rds_conn_wait_conns_freed(struct rds_transport *trans,
+			       void (*resweep)(void))
+{
+	unsigned long warn_interval =
+			msecs_to_jiffies(RDS_CONN_FREE_WARN_INTERVAL_MS);
+	unsigned long warn_at = jiffies + warn_interval;
+
+	while (!wait_event_timeout(rds_conn_freed_waitq,
+				   !atomic_read(&trans->t_conn_count),
+				   msecs_to_jiffies(RDS_CONN_FREE_POLL_MS))) {

[Severity: Medium]

Can an unprivileged application wedge rmmod permanently here?  The loop
has no timeout, no signal check and no bail-out, and wait_event_timeout()
sleeps in TASK_UNINTERRUPTIBLE.

By the end of the series an inc holds a connection reference, so a process
that binds an AF_RDS socket, receives one datagram and never calls
recvmsg() keeps t_conn_count non-zero for as long as it likes.

The transport module reference a bound socket holds does not cover this
case: rds_bind() can take rs->rs_transport preset via SO_RDS_TRANSPORT,
and rds_find_bound() does not filter on transport, so the socket pinning
an IB connection's inc need hold no rds_rdma reference at all - which the
commit message itself notes.

delete_module() has already moved the module to MODULE_STATE_GOING before
calling mod->exit(), so at that point the module can neither finish
unloading nor be re-inserted, and the rmmod task is unkillable.  The same
applies to rds_tcp_exit(), where the wait runs after
unregister_pernet_device() and rds_tcp_destroy_conns(), and to
rds_loop_exit() in the core module.

Was having long-term reference holders take a module reference considered
instead, so delete_module() fails with -EBUSY rather than blocking inside
->exit()?

[Severity: High]

Can this wait ever finish for a connection whose destroy was issued from
the RDMA-CM event handler?

net/rds/ib_cm.c:rds_ib_cm_connect_complete() destroys the connection on a
protocol version mismatch:

	rds_conn_destroy(conn);
	return;

and it runs with conn->c_cm_lock (c_path[0].cp_cm_lock) held by
rds_rdma_cm_event_handler_cmn() in net/rds/rdma_transport.c:

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

The destroy path is then:

rds_conn_destroy() -> rds_conn_path_quiesce()
    rds_conn_path_drop(cp, true)   /* always queues cp_down_w */
    flush_work(&cp->cp_down_w)
        rds_shutdown_worker() -> rds_conn_shutdown()
            mutex_lock(&cp->cp_cm_lock);   /* held by this thread */

rds_conn_destroy_fini() is never reached, so
atomic_dec_return(&trans->t_conn_count) never runs and this wait loops
forever, printing the 10 s warning indefinitely, with no indication of
which connection is stuck.  The protocol version that triggers it is
chosen by the remote peer.

This state persists to the end of the series: net/rds/ib_cm.c still calls
rds_conn_destroy(conn) at that site, even though the later commit
"net/rds: pin the connection across RDMA-CM event handling" says it was
switched to rds_conn_drop().  Should that conversion come with this patch,
since this is the change that turns a stuck destroy into an unbounded
uninterruptible wait?

Separately, the changelog states the wake at the end of
rds_conn_destroy_fini() "can run from a thread executing transport module
text, but never as that thread's last use of it".  Only a few core-module
calls separate the wait's return from the module text being freed - is
there anything in the code that enforces that ordering, or is it an
argument about the current call sites only?

[Severity: Low]

Could this one-line change get a mention in the changelog?  It looks
load-bearing for the new resweep: rds_ib_exit() now passes
rds_ib_destroy_nodev_conns as the resweep callback, so it runs every
100 ms, and plain list_splice() leaves ib_nodev_conns still pointing at
entries that were moved onto the caller's stack list, since __list_splice()
does not re-init the source head.  The commit message describes the resweep
at length but not this fix, which also reads like a separate preparatory
change.

[Severity: Medium]

At this commit, does emptying ib_nodev_conns while the swept entries live
on the rmmod thread's stack list conflict with rds_ib_add_conn()?  That
function still has:

net/rds/ib_rdma.c:rds_ib_add_conn() {
	spin_lock_irq(&ib_nodev_conns_lock);
	BUG_ON(list_empty(&ib_nodev_conns));
	BUG_ON(list_empty(&ic->ib_node));
	list_del(&ic->ib_node);
	...
}

A connect worker already in flight when rds_ib_exit() runs would either hit
the assertion or unlink an entry from the sweeper's stack-local list.

The next patch in the series, "net/rds: unlink transport nodes before a
possibly deferred connection free", removes both BUG_ONs and rewrites the
sweep to take a reference per entry and list_del_init() each node under the
lock just before its destroy, so the window is confined to this commit -
would it be simpler to fold the two together?

[ ... ]

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