Re: [PATCH net-next v8 11/13] net/rds: hold connection references in lookup, sockets and c_passive

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] Two new comments in this patch don't match what the rs_conn cache
  code does.

Pre-existing issues:
- [Medium] The rs_conn cache-hit check in rds_sendmsg() (net/rds/send.c)
  compares only the destination address and ToS…

--

net/rds: hold connection references in lookup, sockets and c_passive

This patch makes rds_conn_lookup(), __rds_conn_create(), the rs->rs_conn
sendmsg cache and parent->c_passive hold real connection references.
rs_lock now serializes the rs_conn cache, which also resolves a syzbot
KCSAN data race between concurrent rds_sendmsg() calls.

Closes: https://syzkaller.appspot.com/bug?extid=879c1877016972360186

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 7235974343dda..bedcd8b836f66 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c

[ ... ]

> @@ -1343,21 +1344,68 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
>  	rm->m_daddr = daddr;
>  
>  	/* 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) {
> -		conn = rs->rs_conn;
> +	 * Caching the conn in the socket helps a lot.
> +	 *
> +	 * The cached rs_conn holds a connection reference; take one of
> +	 * our own for the duration of this call (dropped on both exit
> +	 * paths), so that neither a concurrent sender replacing the
> +	 * cache nor rds_conn_destroy() can free the connection under
> +	 * us.  A cached connection whose destruction has begun is not
> +	 * reused: dropping it here lets the next sendmsg look up or
> +	 * create a live one instead of returning -EAGAIN forever.
> +	 */
> +	spin_lock_irqsave(&rs->rs_lock, flags);
> +	conn = rs->rs_conn;
> +	if (conn && rds_destroy_pending(conn)) {
> +		/* drop the cache's reference right here, or the socket
> +		 * would pin the quiesced connection until it is closed
> +		 */
> +		rs->rs_conn = NULL;
> +		spin_unlock_irqrestore(&rs->rs_lock, flags);
> +		rds_conn_put(conn);
> +		conn = NULL;

[Severity: Low]
This isn't a bug, but do the new comments match what this path does?

The comment above says that dropping the cached connection "lets the next
sendmsg look up or create a live one". After conn = NULL here, though,
execution goes straight into the if (!conn) block below and calls
rds_conn_create_outgoing() in this same call. A later sendmsg only has to
do the work if that create fails.

The new rs_conn comment in net/rds/rds.h also says:

    The cache owns a connection
    reference, dropped when it is replaced or the socket is
    released, ...

That misses this eviction case, where rds_sendmsg() sets rs_conn to NULL
whenever rds_destroy_pending(conn) is true. If the create that follows
fails, rs_conn stays NULL and nothing replaces it.

Could both comments be updated to cover the eviction path?

>  	} else {
> +		if (conn && ipv6_addr_equal(&conn->c_faddr, &daddr) &&
> +		    rs->rs_tos == conn->c_tos)
> +			rds_conn_get(conn);

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
should the cache-hit check also compare scope_id against conn->c_dev_if?

rds_conn_lookup() tells connections apart with conn->c_dev_if == dev_if.
The TCP connect path in net/rds/tcp_connect.c uses
sin6.sin6_scope_id = conn->c_dev_if. So a cached connection is tied to
one interface.

If the socket is bound to a global IPv6 address, rs_bound_scope_id is 0.
This earlier check in rds_sendmsg() then accepts any nonzero scope_id:

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

A send to fe80::X with sin6_scope_id = A creates and caches a conn with
c_dev_if = A. A later send to fe80::X with sin6_scope_id = B hits the
cache here and reuses the interface A connection.

Would that message go to fe80::X on link A instead of link B? The
baseline predicate used the same key, and this rewrite keeps it. Adding
conn->c_dev_if == scope_id to the check looks like it would fix this.

> +		else
> +			conn = NULL;
> +		spin_unlock_irqrestore(&rs->rs_lock, flags);
> +	}

[ ... ]

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