Re: [PATCH net v3 1/2] net/rds: keep the connected peer's scope id apart from the bound one

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 2 · 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] Regression in rds_sendmsg() (net/rds/send.c:1259) for
  destination-less sends.
- [Medium] The patch moves the connected peer's scope into the new
  rs_conn_scope_id but does not update rds_getname()'s peer branch…
- [Medium] rds_recvmsg() (net/rds/recv.c:788-793) builds the IPv6 sender
  name from inc->i_saddr and rs->rs_bound_scope_id, i.e. the socket's…

--

net/rds: keep the connected peer's scope id apart from the bound one

This patch adds a separate rs_conn_scope_id field to struct rds_sock.
With it, rds_connect() no longer stores a link-local peer's scope in
rs_bound_scope_id. The connected-socket check in rds_bind() and the
destination-less path in rds_sendmsg() now use the new field.

> diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
> index d5defe9172e36..ba726782addf7 100644
> --- a/net/rds/af_rds.c
> +++ b/net/rds/af_rds.c

[ ... ]

> @@ -616,11 +617,14 @@ static int rds_connect(struct socket *sock, struct sockaddr_unsized *uaddr,
>  				ret = -EINVAL;
>  				break;
>  			}
> -			/* Remember the connected address scope ID.  It will
> -			 * be checked against the binding local address when
> -			 * the socket is bound.
> +			/* Remember the connected address scope ID.  It is
> +			 * checked against the binding local address when
> +			 * the socket is bound, and gives a send without a
> +			 * destination its scope.
>  			 */
> -			rs->rs_bound_scope_id = sin6->sin6_scope_id;
> +			rs->rs_conn_scope_id = sin6->sin6_scope_id;

[Severity: Medium]
The peer scope now lives only in rs_conn_scope_id. Does rds_getname()
still give getpeername() the right scope? Its peer branch still pairs
the connected address with the bound scope:

net/rds/af_rds.c:rds_getname() {
    ...
			sin6->sin6_addr = rs->rs_conn_addr;
			sin6->sin6_flowinfo = 0;
			/* scope_id is the same as in the bound address. */
			sin6->sin6_scope_id = rs->rs_bound_scope_id;
    ...
}

Before this patch, connect(fe80::x%ifA) on an unbound socket reported
fe80::x%ifA. With this patch it reports scope 0. bind(global) followed
by connect(fe80::x%ifB) also goes from ifB to 0. bind(fe80::a%ifA)
followed by connect(2001::1) reports ifA for a global peer.

rds_connect() and rds_sendmsg() both reject a link-local sockaddr_in6
with sin6_scope_id == 0. So the address getpeername() returns for a
link-local peer can't be passed back to connect() or sendto().

Should this read rs->rs_conn_scope_id instead? That would match the new
field's comment in rds.h ("scope of rs_conn_addr when it is link-local,
0 otherwise"). The later patch in this series doesn't change
rds_getname() either.

[Severity: Medium]
There is a similar question for rds_recvmsg(). It takes the sender's
scope from the socket's bound scope:

net/rds/recv.c:rds_recvmsg() {
    ...
				sin6->sin6_addr = inc->i_saddr;
				sin6->sin6_flowinfo = 0;
				sin6->sin6_scope_id = rs->rs_bound_scope_id;
    ...
}

Take bind(global) followed by connect(fe80::x%ifB). Before this patch,
rds_connect() left ifB in rs_bound_scope_id, so recvmsg() from the
connected peer reported fe80::x%ifB. Now rs_bound_scope_id stays 0.

The connection's laddr is global, so c_bound_if is 0. The message is
still delivered to the socket via:

rds_recv_incoming()->rds_find_bound(daddr, dport, conn->c_bound_if)

Does recvmsg() now report fe80::x with scope 0? rds_sendmsg() would
reject that address if it is used to reply. The same applies to the
connections that connect(fe80::x%ifA) followed by bind(global) can now
set up.

Using the bound scope as the sender's scope is older behavior. It also
affects unconnected global-bound sockets. The change in the connected
case comes from this patch, and the commit message doesn't mention it.

[ ... ]

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 1afa981e5c06d..9380b67675bdc 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c
> @@ -1256,7 +1256,7 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
>  		lock_sock(sk);
>  		daddr = rs->rs_conn_addr;
>  		dport = rs->rs_conn_port;
> -		scope_id = rs->rs_bound_scope_id;
> +		scope_id = rs->rs_conn_scope_id;

[Severity: High]
Can this break destination-less sends on a socket that is bound to a
link-local address and connected to a global peer? For example,
bind(fe80::y%ifA) and connect(2001:db8::1), in either order.

rds_connect() now sets rs_conn_scope_id to 0 for the global peer. So a
send() with msg_namelen == 0 gets scope_id 0, where it used to get ifA
from rs_bound_scope_id.

Later in rds_sendmsg(), the fallback to the bound scope only runs when
namelen != 0:

	} else if (namelen != 0) {
		...
		if (scope_id != rs->rs_bound_scope_id) {
			if (!scope_id) {
				scope_id = rs->rs_bound_scope_id;
		...

So rds_conn_create_outgoing() is called with laddr fe80::y and dev_if
0, which sets both c_dev_if and c_bound_if to 0.
rds_tcp_conn_path_connect() then binds with:

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

__inet6_bind() rejects a link-local bind without an interface with
-EINVAL.

Wouldn't every reconnect attempt fail, leaving the connected socket's
data queued on a connection that never comes up?

sendto(2001:db8::1) on the same socket still works because it inherits
rs_bound_scope_id. That means the two send paths now pick different
connections for the same peer.

The later patch "net/rds: include the scope id in the sendmsg
connection cache check" only changes the rs_conn cache check. It
doesn't seem to address this.

Would something like this work instead?

		scope_id = rs->rs_conn_scope_id ?: rs->rs_bound_scope_id;

>  		release_sock(sk);
>  	}
>

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