Re: [PATCH net-next v4 04/12] 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: 1 · Medium: 0 · Low: 1 (1)

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] Unconditional self-deadlock in the very per-connection destroy
  path this patch is written for.
- [Low] The two comments added by this patch state an invariant the tree
  does not maintain.

Pre-existing issues:
- [Low] Stale contract comment above rds_conn_destroy()
  (net/rds/connection.c:564-570) still claims 'We can only ensure this…

--

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

This adds a per-connection conn->c_destroy_in_prog flag, set by
rds_conn_destroy() before the unhash plus synchronize_rcu() sequence, and
tests it from rds_destroy_pending() so that the work-requeueing sites stop
arming path works on a connection that is being torn down.  The now unused
RDS_DESTROY_PENDING cp_flags bit and its dead test in
rds_ib_is_unloading() are removed.

A few questions on the new comments and on the per-connection destroy path
this is aimed at.

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

[Severity: Low]

This isn't a bug introduced by this patch, but the contract comment
immediately above rds_conn_destroy() 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.
   */

The flag added below, and its comment in net/rds/rds.h, describe protection
for the non-rmmod case, and in-tree callers such as
rds_ib_cm_connect_complete() (net/rds/ib_cm.c) destroy a single connection
outside rmmod.  Later in this series "net/rds: split connection destroy into
quiesce and kref-governed free" replaces that comment, so is it worth
refreshing it here so the two do not disagree in between?

>  		 "%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.
> +	 */

[Severity: Low]

Is the parenthetical "which all test rds_destroy_pending() under
rcu_read_lock()" accurate for the tree as it stands?  The same claim is made
unconditionally in the new struct rds_connection comment:

  /* ... A site that arms
   * a path work must test the predicate and queue the work inside
   * one rcu_read_lock() section: ...
   */

Three arming sites do neither.  rds_send_worker() and rds_recv_worker() in
net/rds/threads.c re-arm their own works with no predicate and no RCU
section:

	switch (ret) {
	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);
		break;

And rds_conn_path_drop() bypasses the predicate on purpose for destroy:

net/rds/connection.c:rds_conn_path_drop() {
	rcu_read_lock();
	if (!destroy && rds_destroy_pending(cp->cp_conn)) {
		rcu_read_unlock();
		return;
	}
	queue_work(cp->cp_wq, &cp->cp_down_w);
	rcu_read_unlock();
}

that being the call rds_conn_path_destroy() makes with destroy == true,
after c_destroy_in_prog has already been set.  The commit message does note
the worker self-requeues and the sync cancel that covers them, but the
in-tree comments do not mention either exception.  Could the comments spell
out that the workers and the destroy == true drop are covered by the sync
cancel and by the destroy path driving its own final shutdown pass instead?

> +	WRITE_ONCE(conn->c_destroy_in_prog, true);

[Severity: High]

Can the single-connection destroy this flag is written for actually
complete?  The RDMA CM handler holds conn->c_cm_lock (which is
conn->c_path[0].cp_cm_lock, see net/rds/rds_single_path.h) across the
ESTABLISHED dispatch:

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

and the version-mismatch destroy runs from inside that dispatch:

net/rds/ib_cm.c:rds_ib_cm_connect_complete() {
	if (conn->c_version < RDS_PROTOCOL_VERSION) {
		if (conn->c_version != RDS_PROTOCOL_COMPAT_VERSION) {
			pr_notice(...);
			rds_conn_destroy(conn);
			return;
	...
}

rds_conn_destroy() then reaches rds_conn_path_destroy(), which queues
cp_down_w with the guard bypassed and waits for it while still holding
cp_cm_lock:

net/rds/connection.c:rds_conn_path_destroy() {
	rds_conn_path_drop(cp, true);	/* cp_state = RDS_CONN_ERROR, queue */
	flush_work(&cp->cp_down_w);
	...
}

The shutdown worker fails the DOWN to DOWN transition because the state is
RDS_CONN_ERROR, and then takes the mutex the flusher owns:

net/rds/connection.c:rds_conn_shutdown() {
	if (!rds_conn_path_transition(cp, RDS_CONN_DOWN, RDS_CONN_DOWN)) {
		...
		mutex_lock(&cp->cp_cm_lock);
	...
}

IB is not t_mp_capable, so npaths is 1 and the path being destroyed is
exactly the one whose cp_cm_lock is held.  ERROR to DISCONNECTING is only
performed under cp_cm_lock, so no other pass can unblock the transition
either.  Does this leave the CM event thread parked in flush_work() holding
cp_cm_lock and the path worker parked in mutex_lock() behind it, with the
peer-supplied CM private data (major 3, minor 0) as the trigger?

If so, the motivating scenario in the commit message, "queue_delayed_work()
on a cp_wq that destroy_workqueue() has already freed", would not be
reachable from this caller, because the destroy hangs before
destroy_workqueue() runs.

There is a related use-after-free on the same path at the handler tail:

net/rds/rdma_transport.c:rds_rdma_cm_event_handler_cmn() {
out:
	if (conn)
		mutex_unlock(&conn->c_cm_lock);
	...
}

which runs on memory already returned by kfree(conn->c_path) and
kmem_cache_free(rds_conn_slab, conn) in rds_conn_destroy().  That part is
addressed later in this series by "net/rds: pin the connection across
RDMA-CM event handling", but the lock re-entry above appears to survive to
the end of the series, where rds_conn_path_quiesce() still does
rds_conn_path_drop(cp, true) plus flush_work(&cp->cp_down_w) and
net/rds/ib_cm.c still calls rds_conn_destroy() from the CM callback.  Would
deferring that destroy out of the CM callback be the better fix for this
site?

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

[ ... ]

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