Re: [PATCH net-next v3 10/13] net/rds: tcp: don't attach an accepted socket to a connection being destroyed

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

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

- [Low] Incomplete at this point in the series: the identical hole in the
  IB accept path (rds_ib_cm_handle_connect(), net/rds/ib_cm.c) is left…
- [Low] The commit message describes a present-tense use-after-free plus
  socket leak ('the next byte from the peer runs the socket callbacks…

--

net/rds: tcp: don't attach an accepted socket to a connection being destroyed

rds_tcp_accept_one() now refuses an incoming accept when the target
connection's destroy has already begun, instead of claiming a DOWN path
and installing the accepted socket on it.  The refusal reuses the existing
unexpected-path-state path: drop the path claim and reset the new socket.

> commit message

>     A connection whose destroy has begun is quiesced - its paths are DOWN
>     and its old sockets released - but stays allocated while a reference
>     holder is still around, and rds_conn_create() hands out exactly such
>     a connection with a reference of its own.

[Severity: Low]
The described reachability appears to depend on earlier patches in this
same series rather than on any released kernel.

The connection surviving its own destroy while a holder exists comes from
"net/rds: split connection destroy into quiesce and kref-governed free",
rds_conn_lookup()/__rds_conn_create() handing out a reference on such a
connection comes from "net/rds: hold connection references in lookup,
sockets and c_passive", and the c_destroy_in_prog term the new test relies
on comes from "net/rds: make rds_destroy_pending() cover single-connection
destroy":

net/rds/rds.h:rds_destroy_pending() {
	return READ_ONCE(conn->c_destroy_in_prog) ||
	       !check_net(rds_conn_net(conn)) ||
	       (conn->c_trans->t_unloading && conn->c_trans->t_unloading(conn));
}

Applied on its own to a tree without those changes, the new test reduces to
the netns and unloading cases and does not close the hole described here.

There is no Fixes: tag and no note about the in-series dependency, so the
message reads as a standalone fix.  Sibling patches in the series that
close the same class of in-series hole ("net/rds: unlink transport nodes
before a possibly deferred connection free", "net/rds: refuse to queue a
message on a connection being destroyed", "net/rds: pin the connection
across RDMA-CM event handling") also carry no Fixes: tag and no stable
note, so this matches the series convention.

Would it be worth stating in the message that the reachability is created
by earlier patches in this series, so an automatic stable pick does not
land a change that cannot achieve its stated purpose there?

> diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
> index dcac10a91a67..e22ea9ca8c1c 100644
> --- a/net/rds/tcp_listen.c
> +++ b/net/rds/tcp_listen.c
> @@ -278,7 +278,15 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
>  	cp = rs_tcp->t_cpath;
>  	conn_state = rds_conn_path_state(cp);
>  	WARN_ON(conn_state == RDS_CONN_UP);
> -	if (conn_state != RDS_CONN_CONNECTING && conn_state != RDS_CONN_ERROR) {
> +	/* A connection whose destroy has begun has been quiesced and is
> +	 * only waiting for its last reference: its paths sit in
> +	 * RDS_CONN_DOWN, which rds_tcp_accept_one_path() happily claims.
> +	 * Installing a socket on it would leave sk_user_data pointing
> +	 * at a path that is about to be freed.
> +	 */
> +	if (rds_destroy_pending(conn) ||
> +	    (conn_state != RDS_CONN_CONNECTING &&
> +	     conn_state != RDS_CONN_ERROR)) {
>  		rds_conn_path_drop(cp, 0);
>  		goto rst_nsk;
>  	}

[Severity: Low]
Should the IB accept path get the same guard in this commit?

rds_ib_cm_handle_connect() in net/rds/ib_cm.c reaches the same place by the
same route: it takes a connection from rds_conn_create(), makes the same
DOWN -> CONNECTING claim that a quiesced-but-still-referenced connection
satisfies, and then installs transport state on it:

net/rds/ib_cm.c:rds_ib_cm_handle_connect() {
	conn = rds_conn_create(&init_net, daddr6, saddr6,
			       &rds_ib_transport, dp_cmn->ricpc_dp_toss,
			       GFP_KERNEL, ifindex);
	...
	mutex_lock(&conn->c_cm_lock);
	if (!rds_conn_transition(conn, RDS_CONN_DOWN, RDS_CONN_CONNECTING)) {
	...
	ic->i_cm_id = cm_id;
	cm_id->context = conn;
	...
	err = rds_ib_setup_qp(conn);
}

At this commit there is no rds_destroy_pending() test anywhere in
net/rds/ib_cm.c, and the hashed rds_conn_lookup() branch of
__rds_conn_create() does not filter c_destroy_in_prog - only the c_passive
branch does:

net/rds/connection.c:__rds_conn_create() {
		conn = rcu_dereference(parent->c_passive);
		if (conn && READ_ONCE(conn->c_destroy_in_prog))
			conn = NULL;
}

So can an incoming RDMA_CM_EVENT_CONNECT_REQUEST attach a fresh cm_id and
QP to a connection that is only waiting for its last reference, leaving
cm_id->context pointing at freed memory and the QP untorn-down?

The very next patch in the series, "net/rds: pin the connection across
RDMA-CM event handling" (3e53d58bba13ea8bec2e328d358bbebdacca18ef), adds
exactly that guard under c_cm_lock:

	if (rds_destroy_pending(conn))
		goto out;

so the gap closes before the end of the series.  What remains is the
bisect window: the enabling reference behaviour arrives two patches earlier
in "net/rds: hold connection references in lookup, sockets and c_passive",
which leaves three commits where a bisect lands on a tree with the IB side
open.  Would folding the IB check in here, or reordering the two patches,
avoid that window?  A note in the message that the IB sibling is handled
separately would also help.

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