Re: [PATCH net-next v4 08/12] 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 6 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 (1) · Low: 5 (1)

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] The commit message asserts that the race it closes 'needs a
  connection destroyed under a live socket, which takes netns teardown,…
- [Low] The commit closes a real, syzbot-reported user-triggerable KCSAN
  data race in rds_sendmsg() (concurrent plain stores to rs->rs_conn)…
- [Low] net/rds/rds.h documents c_destroy_in_prog as 'Set once, by
  rds_conn_destroy(), before it cancels the path works; read through…
- [Low] The new struct-field comment for rs_conn in net/rds/rds.h states
  unconditionally that the cached connection pointer 'is read and…
- [Low] In rds_conn_destroy() the comment above the two new puts claims
  'neither can be the last, since the initial reference is dropped…
- [Low] The patch makes rs->rs_lock the serializing lock for rs_conn and
  moves the SIOCRDSSETTOS store of rs_tos under it, but rds_sendmsg()…

Pre-existing issues:
- [Medium] DISCARDED as pre-existing and Medium: the rds_sendmsg()
  connection-cache hit test compares only c_faddr, ToS and destroy…
- [Low] DISCARDED (resolved later in this series):
  rds_ib_cm_handle_connect() publishes the connection through…

--

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

This patch makes every escaping struct rds_connection pointer carry a real
reference: rds_conn_lookup() and __rds_conn_create() return a referenced
connection, the rs->rs_conn sendmsg cache owns one and is now serialized
under rs_lock, and parent->c_passive owns one and becomes RCU annotated.
It also closes a syzbot-reported KCSAN write/write race on rs->rs_conn
between concurrent rds_sendmsg() calls on the same socket.

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

A few questions on the commit message first, then some inline.

>     Serializing the rs_conn cache under rs_lock also resolves a
>     syzbot-reported KCSAN data race between concurrent rds_sendmsg()
>     calls on the same socket, each installing the connection it created
>     into rs->rs_conn with a plain store:

[Severity: Low]
The trailer block carries Reported-by: and Closes: but no Fixes: tag for
the commit that introduced the unsynchronized rs_conn store
(5c11559046c4 "RDS: send.c").

Since the race is reachable by any unprivileged process with two threads
sending on one PF_RDS socket, should a Fixes: tag be added, together with
the existing note that this is not a standalone stable candidate?

>     This is not a stable candidate on its own: it depends on the
>     connection reference counting introduced by the preceding patches,
>     and the race it closes needs a connection destroyed under a live
>     socket, which takes netns teardown, module unload or device removal.

[Severity: Medium]
Is the list of destruction triggers complete here?  rds_ib_cm_connect_complete()
in net/rds/ib_cm.c destroys a connection based on the peer's advertised
protocol version:

	if (conn->c_version < RDS_PROTOCOL_VERSION) {
		if (conn->c_version != RDS_PROTOCOL_COMPAT_VERSION) {
			pr_notice("RDS/IB: Connection <%pI6c,%pI6c> version %u.%u no longer supported\n", ...);
			rds_conn_destroy(conn);
			return;
		}
	}

The version comes from event->param.conn.private_data via
rds_ib_set_protocol(), so a remote peer can have a socket-cached outgoing
connection destroyed during ordinary connection establishment, with no
netns teardown, module unload or device removal involved.  Could the
paragraph mention that path too, since it changes how reachable the fixed
race is?

> diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
> index d5defe9172e36..1cc20b5cfd213 100644
> --- a/net/rds/af_rds.c
> +++ b/net/rds/af_rds.c
> @@ -80,6 +80,14 @@ static int rds_release(struct socket *sock)
>  	rds_notify_queue_get(rs, NULL);
>  	rds_notify_msg_zcopy_purge(&rs->rs_zcookie_queue);
>  
> +	/* drop the cached connection reference; no sendmsg can race
> +	 * with us here, the socket is going away
> +	 */
> +	if (rs->rs_conn) {
> +		rds_conn_put(rs->rs_conn);
> +		rs->rs_conn = NULL;
> +	}
> +

