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

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().

[ ... ]

[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?

[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.

[ ... ]

[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().

[ ... ]

[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