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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] rds: getname() and recvmsg() break IPv6 link-local scope ID
- [High] rds: destination-less sendmsg() fails for global dests if bound link-local
--

commit 898dc27ebfe8db635a3eb7a78c44763fbfcf0c43
Author: Allison Henderson <achender@kernel.org>

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

This commit isolates the connected peer's scope ID into a new
rs_conn_scope_id field, leaving the bound scope ID in
rs_bound_scope_id alone. It updates rds_connect(), rds_bind(), and
rds_sendmsg() to use the new field to ensure destination-less sends
and link-local binds operate with the correct interface scopes.

> 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: High]
Does this change miss updating rds_getname() and rds_recvmsg()?

By no longer storing the peer's scope ID in rs->rs_bound_scope_id,
userspace programs calling getpeername() or receiving messages via recvmsg()
may receive a scope ID of 0 for connected link-local peers when the
local socket is unbound or globally bound.

For example, rds_getname() appears to still populate the address using
the bound scope:

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

Similarly in rds_recvmsg():

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

Could this prevent userspace from properly parsing the address or
sending replies to the correct interface?

> +		} else {
> +			rs->rs_conn_scope_id = 0;
>  		}
>  		rs->rs_conn_addr = sin6->sin6_addr;
>  		rs->rs_conn_port = sin6->sin6_port;
[ ... ]
> 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;
>  		release_sock(sk);
>  	}
>  

[Severity: High]
Does this cause destination-less sendmsg() calls to bypass the bound
scope fallback for global connections?

If an RDS socket is bound to an IPv6 link-local address (with a valid
scope) and connected to a global IPv6 address, a destination-less
sendmsg() will have namelen == 0.

It looks like it will assign scope_id from rs_conn_scope_id (which is
0 for global peers), but then skip the fallback logic entirely:

net/rds/send.c:rds_sendmsg() {
    ...
    } else {
        /* We only care about consistency with ->connect() */
        ...
        scope_id = rs->rs_conn_scope_id;
    }

    lock_sock(sk);
    if (ipv6_addr_any(&rs->rs_bound_addr) || ipv6_addr_any(&daddr)) {
        ...
    } else if (namelen != 0) {
        ...
        if (scope_id != rs->rs_bound_scope_id) {
            if (!scope_id) {
                scope_id = rs->rs_bound_scope_id;
    ...
}

Because the fallback logic is inside the else if (namelen != 0) block,
scope_id remains 0. The connection could then be created with a dev_if
of 0, causing the TCP transport to attempt a kernel_bind() to a
link-local source address with no scope ID, which the IPv6 stack will
reject.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003163526.250805-1-achender@kernel.org?part=1