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

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

[Allison Henderson]

rds_sendmsg() reuses the connection cached in rs->rs_conn when its
peer address and ToS match the request.  The interface index is part
of a connection's identity as well: rds_conn_create_outgoing() passes
the request's scope_id down as dev_if, and rds_conn_lookup() compares
c_dev_if, so sends to the same link-local address through two
interfaces are two different connections.  The cache-hit test never
looked at it.

A socket bound to a non-link-local address has rs_bound_scope_id 0,
and the scope check at the top of rds_sendmsg() accepts any non-zero
destination scope for such a socket.  So after a send to fe80::x%ifA,
a send to fe80::x%ifB hits the cached ifA connection and the datagram
leaves through ifA, to whichever peer answers to that address there.

Compare c_dev_if with the request's scope_id in the cache test, so
that such a send takes the lookup path and finds, or creates, the ifB
connection.

Fixes: 1e2b44e78eea ("rds: Enable RDS IPv6 support")
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/send.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/rds/send.c b/net/rds/send.c
index 1afa981e5c06..38b9cc9e0d20 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) {
 		conn = rs->rs_conn;
 	} else {
 		conn = rds_conn_create_outgoing(sock_net(sock->sk),
-- 
2.25.1

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

[sashiko-bot]

> rds_sendmsg() reuses the connection cached in rs->rs_conn when its
> peer address and ToS match the request.  The interface index is part
> of a connection's identity as well: rds_conn_create_outgoing() passes
> the request's scope_id down as dev_if, and rds_conn_lookup() compares
> c_dev_if, so sends to the same link-local address through two
> interfaces are two different connections.  The cache-hit test never
> looked at it.
> 
> A socket bound to a non-link-local address has rs_bound_scope_id 0,
> and the scope check at the top of rds_sendmsg() accepts any non-zero
> destination scope for such a socket.  So after a send to fe80::x%ifA,
> a send to fe80::x%ifB hits the cached ifA connection and the datagram
> leaves through ifA, to whichever peer answers to that address there.
> 
> Compare c_dev_if with the request's scope_id in the cache test, so
> that such a send takes the lookup path and finds, or creates, the ifB
> connection.
> 
> Fixes: 1e2b44e78eea ("rds: Enable RDS IPv6 support")
> Assisted-by: Claude-Code:claude-fable-5
> Signed-off-by: Allison Henderson <achender@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

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

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

[netdev-bot+sashiko]

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

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

[Allison Henderson]

On Fri, 2026-09-25 at 00:53 +0000, netdev-bot+sashiko@kernel.org wrote:
> 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?
Yes, the analysis is right, I've sent a V2 that keeps the cached connection when
the request names no interface and only checks c_dev_if when it does.
> 
> 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.

Yes, but I'm handling that in the larger connection lifetime series 
("net/rds: make connection lifetime reference-counted").  It moves
rs_conn cache under rs_lock and gives the cache a real reference.  If 
we did the READ_ONCE() here would still leave the pointer unreferenced
across the test, so I'd rather not let this patch feature creep into
something the ref counting series is already handling.

Thanks!
Allison

pw-bot: cr
> 
> The validation loads and thpw-bot: crpw-bot: cr
> e 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?
>