Re: [PATCH net-next v5 07/12] 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 2 potential
issue(s) to consider.

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] net/rds/ib_rdma.c:rds_ib_add_conn(): the new `if
  (!ic->i_ib_node_detached)` guard covers only the ib_node list…
- [Low] rds_ib_conn_free()'s header comment (net/rds/ib_cm.c:1278-1281)
  still asserts a two-state invariant and denies the very race this…

--

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

The transport teardown helpers now unlink each per-connection transport node
under the transport lock right before calling rds_conn_destroy(), so a
conn_free() deferred past the helper cannot list_del() from a stack frame
that no longer exists.  IB gets an explicit i_ib_node_detached flag and the
two BUG_ON()s in rds_ib_add_conn()/rds_ib_remove_conn() are removed, and the
gather loops now take a connection reference for each node they move.

A couple of questions about the IB side below.

> diff --git a/net/rds/ib.h b/net/rds/ib.h
> index 5ff346a1e8ba..cb410c3ae8d8 100644
> --- a/net/rds/ib.h
> +++ b/net/rds/ib.h
> @@ -143,6 +143,10 @@ struct rds_ib_device;
>  struct rds_ib_connection {
>  
>  	struct list_head	ib_node;
> +	/* set under ib_nodev_conns_lock once a transport teardown has
> +	 * claimed ib_node; from then on only the teardown touches it
> +	 */
> +	bool			i_ib_node_detached;
>  	struct rds_ib_device	*rds_ibdev;
>  	struct rds_connection	*conn;
>  
> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 53147793d44b..89340ecc3116 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -1287,7 +1287,9 @@ void rds_ib_conn_free(void *arg)
>  	lock_ptr = ic->rds_ibdev ? &ic->rds_ibdev->spinlock : &ib_nodev_conns_lock;
>  
>  	spin_lock_irqsave(lock_ptr, flags);
> -	list_del(&ic->ib_node);
> +	/* a transport teardown that gathered us first owns the node */
> +	if (!ic->i_ib_node_detached)
> +		list_del(&ic->ib_node);
>  	spin_unlock_irqrestore(lock_ptr, flags);
>  
>  	rds_ib_recv_free_caches(ic);

[Severity: Low]
This isn't a bug, but the comment a few lines above the new check, in
rds_ib_conn_free(), still describes a world this patch has changed:

net/rds/ib_cm.c:rds_ib_conn_free() {
	/*
	 * Conn is either on a dev's list or on the nodev list.
	 * A race with shutdown() or connect() would cause problems
	 * (since rds_ibdev would change) but that should never happen.
	 */
	lock_ptr = ic->rds_ibdev ? &ic->rds_ibdev->spinlock : &ib_nodev_conns_lock;

After this patch there is a third state: the node sits on
rds_ib_destroy_nodev_conns()'s stack-local tmp_list with
i_ib_node_detached set, and is then list_del_init()'ed, so the connection
is on neither a device list nor the nodev list.

The commit message also says "a connect or shutdown worker can still be
running for a connection the sweep has claimed", which is the
shutdown()/connect() race this comment says should never happen.

Could the comment be updated to describe the detached state, and to note
that the lock choice only matters while i_ib_node_detached is false?

> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index a9b27f06cbfc..1548e5be0e55 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 has claimed it ahead of destroying it: then it is on its
> +	 * way out, and its node belongs to the sweep.
> +	 */
>  	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 (!ic->i_ib_node_detached) {
> +		list_del(&ic->ib_node);
>  
> -	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);
>  
>  	ic->rds_ibdev = rds_ibdev;

[Severity: High]
The new guard covers only the list movement, but the two statements that
follow still run for a connection the sweep has already claimed:

net/rds/ib_rdma.c:rds_ib_add_conn() {
	spin_unlock_irq(&ib_nodev_conns_lock);

	ic->rds_ibdev = rds_ibdev;
	refcount_inc(&rds_ibdev->refcount);
}

Can this leak the rds_ib_device reference?

rds_ib_add_conn() is reached from rds_ib_setup_qp() via the RDMA-CM event
handlers, rds_ib_cm_initiate_connect() on the active side and
rds_ib_cm_handle_connect() on the passive side, which the sweep does not
serialize against.

The only site that clears ic->rds_ibdev and drops that reference is
rds_ib_conn_path_shutdown():

net/rds/ib_cm.c:rds_ib_conn_path_shutdown() {
	...
	if (ic->rds_ibdev)
		rds_ib_remove_conn(ic->rds_ibdev, conn);
	...
}

and rds_ib_remove_conn() ends with ic->rds_ibdev = NULL plus
rds_ib_dev_put().  Once the sweep's rds_conn_destroy() has quiesced the
paths, rds_conn_path_drop(cp, false) is gated by rds_destroy_pending(), so
no further shutdown pass runs for that connection.

rds_ib_conn_free() never calls rds_ib_dev_put(), so does an add_conn() that
lands after the quiesce leave ic->rds_ibdev set forever, with
rds_ibdev->refcount never reaching zero?  That would mean rds_ib_dev_free()
is never queued and the device struct, its PD and its 1M/8K MR pools are
never released, which rds_ib_exit()'s flush_workqueue(rds_wq) cannot
recover.  The QP, CQs and cm_id created by that late rds_ib_setup_qp() also
look like they stay around, since rdma_destroy_id() only happens in
rds_ib_conn_path_shutdown().

There is a second effect in the same window: the connection is bound to the
HCA but deliberately kept off rds_ibdev->conn_list, and device removal only
walks that list:

net/rds/ib.c:rds_ib_dev_shutdown() {
	spin_lock_irqsave(&rds_ibdev->spinlock, flags);
	list_for_each_entry(ic, &rds_ibdev->conn_list, ib_node)
		rds_conn_path_drop(&ic->conn->c_path[0], true);
	spin_unlock_irqrestore(&rds_ibdev->spinlock, flags);
}

Before this patch the node was still moved onto the device conn_list here,
so ib_device removal could at least find it.  Is losing that intentional?

And in the reverse interleaving, rds_ib_conn_path_shutdown() tests
ic->rds_ibdev without a lock and then asserts on it:

net/rds/ib_cm.c:rds_ib_conn_path_shutdown() {
		if (ic->rds_ibdev)
			rds_ib_remove_conn(ic->rds_ibdev, conn);
		...
	}
	BUG_ON(ic->rds_ibdev);

Can a concurrent rds_ib_add_conn() for the claimed connection store
ic->rds_ibdev between that test and the BUG_ON, making it fire?

For what it is worth, at the end of the series the later patch "net/rds: pin
the connection across RDMA-CM event handling" adds an
rds_destroy_pending() check only to rds_ib_cm_handle_connect(), so the
active path rds_ib_cm_initiate_connect() -> rds_ib_setup_qp() ->
rds_ib_add_conn() still looks unguarded, and rds_ib_add_conn() and
rds_ib_conn_free() are otherwise unchanged there.

> @@ -163,13 +173,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;
> +
> +	/* 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.  Marking the
> +	 * node detached claims it for this sweep: rds_ib_add_conn(),
> +	 * rds_ib_remove_conn() and rds_ib_conn_free() leave a claimed
> +	 * node alone, so the walk over tmp_list below needs no lock.
> +	 * 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)) {
> +			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);
> +	}
>  }

[ ... ]

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