Re: [PATCH net] net/rds: include the scope id in the sendmsg connection cache check

[netdev-bot+sashiko@kernel.org]

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

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

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] New regression for connected (destination-less) sends: the
  added `rs->rs_conn->c_dev_if == scope_id` test in rds_sendmsg()…

Pre-existing issues:
- [Medium] rds_sendmsg() validates the cached connection with plain,
  unsynchronized loads of rs->rs_conn (`rs->rs_conn &&`, `->c_faddr`,…

--

net/rds: include the scope id in the sendmsg connection cache check

The patch adds the interface index to the cached-connection test in
rds_sendmsg(), so that a send to a link-local peer through a different
interface no longer reuses the connection created for the first
interface.  Two questions below about the destination-less send case and
about the locking around the cache test.

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 1afa981e5c06d..38b9cc9e0d20b 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c
> @@ -1342,7 +1342,8 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
>  	/* 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) {
> +	    rs->rs_tos == rs->rs_conn->c_tos &&
> +	    rs->rs_conn->c_dev_if == scope_id) {

[Severity: Medium]
Should the new comparison treat scope_id == 0 as "unspecified" rather
than as a mismatch?

For a send with msg_namelen == 0, rds_sendmsg() takes the scope from the
bound scope id:

net/rds/send.c:rds_sendmsg() {
	} else {
		/* We only care about consistency with ->connect() */
		lock_sock(sk);
		daddr = rs->rs_conn_addr;
		dport = rs->rs_conn_port;
		scope_id = rs->rs_bound_scope_id;
		release_sock(sk);
	}

and the namelen != 0 repair branch that would fill in a zero scope from
rs_bound_scope_id is skipped in that case.

rs_bound_scope_id can be 0 while rs_conn_addr is a link-local peer.
rds_connect() records the peer scope there for an unbound socket:

net/rds/af_rds.c:rds_connect() {
		if (addr_type & IPV6_ADDR_LINKLOCAL) {
			...
			rs->rs_bound_scope_id = sin6->sin6_scope_id;
		}
		rs->rs_conn_addr = sin6->sin6_addr;

and a later bind() to a non-link-local address overwrites it
unconditionally:

net/rds/bind.c:rds_add_bound() {
			*port = rs->rs_bound_port;
			rs->rs_bound_scope_id = scope_id;	/* 0 here */

With the sequence connect(fe80::x%ifA) -> bind(global address) ->
sendto(fe80::x%ifA) -> send() with no destination, the last send now
computes scope_id == 0, misses the cached ifA connection, and goes down
the else branch:

>  		conn = rs->rs_conn;
>  	} else {
>  		conn = rds_conn_create_outgoing(sock_net(sock->sk),
> 						&rs->rs_bound_addr, &daddr,
> 						rs->rs_transport, rs->rs_tos,
> 						sock->sk->sk_allocation,
> 						scope_id);

Since rds_conn_lookup() compares c_dev_if, this creates a new connection
with c_dev_if == 0 for a link-local destination.  Can that connection
ever come up?  rds_tcp_conn_path_connect() passes it on as the scope:

net/rds/tcp_connect.c:rds_tcp_conn_path_connect() {
		sin6.sin6_scope_id = conn->c_dev_if;

and tcp_v6_connect() rejects a link-local destination with no scope and
no sk_bound_dev_if:

net/ipv6/tcp_ipv6.c:tcp_v6_connect() {
		/* Connect to link-local address requires an interface */
		if (!sk->sk_bound_dev_if)
			return -EINVAL;

So the message looks like it gets queued on a connection that can never
be established, where before the patch it went out over ifA, and
rs->rs_conn is left pointing at the new connection.  Would skipping the
comparison when scope_id == 0, or keeping the connected peer's scope
separately from rs_bound_scope_id, avoid this?

[Severity: Medium]
This isn't a bug introduced by this patch, but the cache test reads
rs->rs_conn several times without any synchronization, and the new
c_dev_if comparison is one more such read.

The validation loads and the load used for the assignment are separate:

net/rds/send.c:rds_sendmsg() {
	if (rs->rs_conn && ipv6_addr_equal(&rs->rs_conn->c_faddr, &daddr) &&
	    rs->rs_tos == rs->rs_conn->c_tos &&
	    rs->rs_conn->c_dev_if == scope_id) {
		conn = rs->rs_conn;

The socket lock covering the earlier scope-id checks has already been
dropped by the release_sock(sk) before rds_rdma_bytes(), and
rds_sock_lock, which the RDS_TOS ioctl path takes when it inspects
rs->rs_conn, is not held here either.  The publishing store in the else
branch is equally unsynchronized:

		rs->rs_conn = conn;

For two concurrent sendmsg() calls on the same AF_RDS socket (threads or
a shared fd), can one thread validate the connection for fe80::x%ifA and
then reload rs->rs_conn and pick up the fe80::x%ifB connection the other
thread just stored, so that rm->m_conn_path ends up on the unvalidated
connection?  That would be the same misrouting the changelog describes.

Would it be worth folding in a single snapshot of the pointer, for
example conn = READ_ONCE(rs->rs_conn); if (conn && ...), with a matching
WRITE_ONCE() on the store, or holding the socket lock across the cache
test?

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