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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The `rds_destroy_pending(conn)` check introduced by the patch is lockless with respect to connection teardown, leaving a Time-Of-Check to Time-Of-Use (TOCTOU) race condition that renders the patch's attempt to fix the Use-After-Free ineffective.
--

commit 128ba6b33f50fd05797877892e71aaf3ccd2f612
Author: Allison Henderson <achender@kernel.org>

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

This commit attempts to prevent a use-after-free by refusing to attach an
accepted socket to a connection whose teardown has already begun, doing so
by checking rds_destroy_pending().

> diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
> index dcac10a91a67f..e22ea9ca8c1ce 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)) {

[Severity: Critical]
Does evaluating rds_destroy_pending() without an RCU read-side critical
section leave a Time-Of-Check to Time-Of-Use race condition?

Looking at rds_conn_destroy() in net/rds/connection.c, the teardown
sequence sets the c_destroy_in_prog flag and calls synchronize_rcu().
Because rds_tcp_accept_one() isn't holding rcu_read_lock() here,
synchronize_rcu() will not wait for it.

If rds_tcp_accept_one() gets preempted immediately after this check, could
rds_conn_destroy() run concurrently on another CPU, complete the teardown
while the socket is still NULL, and free the connection?

When rds_tcp_accept_one() resumes, it appears it would install the new
socket onto the connection and drop its reference via rds_conn_put().
This would leave the socket's callbacks and sk_user_data pointing to
freed memory, leading to the use-after-free this patch intends to prevent.

>  		rds_conn_path_drop(cp, 0);
>  		goto rst_nsk;
>  	}

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