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