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

[Allison Henderson <achender@kernel.org>]

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