Re: [PATCH net-next v2 5/9] 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 4 potential
issue(s) to consider:

Critical: 0 · High: 2 · Medium: 1 · Low: 1

- [High] rds_ib_destroy_nodev_conns() (net/rds/ib_rdma.c:161) moves the
  global nodev list with list_splice(&ib_nodev_conns, &tmp_list) at line…
- [High] The fix is incomplete: the teardown helpers still walk the
  gathered stack list and dereference the transport node without holding…
- [Medium] The new list_del_init(&ic->ib_node) in
  rds_ib_destroy_nodev_conns() (net/rds/ib_rdma.c:178) leaves a still-…
- [Low] Commit message describes a live memory-corruption bug that is not
  reachable at this commit.

--

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

The transport teardown helpers gather per-connection transport nodes onto a
list head on their own stack and call rds_conn_destroy() for each.  This
patch moves the node unlink out of the transport conn_free() callbacks and
into the teardown loops, under the transport lock, so that nothing is left
on the stack list once the helper returns.

A few questions on the change and on the commit message below.

Starting with the commit message:

> That was fine while rds_conn_destroy() freed the connection before it
> returned.  Once the free is governed by the connection's reference
> count, a holder that outlives the teardown loop - a socket's cached
> rs_conn, an inc parked on a receive queue - defers conn_free() until
> after the helper has returned, and the list_del() then writes the
> neighbours' pointers into a stack frame that no longer exists.

[Severity: Low]
Is that holder reachable at this commit?  rds_conn_get() has no callers in
net/rds at this revision, so the reference taken by __rds_conn_create() is
the only one and the rds_conn_put() at the end of rds_conn_destroy():

net/rds/connection.c:rds_conn_destroy() {
	...
	/* drop the initial reference; the connection is freed from
	 * rds_conn_destroy_fini() once every holder has dropped theirs
	 */
	rds_conn_put(conn);
}

is always the last one.  rds_conn_destroy_fini() -> rds_conn_path_free() ->
trans->conn_free() therefore still runs inside the teardown loop here, and
no list_del() can land on a dead frame yet.

The named holders (a socket's cached rs_conn, an inc on a receive queue)
only appear with the later patches in the series that add rds_conn_get()
calls in recv.c and send.c.

Could the message say that this is a prerequisite for those patches rather
than a fix for a currently reachable corruption?  As written, a backporter
would read it as a standalone fix, and there is no Fixes: tag to anchor it.

> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 4feb0edc360c8..de5759c50b89a 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -1282,7 +1282,9 @@ void rds_ib_conn_free(void *arg)
>  	lock_ptr = ic->rds_ibdev ? &ic->rds_ibdev->spinlock : &ib_nodev_conns_lock;
>  
>  	spin_lock_irq(lock_ptr);
> -	list_del(&ic->ib_node);
> +	/* already unlinked if a transport teardown gathered us first */
> +	if (!list_empty(&ic->ib_node))
> +		list_del(&ic->ib_node);
>  	spin_unlock_irq(lock_ptr);

[Severity: Medium]
This makes an empty ib_node mean "already detached", but two assertions in
ib_rdma.c still treat an empty ib_node on a live connection as impossible:

net/rds/ib_rdma.c:rds_ib_add_conn() {
	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);
	...
}

net/rds/ib_rdma.c:rds_ib_remove_conn() {
	spin_lock_irq(&rds_ibdev->spinlock);
	BUG_ON(list_empty(&ic->ib_node));
	list_del(&ic->ib_node);
	...
}

After the sweep in rds_ib_destroy_nodev_conns() does list_del_init(), the ic
is still live: c_destroy_in_prog is only set once rds_conn_destroy() is
entered, and the path workers have not been quiesced yet.  A connect worker
already running on that connection's own cp_wq (allocated per connection in
__rds_conn_create(), so not drained by the flush_workqueue(rds_wq) in
rds_ib_unregister_client()) reaches rds_ib_conn_path_connect() ->
rds_ib_setup_qp() -> rds_ib_add_conn() and would hit the first BUG_ON.

If instead the connect wins and moves the node onto rds_ibdev->conn_list,
the sweep empties it again and the following rds_ib_conn_path_shutdown() ->
rds_ib_remove_conn() hits the second BUG_ON.

TCP sidesteps this by carrying an explicit t_tcp_node_detached flag rather
than overloading list emptiness.  Would an equivalent flag, or dropping the
two BUG_ONs, be preferable for IB?

> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index db7e92e7bd29f..b30f2a3715878 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c
> @@ -168,8 +168,18 @@ void rds_ib_destroy_nodev_conns(void)
>  	list_splice(&ib_nodev_conns, &tmp_list);
>  	spin_unlock_irq(&ib_nodev_conns_lock);
>  
> -	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
> +	/* 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) {
> +		spin_lock_irq(&ib_nodev_conns_lock);
> +		list_del_init(&ic->ib_node);
> +		spin_unlock_irq(&ib_nodev_conns_lock);
> +
>  		rds_conn_destroy(ic->conn);
> +	}
>  }

[Severity: High]
Does the splice here leave ib_nodev_conns pointing at the gathered nodes?
There is no INIT_LIST_HEAD(&ib_nodev_conns) after the list_splice(), unlike
the loopback side in this same patch:

net/rds/loop.c:rds_loop_exit() {
	spin_lock_irq(&loop_conns_lock);
	list_splice(&loop_conns, &tmp_list);
	INIT_LIST_HEAD(&loop_conns);
	spin_unlock_irq(&loop_conns_lock);
	...
}

list_splice() only relinks the entries, so ib_nodev_conns.next still points
at the first gathered ib_node and .prev at the last, and the new
list_del_init() repairs tmp_list only - it leaves each gathered node
self-linked and never touches the global head.

rds_ib_exit() then calls this helper again as the resweep callback:

net/rds/ib.c:rds_ib_exit() {
	...
	rds_ib_destroy_nodev_conns();
	rds_conn_wait_conns_freed(&rds_ib_transport,
				  rds_ib_destroy_nodev_conns);
	...
}

and rds_conn_wait_conns_freed() invokes resweep() on every poll timeout
while t_conn_count is non-zero:

net/rds/connection.c:rds_conn_wait_conns_freed() {
	while (!wait_event_timeout(rds_conn_freed_waitq,
				   !atomic_read(&trans->t_conn_count),
				   msecs_to_jiffies(RDS_CONN_FREE_POLL_MS))) {
		if (resweep)
			resweep();
	...
}

On that second call, does __list_splice() write first->prev / last->next
into rds_ib_connection objects that rds_ib_conn_free() may already have
kfree()d?  And with the first gathered node self-linked, does
list_for_each_entry_safe() compute n == pos on every pass, so the sweep
never terminates and keeps calling rds_conn_destroy(ic->conn) on a conn
pointer read out of freed memory?

Separately, a late arrival through the stale head:

net/rds/ib_rdma.c:rds_ib_remove_conn() {
	...
	list_add_tail(&ic->ib_node, &ib_nodev_conns);
	...
}

dereferences ib_nodev_conns.prev, which after the sweep points at a
gathered node.  Is that write safe once that node has been freed?

> diff --git a/net/rds/tcp.c b/net/rds/tcp.c
> index 826e4629e4eee..a71d6a4f0939c 100644
> --- a/net/rds/tcp.c
> +++ b/net/rds/tcp.c
> @@ -502,6 +502,28 @@ static bool rds_tcp_is_unloading(struct rds_connection *conn)
>  	return atomic_read(&rds_tcp_unloading) != 0;
>  }
>  
> +/* Destroy the connections whose nodes were gathered on @tmp_list.
> + *
> + * rds_conn_destroy() can return before the connection is freed, and
> + * it is the free - rds_tcp_conn_free() - that unlinks t_tcp_node.
> + * Since @tmp_list lives on the caller's stack, unlink each node here
> + * and mark it detached before its destroy, so that a free that runs
> + * after the caller has returned does not write into a dead frame.
> + */
> +static void rds_tcp_destroy_gathered_conns(struct list_head *tmp_list)
> +{
> +	struct rds_tcp_connection *tc, *_tc;
> +
> +	list_for_each_entry_safe(tc, _tc, tmp_list, t_tcp_node) {
> +		spin_lock_irq(&rds_tcp_conn_lock);
> +		list_del_init(&tc->t_tcp_node);
> +		tc->t_tcp_node_detached = true;
> +		spin_unlock_irq(&rds_tcp_conn_lock);
> +
> +		rds_conn_destroy(tc->t_cpath->cp_conn);
> +	}
> +}

[Severity: High]
Does this still leave the not-yet-visited entries exposed?  Only the node
the cursor is currently on gets detached; every other tc parked on tmp_list
still has t_tcp_node_detached == false, so a concurrent last put takes the
list_del() path here and frees the object:

net/rds/tcp.c:rds_tcp_conn_free() {
	spin_lock_irqsave(&rds_tcp_conn_lock, flags);
	if (!tc->t_tcp_node_detached)
		list_del(&tc->t_tcp_node);
	spin_unlock_irqrestore(&rds_tcp_conn_lock, flags);

	kmem_cache_free(rds_tcp_conn_slab, tc);
}

The walk holds no connection reference, so can the iterator then read
list_next_entry() out of freed slab memory, write list_del_init() and
tc->t_tcp_node_detached into it, and call rds_conn_destroy() on a conn
pointer fetched from it?

There also looks to be a window on the current node: after list_del_init()
nothing pins tc, so can a racing last put free it before
tc->t_cpath->cp_conn is evaluated on the next statement?

rds_loop_destroy_gathered_conns() in loop.c and the reworked
rds_ib_destroy_nodev_conns() in ib_rdma.c walk their stack lists the same
way, with lc->conn and ic->conn read after the unlink.

The freeing side seems reachable for a gathered connection whose destroy
already ran - rds_conn_destroy() notes this itself:

net/rds/connection.c:rds_conn_destroy() {
	...
	 * a conn can be handed to rds_conn_destroy() more than once -
	 * e.g. dropped for a protocol version mismatch and then found
	 * again at module unload.
	...
}

For such a connection the free is governed entirely by a remaining holder,
which can drop it at any point during the walk.  Would taking
rds_conn_get() on each gathered connection under the transport lock and
putting it after rds_conn_destroy() cover both the current and the pending
entries?

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