[Severity: Low]
This access does not take rs->rs_lock, while the new struct field comment
added for rs_conn in net/rds/rds.h states unconditionally that it "is read
and written under rs_lock" (see the rds.h hunk below).

The access itself looks fine, since rds_release() runs from sock_close()
after the last file reference is gone, but the struct-level contract is the
one a later reader will consult.  Would it be worth noting the socket
teardown exception in the rds.h comment, so that someone adding a
lockdep_assert_held(&rs->rs_lock) to an rs_conn accessor does not get a
splat from here?

> @@ -267,18 +276,23 @@ static int rds_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
>  		else
>  			return -ENOIOCTLCMD;
>  
> -		spin_lock_bh(&rds_sock_lock);
> +		/* rs_conn is serialized by rs_lock (see rds_sendmsg());
> +		 * hold it across the "no connection yet" check and the
> +		 * rs_tos store so a racing sendmsg cannot cache a conn
> +		 * whose c_tos then disagrees with rs_tos.
> +		 */
> +		spin_lock_irqsave(&rs->rs_lock, flags);
>  		if (rs->rs_tos || rs->rs_conn) {
> -			spin_unlock_bh(&rds_sock_lock);
> +			spin_unlock_irqrestore(&rs->rs_lock, flags);
>  			return -EINVAL;
>  		}
>  		rs->rs_tos = tos;
> -		spin_unlock_bh(&rds_sock_lock);
> +		spin_unlock_irqrestore(&rs->rs_lock, flags);
>  		break;

[Severity: Low]
This is the plain store side of rs_tos, now under rs_lock.

In rds_sendmsg() the same field is still read without the lock and without
READ_ONCE() when it is handed to rds_conn_create_outgoing():

	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);

The other two rs_tos reads in rds_sendmsg() (the cache hit test and the
c_tos re-check) are both under rs_lock, so the lockset for this one access
is empty while a SIOCRDSSETTOS store runs under rs_lock.  Is this
unmarked concurrent access intentional in a commit whose purpose is to
remove that class of KCSAN report?  A READ_ONCE() on the sample would
document it.

The value itself is re-validated under rs_lock before the connection is
installed, so no stale ToS is sent on.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 1d48da1a794fa..965d68e51a1c1 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -215,7 +237,20 @@ static struct rds_connection *__rds_conn_create(struct net *net,
>  		 * We need a second connection object into which we
>  		 * can stick the other QP. */
>  		parent = conn;
> -		conn = parent->c_passive;
> +		/* The c_passive pointer holds a reference which is only
> +		 * dropped one synchronize_rcu() after the pointer is
> +		 * cleared, so within this RCU section a fetched pointer
> +		 * is always safe to take a reference on.  A passive conn
> +		 * whose own destroy has begun is not handed out, though:
> +		 * it is quiesced and about to clear the parent's pointer
> +		 * itself, and reusing it would re-arm a connection that
> +		 * nothing will tear down again.
> +		 */
> +		conn = rcu_dereference(parent->c_passive);
> +		if (conn && READ_ONCE(conn->c_destroy_in_prog))
> +			conn = NULL;
> +		if (conn)
> +			rds_conn_get(conn);
>  	}

[Severity: Low]
This reads c_destroy_in_prog directly, but net/rds/rds.h documents the
field as "Set once, by rds_conn_destroy(), before it cancels the path
works; read through rds_destroy_pending()".

rds_destroy_pending() also covers the netns and transport-unloading cases:

	static inline bool rds_destroy_pending(struct rds_connection *conn)
	{
		return READ_ONCE(conn->c_destroy_in_prog) ||
		       !check_net(rds_conn_net(conn)) ||
		       (conn->c_trans->t_unloading && conn->c_trans->t_unloading(conn));
	}

