Re: [PATCH net-next v2 4/9] net/rds: wait for connections to be freed on transport unload

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Repeatedly calling `rds_ib_destroy_nodev_conns` as a resweep callback causes list corruption and an infinite loop because `list_splice` does not reinitialize the list head.
--

commit 597676df37a14231916bc15aaa2d2b879b415a67
Author: Sharath Srinivasan <sharath.srinivasan@oracle.com>

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

This patch tracks the number of live connections per transport and adds a
polling wait loop on transport unload to ensure all connections are fully
destroyed before the module exits. It also provides a resweep callback for
transports like IB that detach asynchronously.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
[ ... ]
> @@ -591,7 +596,51 @@ static void rds_conn_destroy_fini(struct kref *kref)
[ ... ]
> +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)) {
[ ... ]
> diff --git a/net/rds/ib.c b/net/rds/ib.c
> --- a/net/rds/ib.c
> +++ b/net/rds/ib.c
[ ... ]
> @@ -537,7 +537,24 @@ void rds_ib_exit(void)
[ ... ]
> +	/* 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 lead to list corruption if rds_ib_destroy_nodev_conns() is called
multiple times?

The new polling loop in rds_conn_wait_conns_freed() will repeatedly call the
resweep function if the transport connection count is greater than zero.

Looking at rds_ib_destroy_nodev_conns() in net/rds/ib_rdma.c, it uses
list_splice() without reinitializing the original list:

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

Because list_splice() leaves the ib_nodev_conns head pointing to elements
that are then moved to tmp_list and freed, a second call to this function
during the wait loop will evaluate list_empty() as false and execute
__list_splice() with stale pointers.

This can result in a use-after-free write to the prev and next fields of
freed memory.

Could this use list_splice_init() instead so the list head is safely
emptied for subsequent polling iterations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912035027.27447-1-achender@kernel.org?part=4