Re: [PATCH net-next v7 07/12] net/rds: unlink transport nodes before a possibly deferred connection free

[netdev-bot+sashiko@kernel.org]

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 rds_conn_destroy() comment in net/rds/connection.c gives a
  reason that this patch makes false, and the comment is still there,…
- [Low] The commit message does not describe the IB changes accurately.

--

net/rds: unlink transport nodes before a possibly deferred connection free

This patch changes the RDS TCP, IB and loopback teardown helpers so each
gathered transport node comes off the on-stack list before
rds_conn_destroy() runs. A deferred conn_free() then cannot write into a
dead stack frame. The gather also takes a connection reference, and a new
i_ib_node_detached flag makes rds_ib_add_conn(), rds_ib_remove_conn() and
rds_ib_conn_free() leave a claimed IB node alone.

> Unlink each node under the transport lock right before its
> rds_conn_destroy(), so that nothing is left on the stack list for a
> later free to touch.

[Severity: Low]
This matches loop (loop_conns_lock) and TCP (rds_tcp_conn_lock). It does
not match IB: rds_ib_destroy_nodev_conns() takes the node off tmp_list
with no lock held:

	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node) {
		conn = ic->conn;
		list_del_init(&ic->ib_node);

Only the later IB paragraph says that the walk is lockless.

> Those movers used to assert that the node is linked (and
> rds_ib_add_conn() that the nodev list is non-empty); a connect or
> shutdown worker can still be running for a connection the sweep has
> claimed, and such a connection is about to be destroyed anyway, so
> the assertions go.

The nodev-list assertion in rds_ib_add_conn() was already removed by the
previous patch, "net/rds: wait for connections to be freed on transport
unload". That patch's message says:

  rds_ib_add_conn() loses its assertion that the nodev list is
  non-empty

This patch only drops the two BUG_ON(list_empty(&ic->ib_node)) checks in
rds_ib_add_conn() and rds_ib_remove_conn().

Could the commit message be updated to match what the IB changes do?

> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index 0c91f1b85c9b7..71926f8ada41a 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c

[ ... ]

> @@ -162,13 +173,40 @@ void rds_ib_destroy_nodev_conns(void)
>  	struct rds_ib_connection *ic, *_ic;
>  	LIST_HEAD(tmp_list);
>  

[ ... ]

>  	spin_lock_irq(&ib_nodev_conns_lock);
> -	list_splice_init(&ib_nodev_conns, &tmp_list);
> +	list_for_each_entry_safe(ic, _ic, &ib_nodev_conns, ib_node) {
> +		if (rds_conn_get_unless_zero(ic->conn)) {
> +			ic->i_ib_node_detached = true;
> +			list_move_tail(&ic->ib_node, &tmp_list);
> +		}
> +	}
>  	spin_unlock_irq(&ib_nodev_conns_lock);
>  
> -	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
> -		rds_conn_destroy(ic->conn);
> +	/* rds_conn_destroy() can return before the connection is freed,
> +	 * and it is the free - rds_ib_conn_free() - that would unlink
> +	 * ib_node.  tmp_list lives on this stack frame, so take each node
> +	 * off it before its destroy; the free then leaves it alone.
> +	 */
> +	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node) {
> +		conn = ic->conn;
> +		list_del_init(&ic->ib_node);
> +
> +		rds_conn_destroy(conn);
> +		rds_conn_put(conn);
> +	}
>  }

[Severity: Low]
After this change, is the rds_conn_destroy() comment in
net/rds/connection.c still accurate? It says:

net/rds/connection.c:rds_conn_destroy() {
	 * 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 -
	 * the IB unload path re-sweeps its connection list until every
	 * connection is gone.  Only the first caller proceeds; the
	 * unhash also happens under rds_conn_lock, so a looked-up conn
	 * can never be quiesced twice.  (With the initial reference the
	 * only one, as it is at this point in the series, nothing can
	 * reach a second call yet; this is the contract the following
	 * patches rely on.)
}

In the sweep above, each gathered node gets i_ib_node_detached set under
ib_nodev_conns_lock and is list_del_init()'d before rds_conn_destroy().
rds_ib_add_conn() and rds_ib_remove_conn() never put a claimed node back
on ib_nodev_conns.

So the resweep that rds_ib_exit() passes to rds_conn_wait_conns_freed()
cannot find a connection it has already destroyed. That is the
double-destroy case the comment gives as the reason for the
c_destroy_in_prog test-and-set.

The other transports behave the same way.
rds_loop_destroy_gathered_conns() list_del_init()s the node and nothing
links it again. In rds_tcp_exit(), unregister_pernet_device() calls
rds_tcp_kill_sock() before the only rds_tcp_destroy_conns(), and nothing
sweeps after that.

Each sweep also holds its own extra reference when it calls
rds_conn_destroy(). So "the initial reference the only one" no longer
matches these callers, and "at this point in the series" means nothing
once the series is merged.

The comment is still unchanged at the end of the series. The guard itself
is harmless. Could the comment be updated to give the real reason for
keeping c_destroy_in_prog, or say that the IB re-sweep no longer reaches
it?

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