Re: [PATCH net-next v3 06/13] 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 2 potential
issue(s) to consider:

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

- [High] rds_ib_exit()'s new comment and the commit message claim the
  resweep guarantees that "a late arrival is destroyed rather than…
- [Low] Assessed as a false positive for this patch and for the final
  state of the series (recorded for the pipeline, baseline decision =…

Pre-existing issues:
- [Low] Assessed as a false positive under the series rule (recorded for
  the pipeline, baseline decision = false).

--

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

This adds a per-transport live connection count (t_conn_count), bumped
when a connection is published in __rds_conn_create() and dropped as the
last step of rds_conn_destroy_fini().  The transport exit paths
(rds_ib_exit(), rds_tcp_exit(), rds_loop_exit()) then wait, without a
bound, for that count to reach zero before letting the module text go
away, with an optional resweep callback used by IB.

A few questions below about the IB resweep scope, the comment on
rds_conn_wait_conns_freed(), and the nodev sweep list handling.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index a44aa4d2a5e8..c3b3d756c52e 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -596,7 +601,51 @@ static void rds_conn_destroy_fini(struct kref *kref)
>  	spin_lock_irqsave(&rds_conn_lock, flags);
>  	rds_conn_count--;
>  	spin_unlock_irqrestore(&rds_conn_lock, flags);
> +
> +	/* only after everything the transport module owns has been
> +	 * freed above may its unload proceed
> +	 */
> +	if (!atomic_dec_return(&trans->t_conn_count))
> +		wake_up_all(&rds_conn_freed_waitq);
> +}
> +
> +/* 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 - 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]

This is a pre-existing hole rather than something introduced here, but
does the comment above rds_conn_wait_conns_freed() describe more than
t_conn_count actually covers at this point in the series?

The comment names inc_free among the frees the wait protects, yet at this
commit a struct rds_incoming holds no connection reference, so an unread
datagram does not contribute to t_conn_count.  The wait in rds_ib_exit()
can therefore complete and rds_ib_recv_exit() can destroy
rds_ib_incoming_slab while an inc is still queued on a socket.

The commit message already flags the sequencing ("once the following
patches make incs hold a connection reference"), and the later patch
"net/rds: hold a connection reference from struct rds_incoming" makes it
true by taking a reference in rds_inc_init():

net/rds/recv.c:rds_inc_init() {
	...
	rds_conn_get(conn);	/* put in rds_inc_put() */
	inc->i_conn = conn;
	...
}

Would it be worth noting in this comment that the inc_free half only
becomes accurate with that later patch, so the intermediate state of the
tree is not read as a guarantee it does not yet provide?

> +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))) {
> +		/* A transport whose teardown is asynchronous (IB moves a
> +		 * connection off its device from the shutdown work) gives
> +		 * us a resweep to destroy what has arrived since.
> +		 */
> +		if (resweep)
> +			resweep();
> +		if (time_after_eq(jiffies, warn_at)) {
> +			pr_warn("RDS/%s: still waiting for %d connection(s) to be freed before unload\n",
> +				trans->t_name,
> +				atomic_read(&trans->t_conn_count));
> +			warn_at = jiffies + warn_interval;
> +		}
> +	}
>  }
> +EXPORT_SYMBOL_GPL(rds_conn_wait_conns_freed);
>  
>  void rds_conn_get(struct rds_connection *conn)
>  {
> diff --git a/net/rds/ib.c b/net/rds/ib.c
> index 9fe3b9951bd3..3fc2de9d19d5 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 the resweep here miss a connection entirely and turn this into an
rmmod that never returns?

rds_ib_destroy_nodev_conns() only looks at the global ib_nodev_conns
list:

net/rds/ib_rdma.c:rds_ib_destroy_nodev_conns() {
	spin_lock_irq(&ib_nodev_conns_lock);
	list_splice_init(&ib_nodev_conns, &tmp_list);
	spin_unlock_irq(&ib_nodev_conns_lock);
	...
}

A connection whose rds_ib_connection is still on an rds_ibdev->conn_list
is not on that list, and after ib_unregister_client() has returned
nothing appears to drop it again.  rds_ib_dev_shutdown() is a one-shot
sweep that already ran:

net/rds/ib.c:rds_ib_dev_shutdown() {
	spin_lock_irqsave(&rds_ibdev->spinlock, flags);
	list_for_each_entry(ic, &rds_ibdev->conn_list, ib_node)
		rds_conn_path_drop(&ic->conn->c_path[0], true);
	...
}

and every other drop site goes through rds_conn_path_drop(cp, false),
which bails out while rds_destroy_pending() is true - and
rds_ib_set_unloading() has made t_unloading true for the whole of
rds_ib_exit():

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

Is there a window where an ic attaches to the device list after that
sweep?  rds_ib_setup_qp() resolves the device and then adds itself:

net/rds/ib_cm.c:rds_ib_setup_qp() {
	rds_ibdev = rds_ib_get_client_data(dev);
	if (!rds_ibdev)
		return -EOPNOTSUPP;
	...
	/* add the conn now so that connection establishment has the dev */
	rds_ib_add_conn(rds_ibdev, conn);
	...
}

and rds_ib_remove_one() clears the client data only after
rds_ib_dev_shutdown() has walked conn_list, so a connect already in
flight when rmmod starts can still get a device pointer and land on
conn_list.

Such a connection never migrates to ib_nodev_conns, the resweep never
sees it, and t_conn_count stays non-zero.  Since the wait is deliberately
unbounded and uninterruptible, rmmod would then block forever with the
ten-second warning repeating, and the module stays in MODULE_STATE_GOING.

Would it make sense for the resweep to also walk the rds_ib_devices
conn_lists, or otherwise cover connections that are still device
attached?  As written, the comment's claim that "a late arrival is
destroyed rather than waited on forever" seems to hold only for
connections that already reached ib_nodev_conns.

> +
> +	/* 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 db7e92e7bd29..a9b27f06cbfc 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: Low]

Now that the free of a connection can outlive rds_conn_destroy(), is the
ib_node of each ic left linked on the stack-local tmp_list across the
destroy?  rds_ib_conn_free() is what unlinks ib_node, so a free that runs
after rds_ib_destroy_nodev_conns() has returned would do a list_del()
against a dead stack frame.  The same shape exists in
rds_tcp_destroy_conns():

net/rds/tcp.c:rds_tcp_destroy_conns() {
	...
	list_for_each_entry_safe(tc, _tc, &tmp_list, t_tcp_node)
		rds_conn_destroy(tc->t_cpath->cp_conn);
}

At this commit rds_conn_get() has no callers, so the rds_conn_put() at
the end of rds_conn_destroy() is always the last reference and conn_free()
still runs inline inside the sweep loop, which means this cannot fire
yet.  The next patch in the series, "net/rds: unlink transport nodes
before a possibly deferred connection free", adds the per-entry
list_del_init() under the transport lock ahead of the reference-holding
patches.

Is it worth mentioning that ordering dependency in this commit message,
so the intermediate state is clearly safe by accident of there being no
extra reference holders yet rather than by design?

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