Re: [PATCH net-next v4 07/12] net/rds: unlink transport nodes before a possibly deferred connection free

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rds: list corruption in rds_ib_destroy_nodev_conns() due to lockless tmp_list iteration
--

commit 74390d0096652a342da459a13d451c2fb0ee6d1f
Author: Allison Henderson <achender@kernel.org>
Subject: net/rds: unlink transport nodes before a possibly deferred connection free

This patch modifies transport teardown helpers in RDS to unlink transport nodes
right before calling rds_conn_destroy(). It ensures that nodes don't remain on
stack-allocated list heads when connections are kept alive by external
references.

> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index a9b27f06cbfcf..bdd66c9b93c70 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c
> @@ -123,15 +123,18 @@ void rds_ib_add_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *con
>  {
>  	struct rds_ib_connection *ic = conn->c_transport_data;
>  
> -	/* conn was previously on the nodev_conns_list */
> +	/* conn was previously on the nodev_conns_list, unless a teardown
> +	 * sweep already unlinked it ahead of destroying it: then it is
> +	 * on its way out and stays off every 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);
> +	if (!list_empty(&ic->ib_node)) {
> +		list_del(&ic->ib_node);

[Severity: High]
Does this correctly identify if a node was already unlinked by a teardown
sweep? 

Since the teardown sweep in rds_ib_destroy_nodev_conns() moves the node to a
local tmp_list, list_empty(&ic->ib_node) will evaluate to false here.

Could this cause list_del(&ic->ib_node) to inadvertently unlink the node
from tmp_list while the teardown sweep is actively iterating over it?

>  
> -	spin_lock(&rds_ibdev->spinlock);
> -	list_add_tail(&ic->ib_node, &rds_ibdev->conn_list);
> -	spin_unlock(&rds_ibdev->spinlock);
> +		spin_lock(&rds_ibdev->spinlock);
> +		list_add_tail(&ic->ib_node, &rds_ibdev->conn_list);
> +		spin_unlock(&rds_ibdev->spinlock);
> +	}
>  	spin_unlock_irq(&ib_nodev_conns_lock);

[ ... ]

> @@ -163,13 +173,38 @@ 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;
> +
> +	/* Gather the connections and take a reference on each, so that
> +	 * none is freed under the walk below (a connection destroyed
> +	 * earlier, for a protocol version mismatch, can be on this list
> +	 * with only a socket's reference still pending).  One whose free
> +	 * is already running gets no reference: its free unlinks the
> +	 * node itself, under this lock, once we drop it.  Avoid calling
> +	 * conn_destroy with irqs off.
> +	 */
>  	spin_lock_irq(&ib_nodev_conns_lock);
> -	list_splice_init(&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))
> +			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 unlinks ib_node.
> +	 * tmp_list lives on this stack frame, so unlink each node before
> +	 * its destroy; the free then finds it empty and leaves it alone.
> +	 */
> +	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node) {
> +		conn = ic->conn;
> +
> +		spin_lock_irq(&ib_nodev_conns_lock);
> +		list_del_init(&ic->ib_node);
> +		spin_unlock_irq(&ib_nodev_conns_lock);

[Severity: High]
Is it safe to iterate over tmp_list locklessly here?

If a concurrent worker calls rds_ib_add_conn(), it will unlink the node from
tmp_list. If the unlinked node happens to be the prefetched _ic pointer in
this loop, its next and prev pointers become LIST_POISON1 and LIST_POISON2,
which can cause a kernel panic on the next loop iteration.

Additionally, if the concurrent rds_ib_add_conn() has already moved the node
onto rds_ibdev->conn_list, could the list_del_init(&ic->ib_node) here
improperly modify rds_ibdev->conn_list without holding rds_ibdev->spinlock?

> +
> +		rds_conn_destroy(conn);
> +		rds_conn_put(conn);
> +	}
>  }

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