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

[Allison Henderson <achender@kernel.org>]

On Fri, 2026-09-25 at 00:53 +0000, netdev-bot+sashiko@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
> 
> Critical: 0 · High: 0 · 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).
> 
> - [Medium] New regression for connected (destination-less) sends: the
>   added `rs->rs_conn->c_dev_if == scope_id` test in rds_sendmsg()…
> 
> Pre-existing issues:
> - [Medium] rds_sendmsg() validates the cached connection with plain,
>   unsynchronized loads of rs->rs_conn (`rs->rs_conn &&`, `->c_faddr`,…
> 
> --
> 
> net/rds: include the scope id in the sendmsg connection cache check
> 
> The patch adds the interface index to the cached-connection test in
> rds_sendmsg(), so that a send to a link-local peer through a different
> interface no longer reuses the connection created for the first
> interface.  Two questions below about the destination-less send case and
> about the locking around the cache test.
> 
> > diff --git a/net/rds/send.c b/net/rds/send.c
> > index 1afa981e5c06d..38b9cc9e0d20b 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: Medium]
> Should the new comparison treat scope_id == 0 as "unspecified" rather
> than as a mismatch?
Yes, the analysis is right, I've sent a V2 that keeps the cached connection when
the request names no interface and only checks c_dev_if when it does.
> 
> For a send with msg_namelen == 0, rds_sendmsg() takes the scope from the
> bound scope id:
> 
> net/rds/send.c:rds_sendmsg() {
> 	} else {
> 		/* We only care about consistency with ->connect() */
> 		lock_sock(sk);
> 		daddr = rs->rs_conn_addr;
> 		dport = rs->rs_conn_port;
> 		scope_id = rs->rs_bound_scope_id;
> 		release_sock(sk);
> 	}
> 
> and the namelen != 0 repair branch that would fill in a zero scope from
> rs_bound_scope_id is skipped in that case.
> 
> rs_bound_scope_id can be 0 while rs_conn_addr is a link-local peer.
> rds_connect() records the peer scope there for an unbound socket:
> 
> net/rds/af_rds.c:rds_connect() {
> 		if (addr_type & IPV6_ADDR_LINKLOCAL) {
> 			...
> 			rs->rs_bound_scope_id = sin6->sin6_scope_id;
> 		}
> 		rs->rs_conn_addr = sin6->sin6_addr;
> 
> and a later bind() to a non-link-local address overwrites it
> unconditionally:
> 
> net/rds/bind.c:rds_add_bound() {
> 			*port = rs->rs_bound_port;
> 			rs->rs_bound_scope_id = scope_id;	/* 0 here */
> 
> With the sequence connect(fe80::x%ifA) -> bind(global address) ->
> sendto(fe80::x%ifA) -> send() with no destination, the last send now
> computes scope_id == 0, misses the cached ifA connection, and goes down
> the else branch:
> 
> >  		conn = rs->rs_conn;
> >  	} else {
> >  		conn = rds_conn_create_outgoing(sock_net(sock->sk),
> > 						&rs->rs_bound_addr, &daddr,
> > 						rs->rs_transport, rs->rs_tos,
> > 						sock->sk->sk_allocation,
> > 						scope_id);
> 
> Since rds_conn_lookup() compares c_dev_if, this creates a new connection
> with c_dev_if == 0 for a link-local destination.  Can that connection
> ever come up?  rds_tcp_conn_path_connect() passes it on as the scope:
> 
> net/rds/tcp_connect.c:rds_tcp_conn_path_connect() {
> 		sin6.sin6_scope_id = conn->c_dev_if;
> 
> and tcp_v6_connect() rejects a link-local destination with no scope and
> no sk_bound_dev_if:
> 
> net/ipv6/tcp_ipv6.c:tcp_v6_connect() {
> 		/* Connect to link-local address requires an interface */
> 		if (!sk->sk_bound_dev_if)
> 			return -EINVAL;
> 
> So the message looks like it gets queued on a connection that can never
> be established, where before the patch it went out over ifA, and
> rs->rs_conn is left pointing at the new connection.  Would skipping the
> comparison when scope_id == 0, or keeping the connected peer's scope
> separately from rs_bound_scope_id, avoid this?
> 
> [Severity: Medium]
> This isn't a bug introduced by this patch, but the cache test reads
> rs->rs_conn several times without any synchronization, and the new
> c_dev_if comparison is one more such read.

Yes, but I'm handling that in the larger connection lifetime series 
("net/rds: make connection lifetime reference-counted").  It moves
rs_conn cache under rs_lock and gives the cache a real reference.  If 
we did the READ_ONCE() here would still leave the pointer unreferenced
across the test, so I'd rather not let this patch feature creep into
something the ref counting series is already handling.

Thanks!
Allison

pw-bot: cr
> 
> The validation loads and thpw-bot: crpw-bot: cr
> e load used for the assignment are separate:
> 
> net/rds/send.c:rds_sendmsg() {
> 	if (rs->rs_conn && ipv6_addr_equal(&rs->rs_conn->c_faddr, &daddr) &&
> 	    rs->rs_tos == rs->rs_conn->c_tos &&
> 	    rs->rs_conn->c_dev_if == scope_id) {
> 		conn = rs->rs_conn;
> 
> The socket lock covering the earlier scope-id checks has already been
> dropped by the release_sock(sk) before rds_rdma_bytes(), and
> rds_sock_lock, which the RDS_TOS ioctl path takes when it inspects
> rs->rs_conn, is not held here either.  The publishing store in the else
> branch is equally unsynchronized:
> 
> 		rs->rs_conn = conn;
> 
> For two concurrent sendmsg() calls on the same AF_RDS socket (threads or
> a shared fd), can one thread validate the connection for fe80::x%ifA and
> then reload rs->rs_conn and pick up the fe80::x%ifB connection the other
> thread just stored, so that rm->m_conn_path ends up on the unvalidated
> connection?  That would be the same misrouting the changelog describes.
> 
> Would it be worth folding in a single snapshot of the pointer, for
> example conn = READ_ONCE(rs->rs_conn); if (conn && ...), with a matching
> WRITE_ONCE() on the store, or holding the socket lock across the cache
> test?
>