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

[netdev-bot+sashiko@kernel.org]

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

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

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

- [Medium] At this commit only, the combination of `list_splice_init()`
  in net/rds/ib_rdma.c:rds_ib_destroy_nodev_conns() (line 168) and the…

--

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

This adds a per-transport live connection count (t_conn_count) that is
incremented when a connection is published in __rds_conn_create() and
decremented at the end of rds_conn_destroy_fini().  The transport exit
paths then poll that count until it reaches zero, with an optional
resweep callback used by the IB transport for connections that are still
detaching from a device.

[ ... ]

> diff --git a/net/rds/ib.c b/net/rds/ib.c
> index 9fe3b9951bd32..3fc2de9d19d55 100644
> --- a/net/rds/ib.c
> +++ b/net/rds/ib.c
> @@ -537,7 +537,24 @@ void rds_ib_exit(void)
>  	rds_info_deregister_func(RDS6_INFO_IB_CONNECTIONS, rds6_ib_ic_info);
>  #endif
>  	rds_ib_unregister_client();
> +
> +	/* rds_ib_dev_shutdown() only dropped the connections still
> +	 * attached to a device; each moves itself to ib_nodev_conns
> +	 * from its shutdown work.  Destroy what is there now and keep
> +	 * sweeping the list while the wait sees connections outstanding,
> +	 * so a late arrival is destroyed rather than waited on forever.
> +	 */
>  	rds_ib_destroy_nodev_conns();
> +	rds_conn_wait_conns_freed(&rds_ib_transport,
> +				  rds_ib_destroy_nodev_conns);

[Severity: Medium]
Passing rds_ib_destroy_nodev_conns as the resweep means the list is
emptied again on every 100 ms poll for the whole duration of the unload
wait, rather than just once.  Combined with the list_splice_init()
change below, can that widen a window where rds_ib_add_conn() takes
BUG()?

At this commit rds_ib_add_conn() still asserts that the list head is
non-empty:

net/rds/ib_rdma.c:rds_ib_add_conn() {
	/* conn was previously on the nodev_conns_list */
	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 that was already in flight when the unload started can reach
that assert while the sweep still holds the claimed nodes on its
stack-local tmp_list:

rds_ib_cm_handle_event() (ADDR_RESOLVED/ROUTE_RESOLVED)
  rds_ib_cm_initiate_connect()
    rds_ib_setup_qp()
      rds_ib_add_conn()          <- list_empty(&ib_nodev_conns) == true

The sweep itself calls rds_conn_destroy(), which unhashes the connection
and then blocks in synchronize_rcu() before driving the path to
RDS_CONN_DISCONNECTING, so there is real time spent with the head empty
and the connections still alive.

> +
> +	/* Tearing down the last connection may have dropped the final
> +	 * reference on a device, deferring rds_ib_dev_free() to rds_wq.
> +	 * Drain it before the module goes away; it queues nothing
> +	 * further on rds_wq.
> +	 */
> +	flush_workqueue(rds_wq);
> +
>  	rds_ib_sysctl_exit();
>  	rds_ib_recv_exit();
>  	rds_trans_unregister(&rds_ib_transport);
> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index db7e92e7bd29f..a9b27f06cbfcf 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c
> @@ -165,7 +165,7 @@ void rds_ib_destroy_nodev_conns(void)
>  
>  	/* avoid calling conn_destroy with irqs off */
>  	spin_lock_irq(&ib_nodev_conns_lock);
> -	list_splice(&ib_nodev_conns, &tmp_list);
> +	list_splice_init(&ib_nodev_conns, &tmp_list);
>  	spin_unlock_irq(&ib_nodev_conns_lock);
>  
>  	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)

[Severity: Medium]
With the old plain list_splice(), ib_nodev_conns was left stale and
non-empty, so the BUG_ON(list_empty(&ib_nodev_conns)) in
rds_ib_add_conn() could not fire during a sweep.  Does re-initializing
the head here make that assert reachable for an ic whose node the sweep
has already moved onto tmp_list?

Worth noting: the next patch in this series, "net/rds: unlink transport
nodes before a possibly deferred connection free", removes both BUG_ONs
from rds_ib_add_conn() and replaces the splice with a per-node
i_ib_node_detached claim, so the end state of the series does not have
this window - it exists only at this intermediate commit, which makes it
a bisection hazard.  Could the assert removal be folded in here, or the
list_splice_init() change moved to the later patch, so no commit in the
series has the reachable BUG()?

[ ... ]

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