and the fresh-conn gate earlier in this same function still uses it:

	if (rds_destroy_pending(conn))
		ret = -ENETDOWN;
	else
		ret = trans->conn_alloc(conn, GFP_ATOMIC);

so __rds_conn_create() now expresses "this conn is going away" with two
different predicates.  Should these new sites use rds_destroy_pending(),
or should the rds.h comment be relaxed to sanction raw reads of the flag
and say why the netns/unloading components are deliberately excluded here?

> @@ -334,13 +369,44 @@ static struct rds_connection *__rds_conn_create(struct net *net,
>  	spin_lock_irqsave(&rds_conn_lock, flags);
>  	if (parent) {
>  		/* Creating passive conn */
> -		if (parent->c_passive) {
> +		if (READ_ONCE(parent->c_destroy_in_prog)) {
> +			/* The parent's destroy has begun (it sets the
> +			 * flag and snatches c_passive under this
> +			 * lock); do not install a new passive conn
> +			 * that nothing would ever destroy.
> +			 */
> +			rds_conn_free_transport_data(conn, npaths);
> +			free_cp = conn->c_path;
> +			kmem_cache_free(rds_conn_slab, conn);
> +			conn = ERR_PTR(-ENETDOWN);
> +		} else if (rcu_access_pointer(parent->c_passive)) {
> +			struct rds_connection *passive;
> +
> +			passive = rds_conn_passive_locked(parent);
>  			rds_conn_free_transport_data(conn, npaths);
>  			free_cp = conn->c_path;
>  			kmem_cache_free(rds_conn_slab, conn);
> -			conn = parent->c_passive;
> +			if (READ_ONCE(passive->c_destroy_in_prog)) {

These are the other two raw reads of the flag referred to above.

> +				/* Its destroy will clear the parent's
> +				 * pointer under this lock shortly; until
> +				 * then there is no usable passive conn.
> +				 */
> +				conn = ERR_PTR(-ENETDOWN);
> +			} else {
> +				rds_conn_get(passive);
> +				conn = passive;
> +			}
>  		} else {
> -			parent->c_passive = conn;
> +			/* The initial reference belongs to whoever
> +			 * destroys the conn (the transport's conn
> +			 * lists, as for any other conn).  Take one
> +			 * for the c_passive pointer - dropped when
> +			 * the parent is destroyed - and one for our
> +			 * caller.
> +			 */
> +			rds_conn_get(conn);	/* c_passive */
> +			rds_conn_get(conn);	/* caller */
> +			rcu_assign_pointer(parent->c_passive, conn);
>  			rds_cong_add_conn(conn);
>  			rds_conn_count++;
>  			atomic_inc(&conn->c_trans->t_conn_count);

[ ... ]

> @@ -702,7 +777,38 @@ void rds_conn_destroy(struct rds_connection *conn)
>  
>  	/* Ensure conn will not be scheduled for reconnect */
>  	hlist_del_init_rcu(&conn->c_hash_node);
> +
> +	/* Snatch c_passive while holding the lock:
> +	 * __rds_conn_create() dereferences it under rcu_read_lock()
> +	 * (and refuses to install a new one once c_destroy_in_prog is
> +	 * set, which it checks under this lock).  After the
> +	 * synchronize_rcu() below no one can pick the pointer up any
> +	 * more and its reference can be dropped.
> +	 */
> +	passive = rds_conn_passive_locked(conn);
> +	RCU_INIT_POINTER(conn->c_passive, NULL);
> +
> +	/* If we are a parent's passive twin, invalidate its pointer to
> +	 * us as well, so that __rds_conn_create() cannot hand out a
> +	 * connection whose teardown has begun.  The parent is the
> +	 * hashed connection for our key (a passive conn is never
> +	 * hashed, and we unhashed ourselves above); it holds its
> +	 * initial reference for as long as it is hashed, so the lookup
> +	 * reference dropped below cannot be its last.
> +	 */
> +	head = rds_conn_bucket(&conn->c_laddr, &conn->c_faddr);
> +	rcu_read_lock();
> +	parent = rds_conn_lookup(rds_conn_net(conn), head, &conn->c_laddr,
> +				 &conn->c_faddr, conn->c_trans, conn->c_tos,
> +				 conn->c_dev_if);
> +	rcu_read_unlock();
> +	if (parent && rds_conn_passive_locked(parent) == conn) {
> +		RCU_INIT_POINTER(parent->c_passive, NULL);
> +		was_passive = true;
> +	}
>  	spin_unlock_irq(&rds_conn_lock);
> +	if (parent)
> +		rds_conn_put(parent);
>  	synchronize_rcu();

[ ... ]

> @@ -719,6 +825,15 @@ void rds_conn_destroy(struct rds_connection *conn)
>  	 */
>  	rds_cong_remove_conn(conn);
>  
> +	/* drop the reference our c_passive pointer held, if any, and
> +	 * the one a parent's c_passive pointer held on us; neither can
> +	 * be the last, since the initial reference is dropped below
> +	 */
> +	if (passive)
> +		rds_conn_put(passive);

[Severity: Low]
Does the "neither can be the last" claim hold for the passive put?  The
reference dropped here is the parent's c_passive reference on the passive
twin, and the twin's own initial reference is dropped by the twin's own
rds_conn_destroy(), not by the put below.

Parent A snatches passive = P and clears A->c_passive under rds_conn_lock,
then spends an unbounded time in synchronize_rcu() and
rds_conn_path_quiesce().  Meanwhile P's own destroy runs, e.g. from the
peer protocol-version path in rds_ib_cm_connect_complete():

	rds_conn_destroy(P)
	  parent lookup finds nothing (A is already unhashed)
	  was_passive == false
	  rds_conn_put(P)		/* P's initial reference */

P is then left at refcount 1, and A's rds_conn_put(passive) is the final
put, running rds_conn_destroy_fini() -> rds_conn_path_free()
(destroy_workqueue()) plus kmem_cache_free() and rds_conn_lock inline from
rds_conn_destroy().

That is legal where the put currently sits, since rds_conn_destroy() is in
sleepable context with no locks held.  Would it be more accurate to say
that this put may be the final one and must therefore stay outside
rds_conn_lock and in sleepable context, rather than that it cannot be the
last?  As written, someone relying on the stated invariant to move the
puts under rds_conn_lock would recurse on that lock via
rds_conn_destroy_fini().

> +	if (was_passive)
> +		rds_conn_put(conn);
> +
>  	/* drop the initial reference; the connection is freed from
>  	 * rds_conn_destroy_fini() once every holder has dropped theirs
>  	 */
> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 26a32c1ec8f7f..98f34b494237a 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -924,8 +924,15 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id,
>  		rds_ib_conn_error(conn, "rdma_accept failed\n");
>  
>  out:
> -	if (conn)
> +	if (conn) {
>  		mutex_unlock(&conn->c_cm_lock);
> +		/* Drop the reference rds_conn_create() handed us.  The
> +		 * conn stays reachable through cm_id->context without a
> +		 * reference of its own for now; the CM event handler is
> +		 * given one of its own by a following patch.
> +		 */
> +		rds_conn_put(conn);
> +	}

[Severity: Low]
This isn't a bug introduced by this patch, but for the record: after this
put, cm_id->context is a bare pointer that
rds_rdma_cm_event_handler_cmn() dereferences with no reference of its own
(mutex_lock(&conn->c_cm_lock) on the mutex embedded in the connection), so
in principle a concurrent rds_conn_destroy() plus final rds_conn_put()
could free it under an in-flight CM event.

Before this patch rds_conn_create() returned a bare pointer and
cm_id->context equally held no reference, so nothing is regressed here,
and the following patch in the series does close it -
rds_rdma_cm_event_handler_cmn() gains

	if (conn && !rds_conn_get_unless_zero(conn)) {
		...
		return 0;
	}

with a matching rds_conn_put() at out:.  Only noting it so the transient
state in this patch is not mistaken for the final one.

> diff --git a/net/rds/rds.h b/net/rds/rds.h
> index b3cc0804156e8..321f2da9e76d9 100644
> --- a/net/rds/rds.h
> +++ b/net/rds/rds.h
> @@ -669,7 +669,9 @@ struct rds_sock {
>  
>  	/*
>  	 * rds_sendmsg caches the conn it used the last time around.
> -	 * This helps avoid costly lookups.
> +	 * This helps avoid costly lookups.  The cache owns a connection
> +	 * reference, dropped when it is replaced or the socket is
> +	 * released, and is read and written under rs_lock.
>  	 */
>  	struct rds_connection	*rs_conn;

This is the contract that rds_release() does not follow, per the comment
on the af_rds.c hunk above.

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 32c411d10e3ef..2d7839438abdc 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c
> @@ -1340,11 +1341,29 @@ 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 && ipv6_addr_equal(&conn->c_faddr, &daddr) &&
> +	    rs->rs_tos == conn->c_tos && !rds_destroy_pending(conn)) {
> +		rds_conn_get(conn);

[Severity: Medium]
This is a pre-existing issue and not introduced by this patch, but since
the hit predicate is being rewritten here: it compares c_faddr, ToS and
destroy state, and never compares the requested scope_id with
conn->c_dev_if, even though the miss path below passes scope_id as the
dev_if that becomes part of the connection identity.

For a socket bound to a non-link-local IPv6 address, rs_bound_scope_id is
0, and the earlier scope check accepts any non-zero destination scope:

	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) {
			release_sock(sk);
			ret = -EINVAL;
			...
		}
	}

So a send to fe80::x%ifA followed by a send to fe80::x%ifB appears to hit
the cached ifA connection and deliver the second message to the ifA peer,
because the lookup that would distinguish c_dev_if is bypassed.  Would
adding conn->c_dev_if == scope_id to the hit condition be the right fix
here?

The baseline condition omits c_dev_if in exactly the same way, so this
patch does not change the behaviour.

> +	} else {
> +		conn = NULL;
> +	}
> +	spin_unlock_irqrestore(&rs->rs_lock, flags);
> +
> +	if (!conn) {
> +		struct rds_connection *old;
> +
>  		conn = rds_conn_create_outgoing(sock_net(sock->sk),
>  						&rs->rs_bound_addr, &daddr,
>  						rs->rs_transport, rs->rs_tos,

This is the unlocked rs_tos sample discussed on the rds_ioctl() hunk
above.

> @@ -1352,9 +1371,28 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
>  						scope_id);
>  		if (IS_ERR(conn)) {
>  			ret = PTR_ERR(conn);
> +			conn = NULL;
>  			goto out;
>  		}
> +		/* rs_tos was sampled without rs_lock for the create above,
> +		 * and SIOCRDSSETTOS only refuses a change once rs_conn is
> +		 * set, so it can have changed underneath us.  Do not
> +		 * install - or send on - a connection whose ToS no longer
> +		 * matches the socket's; the retry uses the new one.
> +		 */
> +		spin_lock_irqsave(&rs->rs_lock, flags);
> +		if (conn->c_tos != rs->rs_tos) {
> +			spin_unlock_irqrestore(&rs->rs_lock, flags);
> +			ret = -EAGAIN;
> +			goto out;
> +		}
> +		/* hand the cache its own reference */
> +		rds_conn_get(conn);
> +		old = rs->rs_conn;
>  		rs->rs_conn = conn;
> +		spin_unlock_irqrestore(&rs->rs_lock, flags);
> +		if (old)
> +			rds_conn_put(old);
>  	}

Thanks for looking at these.

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