Re: [PATCH net-next v5 04/12] net/rds: make rds_destroy_pending() cover single-connection destroy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] rds: unnecessary synchronization logic based on false premise
--

commit 55cca7f8de5c5dd35f087d9e3e677bccea8d4c72
Author: Allison Henderson <achender@kernel.org>

    net/rds: make rds_destroy_pending() cover single-connection destroy
    
    The patch introduces a c_destroy_in_prog flag to synchronize connection
    teardown. It assumes rds_ib_cm_connect_complete() destroys single connections
    and aims to prevent requeueing work on a connection actively being destroyed
    outside of global teardown events.

> rds_conn_destroy() is not limited to the global paths:
> rds_ib_cm_connect_complete() destroys a single connection whose peer
> negotiated an unsupported protocol version.

[Severity: Medium]
Does rds_ib_cm_connect_complete() actually destroy the connection?

Code review shows rds_ib_cm_connect_complete() upstream calls rds_conn_drop(),
which merely transitions the connection state and schedules a reconnect, but does
not destroy the connection object:

net/rds/ib_cm.c:rds_ib_cm_connect_complete() {
    ...
	if (conn->c_version < RDS_PROTOCOL_VERSION) {
		if (conn->c_version != RDS_PROTOCOL_COMPAT_VERSION) {
			pr_notice("RDS/IB: Connection <%pI6c,%pI6c> version %u.%u no longer supported\n",
				  &conn->c_laddr, &conn->c_faddr,
				  RDS_PROTOCOL_MAJOR(conn->c_version),
				  RDS_PROTOCOL_MINOR(conn->c_version));
			rds_conn_drop(conn);
			return;
		}
	}
    ...
}

rds_conn_destroy() is exclusively called during global teardown
(module unload or netns destruction).

During global teardown, rds_destroy_pending() already evaluates to true
via !check_net() or t_unloading().

Is the stated premise invalid because the bug it fixes only existed in the Oracle
UEK tree (where single-connection destruction was present), making the ported
flag redundant for its stated purpose?

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index a96569a3ee9ad..242ca0570a47e 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -579,6 +579,14 @@ void rds_conn_destroy(struct rds_connection *conn)
>  		 "%pI4\n", conn, &conn->c_laddr,
>  		 &conn->c_faddr);
>  
> +	/* Make rds_destroy_pending() true for this conn.  Together with
> +	 * the synchronize_rcu() below this stops the work-requeueing
> +	 * sites (which all test rds_destroy_pending() under
> +	 * rcu_read_lock()) from queueing new work on the path
> +	 * workqueues once we start cancelling and destroying them.
> +	 */
> +	WRITE_ONCE(conn->c_destroy_in_prog, true);
> +

[Severity: Medium]
Is this synchronization flag actually necessary here?

Since rds_conn_destroy() is only called during module or netns teardown,
it appears rds_destroy_pending() already provides full coverage via
!check_net() and t_unloading() without needing this new c_destroy_in_prog flag.

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