Re: [PATCH net-next v2 6/9] net/rds: hold connection references in lookup, sockets and c_passive

[sashiko-bot@kernel.org]

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

New issues:
- [High] Linked-list corruption in cp_send_queue because rds_conn_path_quiesce() mutates it without holding cp_lock, racing with rds_sendmsg().
- [High] rds_tcp_accept_one() can attach a newly accepted socket to a dying RDS connection, leading to a Use-After-Free when the connection is freed.
- [Medium] Mismatched locks used for reading and writing `rs->rs_tos` in `rds_ioctl`.

Pre-existing issues:
- [High] rds_ib_conn_free() uses spin_unlock_irq(), which illegally enables interrupts while the caller __rds_conn_create() holds an outer irqsave spinlock.
--

commit d9df1c8410e812fbc2590e40004d81636ec5259e
Author: Sharath Srinivasan <sharath.srinivasan@oracle.com>

net/rds: hold connection references in lookup, sockets and c_passive

This patch introduces reference counting for struct rds_connection objects to
prevent use-after-free bugs. It hands out live references on connection
lookup, creation, and within the rs_conn sendmsg cache. The c_passive
connection pointer also now owns a reference that is dropped during parent
destruction.

Closes: https://syzkaller.appspot.com/bug?extid=879c1877016972360186

> diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
> index d5defe9172e36..63baac8aeb2d1 100644
> --- a/net/rds/af_rds.c
> +++ b/net/rds/af_rds.c
[ ... ]
> @@ -255,6 +263,7 @@ static int rds_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
[ ... ]
> -		spin_lock_bh(&rds_sock_lock);
> +		spin_lock_irqsave(&rs->rs_lock, flags);
>  		if (rs->rs_tos || rs->rs_conn) {
> -			spin_unlock_bh(&rds_sock_lock);
> +			spin_unlock_irqrestore(&rs->rs_lock, flags);
>  			return -EINVAL;
>  		}
>  		rs->rs_tos = tos;
> -		spin_unlock_bh(&rds_sock_lock);
> +		spin_unlock_irqrestore(&rs->rs_lock, flags);
>  		break;
>  	case SIOCRDSGETTOS:
>  		spin_lock_bh(&rds_sock_lock);
>  		tos = rs->rs_tos;
>  		spin_unlock_bh(&rds_sock_lock);

[Severity: Medium]
Could this introduce a data race on rs->rs_tos?

This patch updates the SIOCRDSSETTOS write path to use the new per-socket
rs->rs_lock, but the SIOCRDSGETTOS read path still uses the global
rds_sock_lock:

	spin_lock_bh(&rds_sock_lock);
	tos = rs->rs_tos;
	spin_unlock_bh(&rds_sock_lock);

Would concurrent ioctl calls with SIOCRDSSETTOS and SIOCRDSGETTOS on the
same socket cause a locking mismatch and potential KCSAN splat?

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 5848dd295911a..1ec141d6faa38 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
[ ... ]
> @@ -318,13 +353,44 @@ static struct rds_connection *__rds_conn_create(struct net *net,
>  	spin_lock_irqsave(&rds_conn_lock, flags);
>  	if (parent) {
>  		/* Creating passive conn */
> -		if (parent->c_passive) {
> +		if (READ_ONCE(parent->c_destroy_in_prog)) {
> +			/* The parent's destroy has begun (it sets the
> +			 * flag and snatches c_passive under this
> +			 * lock); do not install a new passive conn
> +			 * that nothing would ever destroy.
> +			 */
> +			trans->conn_free(conn->c_path[0].cp_transport_data);

[Severity: High]
This is a pre-existing issue, but does calling trans->conn_free() here 
unintentionally enable hardware interrupts?

__rds_conn_create() acquires rds_conn_lock using spin_lock_irqsave(),
disabling interrupts. When it calls trans->conn_free(), for IB connections
this invokes rds_ib_conn_free(), which uses spin_unlock_irq():

net/rds/ib_cm.c:rds_ib_conn_free() {
	...
	spin_lock_irq(lock_ptr);
	if (!list_empty(&ic->ib_node))
		list_del(&ic->ib_node);
	spin_unlock_irq(lock_ptr);
	...
}

Using spin_unlock_irq() unconditionally enables hardware interrupts, which
destroys the original irqsave state of the caller. Can this break mutual
exclusion or cause deadlocks?

[ ... ]
> @@ -696,7 +771,38 @@ void rds_conn_destroy(struct rds_connection *conn)
[ ... ]
>  	spin_unlock_irq(&rds_conn_lock);
> +	if (parent)
> +		rds_conn_put(parent);
>  	synchronize_rcu();
>  
>  	/* shut the connection down */

[Severity: High]
Can this concurrent teardown corrupt the cp_send_queue linked list?

With this patch, a connection can be quiesced via rds_conn_destroy() while
a send is in flight. During teardown, rds_conn_path_quiesce() is called and
locklessly iterates over and mutates the cp_send_queue:

net/rds/connection.c:rds_conn_path_quiesce() {
	...
	list_for_each_entry_safe(rm, rtmp, &cp->cp_send_queue, m_conn_item) {
		list_del_init(&rm->m_conn_item);
	...
}

Concurrently, a racing rds_sendmsg() can add messages to the queue using
the cp_lock:

net/rds/send.c:rds_send_queue_rm() {
	...
	spin_lock(&cp->cp_lock);
	list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
	...
	spin_unlock(&cp->cp_lock);
}

Does modifying this queue without cp_lock during teardown risk memory leaks
or kernel crashes when racing with active senders?

> diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
> index 8a0c54aced5ea..dcac10a91a67f 100644
> --- a/net/rds/tcp_listen.c
> +++ b/net/rds/tcp_listen.c
[ ... ]
> @@ -229,6 +229,7 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
>  
>  	if (IS_ERR(conn)) {
>  		ret = PTR_ERR(conn);
> +		conn = NULL;
>  		goto out;
>  	}
>  	/* An incoming SYN request came in, and TCP just accepted it.

[Severity: High]
Can rds_tcp_accept_one() attach a newly accepted socket to a dying connection?

This patch allows rds_conn_destroy() to run concurrently with active network
events. However, after creating or looking up a connection, this path lacks a
check for rds_destroy_pending(conn) before attaching the socket:

net/rds/tcp_listen.c:rds_tcp_accept_one() {
	...
	rds_tcp_set_callbacks(new_sock, cp);
	rds_connect_path_complete(cp, RDS_CONN_CONNECTING);
	...
}

If the connection's destruction has already begun, rds_tcp_accept_one() could
attach the socket and drop its reference while the connection is subsequently
freed. The socket would remain alive with sk_user_data pointing to the freed
connection path, potentially leading to a use-after-free when network data
arrives. Should we check if the connection is dying before attaching to it?

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