Re: [PATCH net v3 2/2] 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 · 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] In rds_sendmsg() (net/rds/send.c), send() and sendto() to the
  same peer now build different connection keys.
- [Medium] In rds_sendmsg() (net/rds/send.c:1344-1347), the cache test
  reads rs->rs_conn with plain loads and no lock: the NULL check, the…

--

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

This patch adds a comparison of rs->rs_conn->c_dev_if against the
request's scope_id to the connection cache test in rds_sendmsg(). The aim
is that sends to the same link-local address through different interfaces
go through the lookup path and use the matching connection, not the cached
one.

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 9380b67675bdc..26a937a9df6af 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: High]
On a socket bound to a link-local address and connected to a
non-link-local peer, can send() and sendto() to the same peer now end up
with different connection keys?

Take bind(fe80::a%X) followed by connect(G), where G is a global address.
rds_bind() stores rs_bound_scope_id = X, and rds_connect() takes the
non-link-local branch:

net/rds/af_rds.c:rds_connect() {
    ...
		} else {
			rs->rs_conn_scope_id = 0;
		}
    ...
}

A sendto(G) starts with scope_id 0. The namelen != 0 block in
rds_sendmsg() then falls back to the bound scope:

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

So sendto(G) creates and caches a connection with c_dev_if = X.

A send() with no destination takes scope_id = rs->rs_conn_scope_id, which
is 0. It never gets that fallback, because the fallback sits inside the
else if (namelen != 0) branch.

With the new c_dev_if == scope_id test, X != 0, so the cache misses.
rds_conn_create_outgoing(..., 0) then returns a connection with a
link-local c_laddr and c_dev_if = 0, and c_bound_if = 0 from
__rds_conn_create(). rs->rs_conn is overwritten with that connection.

For TCP, rds_tcp_conn_path_connect() binds the kernel socket with:

		sin6.sin6_addr = conn->c_laddr;
		...
		sin6.sin6_scope_id = conn->c_dev_if;

That socket has no sk_bound_dev_if, so __inet6_bind() rejects the
link-local bind:

			/* Binding to link-local address requires an interface */
			if (!sk->sk_bound_dev_if) {
				err = -EINVAL;

Wouldn't that connection never come up, so that data queued by send()
stalls? An application that mixes sendto(G) and send() would also switch
rs_conn between the two connections on every call.

Before this series, send() used rs_bound_scope_id, which matches what
sendto() computes. The earlier patch "net/rds: keep the connected peer's
scope id apart from the bound one" switched send() to rs_conn_scope_id.
This patch then removes the faddr/tos-only cache hit that was hiding the
mismatch.

The commit message says:

  With the previous patch a send without a destination carries the
  connected peer's scope, so the comparison is exact for it too.

Does that hold when the bound address is link-local and the peer is not?

Would it help to apply the rs_bound_scope_id fallback to the namelen == 0
path too, or to store the effective scope in rds_connect(), so both paths
use the same key?

>  		conn = rs->rs_conn;

[Severity: Medium]
Can rs->rs_conn change between the checks above and this assignment?

By this point lock_sock() has been released, and every access to
rs->rs_conn in this test is a plain load. Other senders on the same
socket store to it without a lock in the else branch:

		rs->rs_conn = conn;

That seems to allow a sequence like this:

T1: sendto(fe80::x%ifB) passes the checks against the cached connB
T2: sendto(fe80::x%ifA) misses the cache and stores rs->rs_conn = connA
T1: reloads rs->rs_conn and queues the ifB message on connA

The result would be the same wrong-interface send this patch is trying to
close, reached through a race. It requires the compiler to reload
rs->rs_conn, which C allows for plain accesses.

The unsnapshotted read was already there before this patch. With this
change, though, a socket that alternates interfaces rewrites rs_conn on
every send.

Would reading rs->rs_conn once with READ_ONCE(), running all the
comparisons on that snapshot, and publishing with WRITE_ONCE() close this?

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

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