Re: [PATCH net-next v8 07/13] 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 commit message says the whole change is groundwork: the old
  helpers were "fine for as long as rds_conn_destroy() frees the…
- [Low] After this patch, rds_ib_add_conn() (net/rds/ib_rdma.c) has a
  third outcome.

--

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

This patch changes the RDS TCP, loopback and IB teardown helpers. Each
helper now takes a reference on every connection it gathers and unlinks
the transport node from the on-stack list before calling
rds_conn_destroy(), so a deferred conn_free() later in the series does not
touch a dead stack frame. IB also gains an i_ib_node_detached flag, which
makes rds_ib_add_conn(), rds_ib_remove_conn() and rds_ib_conn_free() leave
a node alone once the sweep has claimed it.

> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index 50c02f47cf683..34e09525a10b2 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c

[ ... ]

> @@ -136,14 +137,15 @@ int rds_ib_add_conn(struct rds_ib_device *rds_ibdev,
>  	 * the caller to fail and the transport exit to find.
>  	 */
>  	if (rds_ibdev->shutting_down) {
> -		spin_unlock(&rds_ibdev->spinlock);
> -		spin_unlock_irq(&ib_nodev_conns_lock);
> -		return -ENODEV;
> +		ret = -ENODEV;
> +	} else if (!ic->i_ib_node_detached) {
> +		list_del(&ic->ib_node);
> +		list_add_tail(&ic->ib_node, &rds_ibdev->conn_list);
>  	}
> -	list_del(&ic->ib_node);
> -	list_add_tail(&ic->ib_node, &rds_ibdev->conn_list);
>  	spin_unlock(&rds_ibdev->spinlock);
>  	spin_unlock_irq(&ib_nodev_conns_lock);
> +	if (ret)
> +		return ret;
>  
>  	ic->rds_ibdev = rds_ibdev;
>  	refcount_inc(&rds_ibdev->refcount);

[Severity: Low]
Take the case where the device is not shutting_down but the sweep has
already set i_ib_node_detached. Here ret stays 0 and the node is not
moved onto rds_ibdev->conn_list. The function still sets ic->rds_ibdev,
takes a device reference and returns success.

Does that third outcome fit the promise made in rds_ib_dev_shutdown()?

    /* Close the device to new connections under the same lock that
     * rds_ib_add_conn() attaches them under, so that every
     * connection is either dropped by the walk below or refused.
     */

As far as I can tell, this branch can't be reached at the moment.
rds_ib_destroy_nodev_conns() only runs from rds_ib_exit(), after
rds_ib_unregister_client(). That call runs
rds_ib_remove_one()->rds_ib_dev_shutdown() for every device, which sets
shutting_down under rds_ibdev->spinlock. rds_ib_add_conn() tests
shutting_down first, under the same lock, so a claimed connection always
gets -ENODEV.

Would it be more robust to return -ENODEV for the detached case as well?
The commit message already says "such a connection is about to be
destroyed anyway", and that change would make the invariant hold by
construction.

The existing comment above, "leave the connection on the nodev list for
the caller to fail and the transport exit to find", is also slightly
inexact for a claimed node. Such a node is on the sweep's stack list or
already unlinked.

[ ... ]

> @@ -176,13 +185,40 @@ void rds_ib_destroy_nodev_conns(void)
>  	struct rds_ib_connection *ic, *_ic;
>  	LIST_HEAD(tmp_list);
>  
> -	/* avoid calling conn_destroy with irqs off */
> +	struct rds_connection *conn;
> +

[ ... ]

>  	spin_lock_irq(&ib_nodev_conns_lock);
> -	list_splice(&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);

[Severity: Low]
The commit message describes the whole change as groundwork:

    That is fine for as long as rds_conn_destroy() frees the connection
    before it returns

and:

    nothing in this tree destroys a connection behind the sweep's back
    today, so this is preparation for those patches rather than a fix.

For IB, does this hunk also fix a race that exists before this patch?

The old list_splice() never re-initialised ib_nodev_conns. Loopback, by
contrast, follows its splice with INIT_LIST_HEAD(&loop_conns). After the
splice, ib_nodev_conns.next and .prev still point at nodes that now sit
on the stack tmp_list.

In rds_ib_exit(), rds_ib_unregister_client() leads to
rds_ib_dev_shutdown()->rds_conn_path_drop(cp, true), which queues the
shutdown on the per-path cp_wq. The flush_workqueue(rds_wq) in
rds_ib_unregister_client() doesn't flush cp_wq, so this path can overlap
the sweep:

rds_ib_conn_path_shutdown()
  rds_ib_remove_conn()
    list_add_tail(&ic->ib_node, &ib_nodev_conns);

Because it goes through the stale head, that insert links the node after
the last entry of tmp_list. That entry's ->next then points to
&ib_nodev_conns rather than &tmp_list.

The lockless walk in rds_ib_destroy_nodev_conns() would then treat the
global head as an rds_ib_connection and pass garbage to
rds_conn_destroy().

In the same way, a connect worker's rds_ib_add_conn() could list_del() a
spliced node off tmp_list during the unlocked walk. The commit message
mentions this overlap itself ("Either can run while the sweep holds the
node on its stack list").

Both cases are closed by the per-entry list_move_tail() under
ib_nodev_conns_lock together with the i_ib_node_detached claim.

Could the commit message say that the IB part fixes an existing list
corruption race on module unload? It may also be worth adding a Fixes:
tag so stable backporters can find it.

[ ... ]

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