Re: [PATCH net-next v6 08/12] 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 3 potential
issue(s) to consider.

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message's central justification for the new
  rs->rs_conn reference is factually wrong about reachability.
- [Medium] Concern as filed: rds_sendmsg()'s new connection reference can
  be the final reference at either exit put (net/rds/send.c:1516 success…
- [Low] The comment newly added to rds_tcp_accept_one()
  (net/rds/tcp_listen.c, around line 281) misattributes behaviour: it…

Pre-existing issues:
- [Medium] Concern as filed: this patch makes it an explicitly supported
  state for rds_sendmsg() to keep running on a connection whose…

--

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

This patch hands out real references wherever a struct rds_connection
pointer previously escaped bare: rds_conn_lookup() and __rds_conn_create()
return a referenced connection, the rs->rs_conn sendmsg cache owns a
reference and is read/written under rs_lock, and parent->c_passive owns an
RCU-annotated reference.  Serializing the rs_conn cache also resolves a
syzbot-reported KCSAN data race between concurrent rds_sendmsg() calls.

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

A few questions on the commit message and on the new reference handling.

[Severity: Medium]

The commit message says:

  The connection may still be destroyed while a send is in flight -
  when its device is removed or its netns is torn down - but it is
  only quiesced; the free is held off by the sender's reference.

and later:

  the race it closes needs a connection destroyed under a live
  socket, which takes netns teardown, module unload or device removal.

Are either of the two named triggers reachable at this commit?

For device removal, rds_ib_remove_one() only calls rds_ib_dev_shutdown(),
which drops paths rather than destroying connections:

net/rds/ib.c:rds_ib_dev_shutdown() {
	...
	list_for_each_entry(ic, &rds_ibdev->conn_list, ib_node)
		rds_conn_path_drop(&ic->conn->c_path[0], true);
}

The only IB caller of rds_conn_destroy() is rds_ib_destroy_nodev_conns()
from rds_ib_exit(), i.e. module unload.

For netns teardown, rds_tcp_kill_sock() and rds_loop_kill_conns() run from
pernet ->exit hooks, which only run once the netns refcount reaches zero,
and a userspace RDS socket holds a netns reference from sk_alloc().
rds_sendmsg() also only ever caches a conn whose c_net matches the
socket's netns, given rds_conn_create_outgoing(sock_net(sock->sk), ...)
and the net == rds_conn_net(conn) test in rds_conn_lookup().

Module unload is blocked for as long as a bound socket exists:

net/rds/transport.c:rds_trans_get_preferred() {
	if (trans && (trans->laddr_check(net, addr, scope_id) == 0) &&
	    (!trans->t_owner || try_module_get(trans->t_owner))) {

and that module reference is released only by rds_trans_put() in
rds_release().

Would it be more accurate to describe what is fixed here as the KCSAN data
race on the plain rs->rs_conn stores plus the -EAGAIN-forever behaviour
against a quiesced cached conn, rather than a free under a sender?

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 32c411d10e3ef..a83d4eca0c77c 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c

[ ... ]

> @@ -1340,21 +1341,59 @@ 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: Medium]

This is a pre-existing issue and not introduced by this patch, but the
patch does make it an explicitly supported state for rds_sendmsg() to keep
running on a connection whose rds_conn_destroy() is in progress ("it is
only quiesced; the free is held off by the sender's reference").

Is the quiesce synchronized against a sender that is already past this
rds_destroy_pending() test?

rds_conn_path_quiesce() walks and unlinks cp_send_queue with no cp_lock
held, and BUG_ON()s on any message a socket still has linked:

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));

Meanwhile rds_sendmsg() tests rds_destroy_pending(conn) once and can then
block for an unbounded time in rds_cong_wait() and in the
wait_event_interruptible_timeout() retry loop before rds_send_queue_rm()
inserts the message under cp_lock, with no re-test at insertion.

The unlocked traversal is addressed later in the same series by "net/rds:
take cp_lock to purge cp_send_queue in the quiesce", which splices the
queue under cp_lock.  The remaining window - a message landing on
cp_send_queue after the purge - still needs a destroy running under a live
sender, which does not look reachable at this commit for the reasons above.

[ ... ]

> @@ -1474,6 +1513,8 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
>  		kfree(vct.vec[ind].iov);
>  	kfree(vct.vec);
>  
> +	rds_conn_put(conn);
> +
>  	return payload_len;
>  
>  out:
> @@ -1481,6 +1522,9 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
>  		kfree(vct.vec[ind].iov);
>  	kfree(vct.vec);
>  
> +	if (conn)
> +		rds_conn_put(conn);
> +

[Severity: Medium]

Can either of these puts (or the rds_conn_put(old) in the install path
above) be the final reference while a message that this same call queued is
still linked on cp->cp_send_queue?

At this commit rds_send_queue_rm() stores the connection and path pointers
into the message but takes only a message reference:

net/rds/send.c:rds_send_queue_rm() {
	...
		rds_message_populate_header(&rm->m_inc.i_hdr, sport, dport, 0);
		rm->m_inc.i_conn = conn;
		rm->m_inc.i_conn_path = cp;
		rds_message_addref(rm);

and the socket side later dereferences both:

net/rds/send.c:rds_send_drop_to() {
		conn = rm->m_inc.i_conn;
		if (conn->c_trans->t_mp_capable)
			cp = rm->m_inc.i_conn_path;
		else
			cp = &conn->c_path[0];

		spin_lock_irqsave(&cp->cp_lock, flags);

after rds_conn_destroy_fini() has done kfree(conn->c_path) and
kmem_cache_free(rds_conn_slab, conn).

A later patch in this series, "net/rds: hold a connection reference from
struct rds_incoming" (fe1de9d527be), adds rds_conn_get(conn) in
rds_send_queue_rm() immediately before the i_conn store, released from
rds_message_put(), which closes this gap.  Hitting it at this commit also
requires rds_conn_destroy() to run while a socket still has a message
queued on that connection, which does not look reachable here.

Would it make sense to order the rds_incoming reference patch ahead of this
one, or to note the dependency in the commit message?

> diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
> index 8a0c54aced5ea..7fea5501d7567 100644
> --- a/net/rds/tcp_listen.c
> +++ b/net/rds/tcp_listen.c

[ ... ]

> @@ -277,6 +278,12 @@ 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);
> +	/* A connection whose destroy has begun cannot be found here:
> +	 * TCP connections are only destroyed on netns teardown and on
> +	 * module unload, and both run rds_tcp_listen_stop() - which
> +	 * flushes this work and clears the listen socket that the top
> +	 * of this function tests - before any connection is destroyed.
> +	 */

[Severity: Low]

The comment credits rds_tcp_listen_stop() with clearing the listen socket,
but that helper is handed the socket by value and never touches
rtn->rds_tcp_listen_sock:

net/rds/tcp_listen.c:rds_tcp_listen_stop() {
	...
	flush_workqueue(rds_wq);
	flush_work(acceptor);
	sock_release(sock);
}

The pointer is cleared by the caller, just before it calls the helper:

net/rds/tcp.c:rds_tcp_kill_sock() {
	...
	rtn->rds_tcp_listen_sock = NULL;
	rds_tcp_listen_stop(lsock, &rtn->rds_tcp_accept_w);

The clear-then-flush-then-destroy ordering the safety argument relies on
does hold, but it comes from rds_tcp_kill_sock().  Could the comment name
that function instead?

> +	if (conn_state != RDS_CONN_CONNECTING && conn_state != RDS_CONN_ERROR) {
>  		rds_conn_path_drop(cp, 0);
>  		goto rst_nsk;

[ ... ]

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