Re: [PATCH net-next v7 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 3 potential
issue(s) to consider.

Critical: 0 · High: 2 · Medium: 0 · Low: 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] At this commit rds_ib_add_conn() (net/rds/ib_rdma.c) loses its
  BUG_ON(list_empty(&ib_nodev_conns)).
- [High] rds_ib_exit() can hang forever in rds_conn_wait_conns_freed()
  when an IB connection attaches to a device's conn_list after…
- [Low] In rds_ib_exit() (net/rds/ib.c) this patch fixes two rmmod bugs
  that already exist, but the commit message calls the change…

--

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

This adds a per-transport t_conn_count. It is incremented when a
connection is published and decremented at the end of
rds_conn_destroy_fini(). The IB, TCP and loop exit paths now wait,
with no bound, for that count to reach zero. On IB, the wait resweeps
ib_nodev_conns on every poll, and rds_wq is flushed once afterwards.

> At this point in the series the initial
> reference is the only one and the wait returns at once; the guarantee
> becomes load-bearing with those patches.

[ ... ]

> At this point in the series the only reference holder is the
> destroyer itself, so the count reaches zero as soon as the sweep has
> run; the guarantee only becomes load-bearing once the following
> patches hand references to sockets and incs.

[ ... ]

> rds_ib_exit() has one more wrinkle.

[Severity: Low]
Is this accurate for IB?

rds_ib_remove_one()->rds_ib_dev_shutdown() only queues cp_down_w on
each connection's own ordered cp_wq:

    list_for_each_entry(ic, &rds_ibdev->conn_list, ib_node)
        rds_conn_path_drop(&ic->conn->c_path[0], true);

A connection moves to ib_nodev_conns only when that work runs
rds_ib_conn_path_shutdown()->rds_ib_remove_conn(). The
flush_workqueue(rds_wq) in rds_ib_unregister_client() does not wait for
cp_wq. So whenever a shutdown work is still pending, the new wait polls
and resweeps instead of returning at once.

That means the resweep fixes an existing rmmod bug. Before this patch,
rds_ib_exit() swept ib_nodev_conns only once. A connection that moved
there after that sweep was never destroyed, and it outlived the module
while still pointing at rds_ib_transport.

The same goes for the new flush_workqueue(rds_wq) in rds_ib_exit().
rds_ib_remove_conn()->rds_ib_dev_put() can drop the last device
reference after the flush in rds_ib_unregister_client(). That queues
rds_ib_dev_free() on rds_wq, and before this patch nothing flushed
rds_wq again before the module went away.

Could the commit message describe these two changes as fixes for
existing bugs, with a Fixes: tag, rather than as a wrinkle of the new
counter?

The message also doesn't mention the change from list_splice() to
list_splice_init() in rds_ib_destroy_nodev_conns(). With list_splice(),
the global ib_nodev_conns head was left pointing into the previous
call's stack-local tmp_list. That change is what makes repeated
resweeps safe. It is also why BUG_ON(list_empty(&ib_nodev_conns)) in
rds_ib_add_conn() can now trip.

This isn't a bug, but the two "At this point in the series" paragraphs
quoted above are nearly identical.

[ ... ]

> 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: High]
Can this wait loop forever if an IB connection attaches to a device's
conn_list after rds_ib_dev_shutdown() has already walked it?

rds_ib_remove_one() does:

	rds_ib_dev_shutdown(rds_ibdev);

	/* stop connection attempts from getting a reference to this device. */
	ib_set_client_data(device, &rds_ib_client, NULL);
	...
	synchronize_rcu();

An active connect's ROUTE_RESOLVED handler goes through
rds_ib_cm_initiate_connect()->rds_ib_setup_qp():

	rds_ibdev = rds_ib_get_client_data(dev);
	if (!rds_ibdev)
		return -EOPNOTSUPP;
	...
	rds_ib_add_conn(rds_ibdev, conn);

Suppose the handler gets rds_ibdev before ib_set_client_data(NULL), but
calls rds_ib_add_conn() after the rds_ib_dev_shutdown() walk. Then the
connection moves from ib_nodev_conns onto the removed device's
conn_list. The synchronize_rcu() doesn't order anything against the
attach, because the attach happens after the RCU section in
rds_ib_get_client_data().

After that, nothing seems to drop the connection:

  - rds_conn_path_drop(cp, false) returns early, because
    rds_destroy_pending() checks t_unloading, which is now true.

  - The resweep callback, rds_ib_destroy_nodev_conns(), only scans
    ib_nodev_conns.

So rds_ib_transport.t_conn_count never reaches zero. The only exit from
rds_conn_wait_conns_freed() is:

	while (!wait_event_timeout(rds_conn_freed_waitq,
				   !atomic_read(&trans->t_conn_count),
				   msecs_to_jiffies(RDS_CONN_FREE_POLL_MS))) {

rmmod would then block uninterruptibly, and the 100 ms polling keeps
the hung task detector quiet. Before this patch, the same race left a
live connection behind after unload. With this patch it becomes an
rmmod hang that never ends.

The commit message says a connection that has not migrated "would never
be destroyed, and would hold the count up for good", and the resweep is
meant to handle that. Doesn't that stay reachable for a connection that
never goes back to ib_nodev_conns?

Later in the series, "net/rds: pin the connection across RDMA-CM event
handling" adds an rds_destroy_pending() check at the start of
rds_ib_cm_initiate_connect(). That narrows the window but doesn't seem
to close it. A handler that passed the check before
rds_ib_set_unloading() and is then preempted around
rds_ib_get_client_data()/rds_ib_add_conn() still escapes. That commit's
own message notes this outcome ("rds_ib_exit() would wait forever for
the device").

[ ... ]

> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index db7e92e7bd29f..0c91f1b85c9b7 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c
> @@ -125,7 +125,6 @@ void rds_ib_add_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *con
>  
>  	/* 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);

[Severity: High]
With this assertion gone, what stops an in-flight connect from moving a
node that is sitting on the sweeper's stack-local tmp_list?

rds_ib_destroy_nodev_conns() moves everything onto tmp_list, drops
ib_nodev_conns_lock, and then walks tmp_list without the lock:

	spin_lock_irq(&ib_nodev_conns_lock);
	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)
		rds_conn_destroy(ic->conn);

Meanwhile, a ROUTE_RESOLVED handler may have got rds_ibdev from
rds_ib_get_client_data() before rds_ib_remove_one() cleared it. That
handler can still reach rds_ib_setup_qp()->rds_ib_add_conn(), which
does:

	list_del(&ic->ib_node);

	spin_lock(&rds_ibdev->spinlock);
	list_add_tail(&ic->ib_node, &rds_ibdev->conn_list);

Suppose that node is the one the sweeper has already cached in _ic.
The walk then follows ib_node.next to &rds_ibdev->conn_list, which is
not &tmp_list, so the loop doesn't stop. It then calls
rds_conn_destroy() on an ic->conn computed from memory inside struct
rds_ib_device.

The resweep callback in rds_ib_exit() repeats this unlocked walk on
every 100 ms poll, so the window opens again on each poll. A similar
list_del() from tmp_list was already possible before this patch, when
list_splice() was used.

The next patch in the series, "net/rds: unlink transport nodes before a
possibly deferred connection free", appears to fix this. It adds
i_ib_node_detached, which the sweep sets under ib_nodev_conns_lock, and
rds_ib_add_conn(), rds_ib_remove_conn() and rds_ib_conn_free() then
leave a claimed node alone. Could that be folded in here, so this
commit doesn't leave the hole open in between?

[ ... ]

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