Re: [PATCH net-next v6 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 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] Stale/unreachable justification for the new per-connection
  reference in rds_ib_destroy_nodev_conns(): both the commit message ("A…

--

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

The RDS transport teardown helpers now unlink each per-connection
transport node under the transport lock right before calling
rds_conn_destroy(), so a conn_free() that runs after the helper has
returned does not touch a list head on a dead stack frame.  IB gets an
explicit i_ib_node_detached flag, and the gather loops take a reference
on each connection they move onto the stack list.

> The walk itself must not lose the entries either.  A connection that
> was destroyed earlier - dropped for a protocol version mismatch, then
> found again at module unload - is kept alive only by whatever
> reference is still pending, and that can be dropped at any point
> during the walk, freeing the transport node the iterator is about to
> read.

[Severity: Low]
Is this scenario reachable in this tree?  A grep for rds_conn_destroy()
callers at this commit finds only the five teardown helpers:

  net/rds/ib_rdma.c:  rds_ib_destroy_nodev_conns()
  net/rds/loop.c:     rds_loop_destroy_gathered_conns()
  net/rds/tcp.c:      rds_tcp_destroy_gathered_conns()

There is no protocol-version-mismatch caller of rds_conn_destroy() any
more - net/rds/ib_cm.c has none, that path was converted to
rds_conn_drop() upstream by f97d8c7bab78 - so a connection cannot have
been "destroyed earlier ... for a protocol version mismatch".

The same commit message also says two paragraphs earlier:

> That is fine for as long as rds_conn_destroy() frees the connection
> before it returns, which is still the case at this point in the
> series: the initial reference is the only one.

and the sibling patch "net/rds: make rds_destroy_pending() report a
connection's own destroy" states that "Today every rds_conn_destroy()
does happen on one of those two global paths".

Could this paragraph be reworded to say that the reference is
preparation for the later patches that hand out references, rather than
describing a use-after-free that exists today?  As written, and with no
Fixes: tag, it reads like a fix for a live bug.

> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index a9b27f06cbfcf..1548e5be0e559 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c

[ ... ]

> @@ -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

[Severity: Low]
The same claim is repeated here in the comment on
rds_ib_destroy_nodev_conns(), so the stale justification ends up in the
source tree and stays there through the rest of the series.  Since
nothing calls rds_conn_destroy() for a protocol version mismatch, and
rds_ib_destroy_nodev_conns() is only reached from rds_ib_exit() and its
resweep, is there a caller that can drop a pending reference under this
walk today?

Would it be clearer to describe the rds_conn_get_unless_zero() here as
guarding against the deferred frees introduced later in the series?

> +	 * 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/20260922085410.391323-1-achender%40kernel.org