Re: [PATCH net-next v9 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 carries 'Fixes: 4716af3897e9', which makes it a stable
  candidate.
- [Low] Parts of the commit message are inaccurate or garbled.

Pre-existing issues:
- [Medium] At this commit, rds_ib_exit() still sweeps ib_nodev_conns only
  once, through rds_ib_destroy_nodev_conns(), right after…

--

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

This patch changes the RDS TCP, loopback and IB teardown helpers so they
unlink each transport node from the on-stack gather list before calling
rds_conn_destroy(). A deferred conn_free() then cannot write into a dead
stack frame. For IB it adds an i_ib_node_detached claim flag and gathers
the nodev list entry by entry under ib_nodev_conns_lock, instead of
splicing it, and it pins each gathered connection with a reference.

> The node is unlinked much later, by the transport's conn_free():
> rds_tcp_conn_free() and rds_loop_conn_free() list_del() it, and
> rds_ib_conn_free() does so unconditionally.

[Severity: Low]
Is this accurate for rds_loop_conn_free()? Before this patch it also
called list_del() unconditionally:

net/rds/loop.c:rds_loop_conn_free() {
	...
	spin_lock_irqsave(&loop_conns_lock, flags);
	list_del(&lc->loop_node);
	...
}

Only rds_tcp_conn_free() checked t_tcp_node_detached first.

> today, so that part is preparation for those patches.  The IB list
> handling is a fix in its own right, below.
> So the gather takes a reference on each connection it moves

The paragraph above this one describes the IB list fix ("For IB this also
closes a race that exists today..."). Should "below" be "above"?

This sentence also falls between "that part is preparation for those
patches" and the sentence that continues it, "So the gather takes a
reference...". It looks like it was left over from an edit.

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

[ ... ]

> @@ -155,15 +159,22 @@ void rds_ib_remove_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *
>  {
>  	struct rds_ib_connection *ic = conn->c_transport_data;
>  
> -	/* place conn on nodev_conns_list */
> +	bool detached;
> +
> +	/* place conn on nodev_conns_list - unless a teardown sweep has
> +	 * claimed it ahead of destroying it, in which case its node
> +	 * belongs to the sweep
> +	 */
>  	spin_lock(&ib_nodev_conns_lock);
>  
>  	spin_lock_irq(&rds_ibdev->spinlock);
> -	BUG_ON(list_empty(&ic->ib_node));
> -	list_del(&ic->ib_node);
> +	detached = ic->i_ib_node_detached;
> +	if (!detached)
> +		list_del(&ic->ib_node);
>  	spin_unlock_irq(&rds_ibdev->spinlock);
>  
> -	list_add_tail(&ic->ib_node, &ib_nodev_conns);
> +	if (!detached)
> +		list_add_tail(&ic->ib_node, &ib_nodev_conns);

[Severity: Medium]
This is a pre-existing issue, not one introduced by this patch. Can a late
shutdown leave a connection on ib_nodev_conns that is never destroyed?

At this commit rds_ib_exit() still sweeps the nodev list only once:

net/rds/ib.c:rds_ib_exit() {
	...
	rds_ib_unregister_client();
	rds_ib_destroy_nodev_conns();
	rds_ib_sysctl_exit();
	rds_ib_recv_exit();
	...
}

rds_ib_unregister_client() only waits on rds_wq:

	ib_unregister_client(&rds_ib_client);
	/* wait for rds_ib_dev_free() to complete */
	flush_workqueue(rds_wq);

rds_ib_dev_shutdown() queues the shutdowns on the per-path cp_wq and does
not wait for them:

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

A connection whose shutdown has not yet reached rds_ib_remove_conn() is
still on rds_ibdev->conn_list, so rds_ib_destroy_nodev_conns() never
claims it. When that shutdown finishes, i_ib_node_detached is false. The
node is added to ib_nodev_conns here after the only sweep has already run,
and rds_conn_destroy() is never called for it.

The rds_ib_dev_put() that follows can also queue rds_ib_dev_free() on
rds_wq after the flush has returned. Both paths can end up running module
text after rds_rdma is unloaded.

This commit does not claim to fix this; its "closes both" refers to the
stale head corruption and to a connect unlinking a node under the walk.
The next patch in the series, "net/rds: wait for connections to be freed
on transport unload", fixes it. There, rds_ib_exit() follows the sweep
with rds_conn_wait_conns_freed(&rds_ib_transport,
rds_ib_destroy_nodev_conns), which sweeps again on every poll until
t_conn_count reaches zero, and then runs a second flush_workqueue(rds_wq).

>  
>  	spin_unlock(&ib_nodev_conns_lock);
>  

[ ... ]

> @@ -176,13 +187,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);
>  
> -	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
> -		rds_conn_destroy(ic->conn);
> +	/* rds_conn_destroy() can return before the connection is freed,
> +	 * and it is the free - rds_ib_conn_free() - that would unlink
> +	 * ib_node.  tmp_list lives on this stack frame, so take each node
> +	 * off it before its destroy; the free then leaves it alone.
> +	 */
> +	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node) {
> +		conn = ic->conn;
> +		list_del_init(&ic->ib_node);
> +
> +		rds_conn_destroy(conn);
> +		rds_conn_put(conn);
> +	}

[Severity: Low]
Because of the "Fixes: 4716af3897e9" tag, this commit will be a stable
candidate. However, rds_conn_get_unless_zero() and rds_conn_put() don't
exist at the series baseline. They come from the previous patch, "net/rds:
split connection destroy into quiesce and kref-governed free", which has
no Fixes: tag.

Would a backport to a tree that has 4716af3897e9 but not the kref rework
fail to build?

The commit message calls the per-entry references "preparation" and says
"The IB list handling is a fix in its own right". It does not name the
prerequisite, for example with a stable dependency note. Later in the
series, "net/rds: hold a connection reference from struct rds_incoming"
leaves out a Fixes: tag for this same reason.

The IB list fix (the entry-by-entry gather under ib_nodev_conns_lock plus
the i_ib_node_detached claim) does not need the references while frees
are still synchronous. Could it be split out as a standalone patch that
goes ahead of the kref rework?

>  }
>  
>  void rds_ib_get_mr_info(struct rds_ib_device *rds_ibdev, struct rds_info_rdma_connection *iinfo)

[ ... ]

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