Re: [PATCH net-next v2 8/9] net/rds: drop rds_conn_count in favor of t_conn_count

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Leaking transport data for multipath connections in passive connection error paths.
--

commit 149715f475fee80907a20e835a5065660d049fe5
Author: Allison Henderson <achender@kernel.org>

net/rds: drop rds_conn_count in favor of t_conn_count

The previous patch gave each transport its own connection count in
t_conn_count, incremented and decremented at exactly the points where
the global rds_conn_count is.  This switches the name seed to
t_conn_count, and removes rds_conn_count.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 1ec141d6faa38..11813f93961d1 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -392,7 +392,6 @@ static struct rds_connection *__rds_conn_create(struct net *net,
>  			rds_conn_get(conn);	/* caller */
>  			rcu_assign_pointer(parent->c_passive, conn);
>  			rds_cong_add_conn(conn);
> -			rds_conn_count++;

[Severity: High]
This isn't a bug introduced by this patch, but this section of
__rds_conn_create() has a pre-existing memory leak when tearing down
passive connections.

When establishing a passive connection, if the parent's destroy has begun
or another passive connection won the race, the error paths only free
the transport data for path 0:

	if (parent) {
		/* Creating passive conn */
		if (READ_ONCE(parent->c_destroy_in_prog)) {
			...
			trans->conn_free(conn->c_path[0].cp_transport_data);
			...
		} else if (rcu_access_pointer(parent->c_passive)) {
			...
			trans->conn_free(conn->c_path[0].cp_transport_data);
			...

Since conn_alloc() can allocate transport data for multiple paths (npaths),
doesn't freeing only c_path[0] permanently leak the transport-specific
connection data (like rds_tcp_connection) for paths 1 through npaths-1?

The normal connection error path just below this handles it correctly by
looping over all npaths:

		if (found) {
			for (i = 0; i < npaths; i++) {
				cp = &conn->c_path[i];
				if (cp->cp_transport_data)
					trans->conn_free(cp->cp_transport_data);
			}
		}

Could the passive error paths also be updated to iterate over all paths and
prevent this resource leak?

>  			atomic_inc(&conn->c_trans->t_conn_count);
>  		}
>  	} else {

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