Re: [PATCH net-next v3 08/13] net/rds: hold connection references in lookup, sockets and c_passive

[netdev-bot+sashiko@kernel.org]

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

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

- [High] rds_send_probe() (net/rds/send.c:1579-1586) links a probe/pong
  message onto cp->cp_send_queue and takes a connection reference…
- [Medium] The commit message claims the new rs_lock coverage in
  rds_ioctl(SIOCRDSSETTOS) closes the window that "let a racing sendmsg…
- [Low] The comment block documenting rds_conn_lookup()'s contract now
  sits above the newly added rds_conn_passive_locked() helper, which has…
- [Low] struct rds_sock in net/rds/rds.h explicitly enumerates what
  rs_lock protects ("rs_lock protects all these adjacent members before…

--

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

This patch makes rds_conn_lookup() and __rds_conn_create() return a
connection with a reference held for the caller, gives the rs->rs_conn
sendmsg cache and parent->c_passive references of their own, and moves the
rs_conn/rs_tos serialization from the global rds_sock_lock to rs_lock.  It
also RCU-annotates c_passive and closes a KCSAN-reported data race between
concurrent rds_sendmsg() calls on the same socket.

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

A few questions below about the new rs_lock discipline, the misplaced
comment block in connection.c, and the interaction between the new
"sender keeps running against a quiesced conn" rule and rds_send_probe().

> diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
> index d5defe9172e3..1cc20b5cfd21 100644
> --- a/net/rds/af_rds.c
> +++ b/net/rds/af_rds.c

[ ... ]

> @@ -267,18 +276,23 @@ static int rds_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
>  		else
>  			return -ENOIOCTLCMD;
>  
> -		spin_lock_bh(&rds_sock_lock);
> +		/* rs_conn is serialized by rs_lock (see rds_sendmsg());
> +		 * hold it across the "no connection yet" check and the
> +		 * rs_tos store so a racing sendmsg cannot cache a conn
> +		 * whose c_tos then disagrees with rs_tos.
> +		 */
> +		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;

[Severity: Medium]

Does this comment (and the matching claim in the commit message, "it used
the unrelated global rds_sock_lock before, which also let a racing sendmsg
cache a connection whose c_tos disagrees with the rs_tos being set") hold
for a sendmsg that is already past its own rs_lock section?

rds_sendmsg() drops rs_lock before it samples rs->rs_tos:

net/rds/send.c:rds_sendmsg() {
	spin_lock_irqsave(&rs->rs_lock, flags);
	conn = rs->rs_conn;
	...
	} else {
		conn = NULL;
	}
	spin_unlock_irqrestore(&rs->rs_lock, flags);

	if (!conn) {
		...
		conn = rds_conn_create_outgoing(sock_net(sock->sk),
						&rs->rs_bound_addr, &daddr,
						rs->rs_transport, rs->rs_tos,
	...
}

and the install re-takes rs_lock without re-checking conn->c_tos against
rs->rs_tos:

		spin_lock_irqsave(&rs->rs_lock, flags);
		old = rs->rs_conn;
		rs->rs_conn = conn;
		spin_unlock_irqrestore(&rs->rs_lock, flags);

So with thread A between the two rs_lock sections (conn == NULL) and
thread B in SIOCRDSSETTOS, B sees rs_tos == 0 && rs_conn == NULL, succeeds
and stores the new tos; A then creates or looks up a connection with the
old tos and caches it.  Can that leave rs->rs_tos == X while
rs->rs_conn->c_tos == 0, and send the in-flight message over a connection
whose service level does not match the socket's configured ToS?

Would snapshotting rs_tos inside the first rs_lock section, or validating
conn->c_tos == rs->rs_tos when installing the cache, close this?

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index c3b3d756c52e..7ef6fb9d352b 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -81,7 +81,18 @@ static struct hlist_head *rds_conn_bucket(const struct in6_addr *laddr,
>  		var |= RDS_INFO_CONNECTION_FLAG_##suffix;	\
>  } while (0)
>  
> -/* rcu read lock must be held or the connection spinlock */
> +/* rcu read lock must be held or the connection spinlock.
> + * On success a reference is taken on the returned connection; the
> + * caller must drop it with rds_conn_put().
> + */
> +/* c_passive is written under rds_conn_lock and read under RCU */
> +static struct rds_connection *
> +rds_conn_passive_locked(struct rds_connection *conn)
> +{
> +	return rcu_dereference_protected(conn->c_passive,
> +					 lockdep_is_held(&rds_conn_lock));
> +}
> +
>  static struct rds_connection *rds_conn_lookup(struct net *net,

[Severity: Low]

Was the new helper meant to be inserted above rds_conn_lookup()'s comment
rather than below it?  As written, the "rcu read lock must be held or the
connection spinlock ... a reference is taken on the returned connection"
text now documents rds_conn_passive_locked(), which has neither property:
it asserts rds_conn_lock is held (RCU alone is not enough) and returns the
pointer without any reference.  __rds_conn_create() has to take the
reference itself right after calling it:

			passive = rds_conn_passive_locked(parent);
			...
				rds_conn_get(passive);

Meanwhile rds_conn_lookup(), whose contract this patch actually changes,
ends up with no comment describing that the caller now owns a reference.

> @@ -98,6 +109,17 @@ static struct rds_connection *rds_conn_lookup(struct net *net,
>  		    conn->c_tos == tos &&
>  		    net == rds_conn_net(conn) &&
>  		    conn->c_dev_if == dev_if) {
> +			/* Only ever hand out a live reference.
> +			 * rds_conn_destroy() unhashes under
> +			 * rds_conn_lock and waits a grace period
> +			 * before dropping the initial reference, so
> +			 * an entry this traversal reaches still holds
> +			 * at least that one; the conditional get
> +			 * documents the contract rather than
> +			 * papering over a zero-refcount entry.
> +			 */
> +			if (!kref_get_unless_zero(&conn->c_refcount))
> +				continue;
>  			ret = conn;
>  			break;
>  		}

[ ... ]

> diff --git a/net/rds/rds.h b/net/rds/rds.h
> index 8a969444e698..4608615e09e9 100644
> --- a/net/rds/rds.h
> +++ b/net/rds/rds.h
> @@ -161,7 +161,7 @@ struct rds_connection {
>  	 * cancellation from landing on a destroyed workqueue.
>  	 */
>  	bool			c_destroy_in_prog;
> -	struct rds_connection	*c_passive;
> +	struct rds_connection __rcu *c_passive;
>  	struct rds_transport	*c_trans;

[Severity: Low]

While touching this header, should the struct rds_sock declarations be
updated too?  rs_conn is now both rs_lock-serialized and reference-owning,
but its comment still only says:

	/*
	 * rds_sendmsg caches the conn it used the last time around.
	 * This helps avoid costly lookups.
	 */
	struct rds_connection	*rs_conn;

and rs_lock's own annotation reads:

	/* rs_lock protects all these adjacent members before the newline */
	spinlock_t		rs_lock;

rs_conn sits before that block and rs_tos sits at the very end of the
struct, so the header currently tells a reader that neither is covered by
rs_lock, which is the opposite of the new rule in rds_ioctl() and
rds_sendmsg().

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 32c411d10e3e..1ae1f24c24e8 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c

[ ... ]

> @@ -1340,11 +1341,29 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
>  	rm->m_daddr = daddr;
>  
>  	/* rds_conn_create has a spinlock that runs with IRQ off.
> -	 * Caching the conn in the socket helps a lot. */
> -	if (rs->rs_conn && ipv6_addr_equal(&rs->rs_conn->c_faddr, &daddr) &&
> -	    rs->rs_tos == rs->rs_conn->c_tos) {
> -		conn = rs->rs_conn;
> +	 * Caching the conn in the socket helps a lot.
> +	 *
> +	 * The cached rs_conn holds a connection reference; take one of
> +	 * our own for the duration of this call (dropped on both exit
> +	 * paths), so that neither a concurrent sender replacing the
> +	 * cache nor rds_conn_destroy() can free the connection under
> +	 * us.  A cached connection whose destruction has begun is not
> +	 * reused: dropping it here lets the next sendmsg look up or
> +	 * create a live one instead of returning -EAGAIN forever.
> +	 */
> +	spin_lock_irqsave(&rs->rs_lock, flags);
> +	conn = rs->rs_conn;
> +	if (conn && ipv6_addr_equal(&conn->c_faddr, &daddr) &&
> +	    rs->rs_tos == conn->c_tos && !rds_destroy_pending(conn)) {
> +		rds_conn_get(conn);
>  	} else {
> +		conn = NULL;
> +	}
> +	spin_unlock_irqrestore(&rs->rs_lock, flags);

[Severity: High]

The commit message says the connection "may still be destroyed while a
send is in flight ... but it is only quiesced; the free is held off by the
sender's reference."  Given that, can a sender that is already holding this
reference strand a probe message on a path that has already been purged?

rds_sendmsg() reaches the ping path before its own rds_destroy_pending()
re-check:

	if (conn->c_trans->t_mp_capable &&
	    !rds_conn_path_up(&conn->c_path[0])) {
		...
		rds_send_ping(conn, 0);
	}

and rds_send_probe() enqueues under cp_lock with no destroy test at all;
only the work queueing afterwards is guarded:

net/rds/send.c:rds_send_probe() {
	spin_lock_irqsave(&cp->cp_lock, flags);
	list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
	set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
	rds_message_addref(rm);
	rm->m_inc.i_conn = cp->cp_conn;
	...
	spin_unlock_irqrestore(&cp->cp_lock, flags);
	...
	rcu_read_lock();
	if (!rds_destroy_pending(cp->cp_conn))
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 1);
	rcu_read_unlock();
}

rds_conn_path_quiesce() drains cp_send_queue exactly once, and
rds_conn_destroy() runs that pass only once (c_destroy_in_prog guards it):

net/rds/connection.c:rds_conn_path_quiesce() {
	...
	/* tear down queued messages */
	list_for_each_entry_safe(rm, rtmp,
				 &cp->cp_send_queue,
				 m_conn_item) {
		list_del_init(&rm->m_conn_item);
		BUG_ON(!list_empty(&rm->m_sock_item));
		rds_message_put(rm);
	}
	...
}

If the cp_lock section in rds_send_probe() runs after that drain, nothing
reaps the message afterwards - rds_send_xmit() refuses destroy-pending
conns and there is no second purge pass.  Later in the series the same
enqueue also takes a connection reference released only from
rds_message_put() ("put in rds_message_put()"), so at that point does the
stranded probe keep c_refcount from reaching zero, leaving
rds_conn_destroy_fini() unreached and rds_conn_wait_conns_freed() waiting
forever at transport unload or netns teardown?

The sibling enqueue rds_send_queue_rm() does get exactly this guard later
in the series, with rds_conn_path_quiesce() splicing under cp_lock to pair
with it:

		spin_lock(&cp->cp_lock);
		if (rds_destroy_pending(conn)) {
			spin_unlock(&cp->cp_lock);
			*queued = -EAGAIN;
			goto unlock;
		}

Should rds_send_probe() get the same test under cp_lock?  Note the probe
enqueue is also reachable from the receive path, since a peer's probe ping
drives rds_recv_incoming() -> rds_send_pong() -> rds_send_probe().

> +
> +	if (!conn) {
> +		struct rds_connection *old;
> +
>  		conn = rds_conn_create_outgoing(sock_net(sock->sk),
>  						&rs->rs_bound_addr, &daddr,
>  						rs->rs_transport, rs->rs_tos,

[ ... ]

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