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

[PATCH net v2] 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.

A request without a scope keeps matching the cached connection.  A
send with a destination always carries one for a link-local peer, but
a send without a destination takes rs_bound_scope_id, and a bind() to
a non-link-local address after connect() to a link-local peer resets
that to 0.  Such a send never named an interface, so the connection
the socket last used is the one it means; making it miss the cache
would send it to look up a connection with no interface, which cannot
be established.

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..393898a079c1 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 &&
+	    (!scope_id || 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 v2] net/rds: include the scope id in the sendmsg connection cache check

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · 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).

- [High] The `!scope_id ||` wildcard added to the rs_conn cache test in
  rds_sendmsg() (net/rds/send.c:1344-1347) leaves the bug this patch…
- [Medium] The new `c_dev_if == scope_id` comparison in rds_sendmsg()
  (net/rds/send.c:1346) also applies to non-link-local destinations,…

Pre-existing issues:
- [Medium] rds_sendmsg() reads rs->rs_conn several times in the cache
  test (send.c:1344-1347: NULL check, c_faddr, c_tos, the new c_dev_if…

--

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

This commit adds a c_dev_if comparison to the rs->rs_conn cache-hit test
in rds_sendmsg(). A send to a link-local address through a different
interface then no longer reuses the cached connection. A request with a
zero scope_id still matches the cached connection whatever its c_dev_if.

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 1afa981e5c06d..393898a079c1d 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 &&
> +	    (!scope_id || rs->rs_conn->c_dev_if == scope_id)) {

[Severity: High]
Can the !scope_id short-circuit still send a connected socket's traffic
out the wrong interface?

The commit message says:

  Such a send never named an interface, so the connection
  the socket last used is the one it means

However, connect() did name one. rds_connect() stores the peer's scope in
the bound scope field:

net/rds/af_rds.c:rds_connect() {
    ...
			rs->rs_bound_scope_id = sin6->sin6_scope_id;
    ...
}

rs_bound_scope_id is a macro for rs_bound_sin6.sin6_scope_id. So a later
bind() to a global address overwrites it with 0 in rds_add_bound():

net/rds/bind.c:rds_add_bound() {
    ...
			rs->rs_bound_scope_id = scope_id;
    ...
}

Consider this sequence on an unbound socket:

  connect(fe80::x%ifB)     rs_bound_scope_id = ifB
  bind(global L)           allowed because the new scope_id is 0,
                           rs_bound_scope_id = 0
  sendto(fe80::x%ifA)      allowed because rs_bound_scope_id is 0,
                           rs_conn = conn(L, fe80::x, dev_if ifA)
  send() with no address   daddr = rs_conn_addr = fe80::x,
                           scope_id = rs_bound_scope_id = 0

The last send() passes the new test through !scope_id and reuses the ifA
connection. rds_tcp_conn_path_connect() and the IB CM code in ib_cm.c
use c_dev_if as the peer's sin6_scope_id.

Wouldn't the datagram the application addressed to fe80::x%ifB via
connect() then go out on ifA, to whichever host answers to fe80::x on
that link? That looks like the misdelivery the commit message describes.

One option is to store the connected peer's scope separately from the
bound scope and use it on the no-destination path. Would that close this
case? It would also avoid the dev_if 0 lookup, so the !scope_id wildcard
would no longer be needed.

[Severity: Medium]
Does the c_dev_if comparison also change behavior for global
destinations, where the user never supplied scope_id?

For a non-link-local daddr, scope_id is inherited from rs_bound_scope_id
earlier in rds_sendmsg():

		if (scope_id != rs->rs_bound_scope_id) {
			if (!scope_id) {
				scope_id = rs->rs_bound_scope_id;

Consider this sequence:

  bind(global L)           rs_bound_scope_id = 0
  sendto(global G)         rs_conn = conn(L, G, dev_if 0)
  connect(fe80::x%ifB)     allowed because the bound scope is 0,
                           rs_bound_scope_id = ifB
  sendto(global G)         scope_id 0 is replaced with ifB

Before this patch, the last sendto() hit the cached conn(L, G, 0). With
the patch it misses, and rds_conn_create_outgoing(..., ifB) creates a
second conn(L, G, dev_if ifB), because rds_conn_lookup() requires an
exact c_dev_if match.

dev_if has no meaning for a global address, so the result is a second
transport connection to the same peer.

Could messages already queued on the dev_if 0 connection be reordered
with later ones on the new connection?

On the peer's passive side, rds_tcp_accept_one() uses sk_bound_dev_if,
which is normally 0. Both incoming TCP connections would therefore map to
the same conn there.

The commit message says "A request without a scope keeps matching the
cached connection" but does not mention this case. Would limiting the
c_dev_if comparison to a link-local daddr avoid it?

>  		conn = rs->rs_conn;

[Severity: Medium]
This isn't a bug introduced by this patch, but rs->rs_conn is loaded
several times in this test: the NULL check, c_faddr, c_tos, now c_dev_if,
and then conn = rs->rs_conn. No socket lock is held here and there is no
READ_ONCE() snapshot, because lock_sock() was released earlier.

A concurrent rds_sendmsg() on the same socket can store a new pointer in
the else branch, also without a lock:

		rs->rs_conn = conn;

If the compiler reloads rs->rs_conn between the checks and the
assignment, could the message be queued on a connection that was never
checked against daddr, tos or scope?

This seems unlikely with current compilers. Taking a single READ_ONCE()
snapshot into a local, with WRITE_ONCE() on the store, would avoid it.
The patch adds one more load to this sequence.

>  	} else {
>  		conn = rds_conn_create_outgoing(sock_net(sock->sk),

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

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

[Allison Henderson]

On Thu, 2026-10-01 at 15:26 +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 1 · 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).
> 
> - [High] The `!scope_id ||` wildcard added to the rs_conn cache test in
>   rds_sendmsg() (net/rds/send.c:1344-1347) leaves the bug this patch…
> - [Medium] The new `c_dev_if == scope_id` comparison in rds_sendmsg()
>   (net/rds/send.c:1346) also applies to non-link-local destinations,…
> 
> Pre-existing issues:
> - [Medium] rds_sendmsg() reads rs->rs_conn several times in the cache
>   test (send.c:1344-1347: NULL check, c_faddr, c_tos, the new c_dev_if…
> 
> --
> 
> net/rds: include the scope id in the sendmsg connection cache check
> 
> This commit adds a c_dev_if comparison to the rs->rs_conn cache-hit test
> in rds_sendmsg(). A send to a link-local address through a different
> interface then no longer reuses the cached connection. A request with a
> zero scope_id still matches the cached connection whatever its c_dev_if.
> 
> > diff --git a/net/rds/send.c b/net/rds/send.c
> > index 1afa981e5c06d..393898a079c1d 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 &&
> > +	    (!scope_id || rs->rs_conn->c_dev_if == scope_id)) {
> 
> [Severity: High]
> Can the !scope_id short-circuit still send a connected socket's traffic
> out the wrong interface?
> 
> The commit message says:
> 
>   Such a send never named an interface, so the connection
>   the socket last used is the one it means
> 
> However, connect() did name one. rds_connect() stores the peer's scope in
> the bound scope field:
> 
> net/rds/af_rds.c:rds_connect() {
>     ...
> 			rs->rs_bound_scope_id = sin6->sin6_scope_id;
>     ...
> }
> 
> rs_bound_scope_id is a macro for rs_bound_sin6.sin6_scope_id. So a later
> bind() to a global address overwrites it with 0 in rds_add_bound():
> 
> net/rds/bind.c:rds_add_bound() {
>     ...
> 			rs->rs_bound_scope_id = scope_id;
>     ...
> }
> 
> Consider this sequence on an unbound socket:
> 
>   connect(fe80::x%ifB)     rs_bound_scope_id = ifB
>   bind(global L)           allowed because the new scope_id is 0,
>                            rs_bound_scope_id = 0
>   sendto(fe80::x%ifA)      allowed because rs_bound_scope_id is 0,
>                            rs_conn = conn(L, fe80::x, dev_if ifA)
>   send() with no address   daddr = rs_conn_addr = fe80::x,
>                            scope_id = rs_bound_scope_id = 0
> 
> The last send() passes the new test through !scope_id and reuses the ifA
> connection. rds_tcp_conn_path_connect() and the IB CM code in ib_cm.c
> use c_dev_if as the peer's sin6_scope_id.
> 
> Wouldn't the datagram the application addressed to fe80::x%ifB via
> connect() then go out on ifA, to whichever host answers to fe80::x on
> that link? That looks like the misdelivery the commit message describes.
> 
Yes, the wildcard traded one misdelivery for another.  I've send a v3
that gives the connected peer its own scope field as suggested.  That should
also take care of the global-destination case in the second point as well.


> One option is to store the connected peer's scope separately from the
> bound scope and use it on the no-destination path. Would that close this
> case? It would also avoid the dev_if 0 lookup, so the !scope_id wildcard
> would no longer be needed.
> 
> [Severity: Medium]
> Does the c_dev_if comparison also change behavior for global
> destinations, where the user never supplied scope_id?
> 
> For a non-link-local daddr, scope_id is inherited from rs_bound_scope_id
> earlier in rds_sendmsg():
> 
> 		if (scope_id != rs->rs_bound_scope_id) {
> 			if (!scope_id) {
> 				scope_id = rs->rs_bound_scope_id;
> 
> Consider this sequence:
> 
>   bind(global L)           rs_bound_scope_id = 0
>   sendto(global G)         rs_conn = conn(L, G, dev_if 0)
>   connect(fe80::x%ifB)     allowed because the bound scope is 0,
>                            rs_bound_scope_id = ifB
>   sendto(global G)         scope_id 0 is replaced with ifB
> 
> Before this patch, the last sendto() hit the cached conn(L, G, 0). With
> the patch it misses, and rds_conn_create_outgoing(..., ifB) creates a
> second conn(L, G, dev_if ifB), because rds_conn_lookup() requires an
> exact c_dev_if match.
> 
> dev_if has no meaning for a global address, so the result is a second
> transport connection to the same peer.
> 
> Could messages already queued on the dev_if 0 connection be reordered
> with later ones on the new connection?
> 
> On the peer's passive side, rds_tcp_accept_one() uses sk_bound_dev_if,
> which is normally 0. Both incoming TCP connections would therefore map to
> the same conn there.
> 
> The commit message says "A request without a scope keeps matching the
> cached connection" but does not mention this case. Would limiting the
> c_dev_if comparison to a link-local daddr avoid it?
> 
> >  		conn = rs->rs_conn;
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but rs->rs_conn is loaded
> several times in this test: the NULL check, c_faddr, c_tos, now c_dev_if,
> and then conn = rs->rs_conn. No socket lock is held here and there is no
> READ_ONCE() snapshot, because lock_sock() was released earlier.

Yes, but that is baseline, and I have a fix for that in the connection-lifetime
series ("net/rds: hold connection references in lookup, sockets and c_passive")
targeted to net-next.  A snapshot here would still leave the pointer
unreferenced across the test, so I'd rather not grow a net fix into a
partial copy of that.

Thanks!
Allison

> 
> A concurrent rds_sendmsg() on the same socket can store a new pointer in
> the else branch, also without a lock:
> 
> 		rs->rs_conn = conn;
> 
> If the compiler reloads rs->rs_conn between the checks and the
> assignment, could the message be queued on a connection that was never
> checked against daddr, tos or scope?
> 
> This seems unlikely with current compilers. Taking a single READ_ONCE()
> snapshot into a local, with WRITE_ONCE() on the store, would avoid it.
> The patch adds one more load to this sequence.
> 
> >  	} else {
> >  		conn = rds_conn_create_outgoing(sock_net(sock->sk),
>