[PATCH] rxrpc: Serialize client connection IDR updates

[PATCH] rxrpc: Serialize client connection IDR updates

[Kyle Zeng]

The common connection destructor calls rxrpc_kill_client_conn() for
service connections as well as client connections.  A service connection
gets its CID from the peer and never owns an entry in local->conn_ids.
Its destruction can therefore remove an unrelated client connection.

The final reference can be dropped by the service reaper or a call
destruction worker, outside the owning local endpoint's I/O thread.
Client connection destruction can also run off-thread, and can remove an
ID already released by rxrpc_unbundle_conn().  These writers can race
idr_alloc_cyclic(), corrupting the radix tree and its RCU callbacks.  A
second removal can also delete a new owner after an ID has been reused.

Only run client cleanup for client connections.  Restore a per-local IDR
writer lock and the client-ID ownership flag, clearing the flag under the
lock when releasing the ID.  Serialize IDR destruction with the same
lock.  This covers deferred final puts without changing connection
expiry or the RCU lookup path.

Fixes: 9d35d880e0e4 ("rxrpc: Move client call connection to the I/O thread")
Signed-off-by: Kyle Zeng <kylebot@openai.com>
---
I have a reproducer for this issue and can share via DM

 net/rxrpc/ar-internal.h  |  2 ++
 net/rxrpc/conn_client.c  | 17 ++++++++++++++---
 net/rxrpc/conn_object.c  |  3 ++-
 net/rxrpc/local_object.c |  1 +
 4 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 865f05fe37ab..5e49fe0f4a6f 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -377,6 +377,7 @@ struct rxrpc_local {
 	bool			dead;
 	bool			service_closed;	/* Service socket closed */
 	struct idr		conn_ids;	/* List of connection IDs */
+	spinlock_t		conn_ids_lock;	/* Lock for conn_ids */
 	struct list_head	new_client_calls; /* Newly created client calls need connection */
 	spinlock_t		client_call_lock; /* Lock for ->new_client_calls */
 	struct sockaddr_rxrpc	srx;		/* local address */
@@ -472,6 +473,7 @@ enum rxrpc_call_completion {
  * Bits in the connection flags.
  */
 enum rxrpc_conn_flag {
+	RXRPC_CONN_HAS_IDR,		/* Has a client conn ID assigned */
 	RXRPC_CONN_IN_SERVICE_CONNS,	/* Conn is in peer->service_conns */
 	RXRPC_CONN_DONT_REUSE,		/* Don't reuse this connection */
 	RXRPC_CONN_PROBING_FOR_UPGRADE,	/* Probing for service upgrade */
diff --git a/net/rxrpc/conn_client.c b/net/rxrpc/conn_client.c
index 48519f0de185..4c2db83fceae 100644
--- a/net/rxrpc/conn_client.c
+++ b/net/rxrpc/conn_client.c
@@ -40,12 +40,16 @@ static void rxrpc_activate_bundle(struct rxrpc_bundle *bundle)
 }
 
 /*
- * Release a connection ID for a client connection.
+ * Release a client connection ID exactly once.  The final reference may be
+ * dropped outside the local endpoint's I/O thread.
  */
 static void rxrpc_put_client_connection_id(struct rxrpc_local *local,
 					   struct rxrpc_connection *conn)
 {
-	idr_remove(&local->conn_ids, conn->proto.cid >> RXRPC_CIDSHIFT);
+	spin_lock(&local->conn_ids_lock);
+	if (test_and_clear_bit(RXRPC_CONN_HAS_IDR, &conn->flags))
+		idr_remove(&local->conn_ids, conn->proto.cid >> RXRPC_CIDSHIFT);
+	spin_unlock(&local->conn_ids_lock);
 }
 
 /*
@@ -56,6 +60,7 @@ static void rxrpc_destroy_client_conn_ids(struct rxrpc_local *local)
 	struct rxrpc_connection *conn;
 	int id;
 
+	spin_lock(&local->conn_ids_lock);
 	if (!idr_is_empty(&local->conn_ids)) {
 		idr_for_each_entry(&local->conn_ids, conn, id) {
 			pr_err("AF_RXRPC: Leaked client conn %p {%d}\n",
@@ -65,6 +70,7 @@ static void rxrpc_destroy_client_conn_ids(struct rxrpc_local *local)
 	}
 
 	idr_destroy(&local->conn_ids);
+	spin_unlock(&local->conn_ids_lock);
 }
 
 /*
@@ -162,15 +168,20 @@ rxrpc_alloc_client_connection(struct rxrpc_bundle *bundle)
 	if (!conn)
 		return ERR_PTR(-ENOMEM);
 
+	spin_lock(&local->conn_ids_lock);
 	id = idr_alloc_cyclic(&local->conn_ids, conn, 1, 0x40000000,
 			      GFP_ATOMIC | __GFP_NOWARN);
+	if (id >= 0) {
+		conn->proto.cid = id << RXRPC_CIDSHIFT;
+		set_bit(RXRPC_CONN_HAS_IDR, &conn->flags);
+	}
+	spin_unlock(&local->conn_ids_lock);
 	if (id < 0) {
 		kfree(conn);
 		return ERR_PTR(id);
 	}
 
 	refcount_set(&conn->ref, 1);
-	conn->proto.cid		= id << RXRPC_CIDSHIFT;
 	conn->proto.epoch	= local->rxnet->epoch;
 	conn->out_clientflag	= RXRPC_CLIENT_INITIATED;
 	conn->bundle		= rxrpc_get_bundle(bundle, rxrpc_bundle_get_client_conn);
diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
index 0ece717db0f8..bae95cadd72b 100644
--- a/net/rxrpc/conn_object.c
+++ b/net/rxrpc/conn_object.c
@@ -332,7 +332,8 @@ static void rxrpc_clean_up_connection(struct work_struct *work)
 	rxrpc_purge_queue(&conn->rx_queue);
 	rxrpc_free_skb(conn->tx_response, rxrpc_skb_put_response);
 
-	rxrpc_kill_client_conn(conn);
+	if (rxrpc_conn_is_client(conn))
+		rxrpc_kill_client_conn(conn);
 
 	conn->security->clear(conn);
 	key_put(conn->key);
diff --git a/net/rxrpc/local_object.c b/net/rxrpc/local_object.c
index 169f9dfdaa77..5a7acd595e11 100644
--- a/net/rxrpc/local_object.c
+++ b/net/rxrpc/local_object.c
@@ -140,6 +140,7 @@ static struct rxrpc_local *rxrpc_alloc_local(struct net *net,
 		memcpy(&local->srx, srx, sizeof(*srx));
 		local->srx.srx_service = 0;
 		idr_init(&local->conn_ids);
+		spin_lock_init(&local->conn_ids_lock);
 		get_random_bytes(&tmp, sizeof(tmp));
 		tmp &= 0x3fffffff;
 		if (tmp == 0)

base-commit: d89e1d186ec9271bf78e62771779e9fc03bf0fc3
-- 
2.56.0

Re: [PATCH] rxrpc: Serialize client connection IDR updates

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

Re: [PATCH] rxrpc: Serialize client connection IDR updates

[Andrew Lunn]

On Thu, Oct 29, 2026 at 09:21:29PM -0700, Kyle Zeng wrote:
> The common connection destructor calls rxrpc_kill_client_conn() for
> service connections as well as client connections.  A service connection
> gets its CID from the peer and never owns an entry in local->conn_ids.
> Its destruction can therefore remove an unrelated client connection.
> 
> The final reference can be dropped by the service reaper or a call
> destruction worker, outside the owning local endpoint's I/O thread.
> Client connection destruction can also run off-thread, and can remove an
> ID already released by rxrpc_unbundle_conn().  These writers can race
> idr_alloc_cyclic(), corrupting the radix tree and its RCU callbacks.  A
> second removal can also delete a new owner after an ID has been reused.
> 
> Only run client cleanup for client connections.  Restore a per-local IDR
> writer lock and the client-ID ownership flag, clearing the flag under the
> lock when releasing the ID.  Serialize IDR destruction with the same
> lock.  This covers deferred final puts without changing connection
> expiry or the RCU lookup path.
> 
> Fixes: 9d35d880e0e4 ("rxrpc: Move client call connection to the I/O thread")
> Signed-off-by: Kyle Zeng <kylebot@openai.com>
> ---
> I have a reproducer for this issue and can share via DM

Please take a read of

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

You need to set the subject line correctly to indicate the three they
are applied to.

All the patches you have posted are very likely to be sent to
/dev/null because of this. It is a good idea to start slow, only one
patch at a time, and figure out the processes.

      Andrew

Re: [PATCH] rxrpc: Serialize client connection IDR updates

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 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).

- [High] Local endpoint teardown can still reach the BUG() in
  rxrpc_destroy_client_conn_ids() while a replaced client connection's…
- [Low] The commit message says the patch works "without changing
  connection expiry", but it does change client-connection expiry.

--

rxrpc: Serialize client connection IDR updates

This patch limits the rxrpc_kill_client_conn() call in the common
connection destructor to client connections. Service connections
therefore no longer remove unrelated entries from local->conn_ids. It
also adds a per-local conn_ids_lock and an RXRPC_CONN_HAS_IDR ownership
flag, so that a client ID is released only once, and takes the same lock
around IDR allocation and destruction.

> diff --git a/net/rxrpc/conn_client.c b/net/rxrpc/conn_client.c
> index 48519f0de1853..4c2db83fceaee 100644
> --- a/net/rxrpc/conn_client.c
> +++ b/net/rxrpc/conn_client.c

[ ... ]

> @@ -56,6 +60,7 @@ static void rxrpc_destroy_client_conn_ids(struct rxrpc_local *local)
>  	struct rxrpc_connection *conn;
>  	int id;
>  
> +	spin_lock(&local->conn_ids_lock);
>  	if (!idr_is_empty(&local->conn_ids)) {
>  		idr_for_each_entry(&local->conn_ids, conn, id) {
>  			pr_err("AF_RXRPC: Leaked client conn %p {%d}\n",
> @@ -65,6 +70,7 @@ static void rxrpc_destroy_client_conn_ids(struct rxrpc_local *local)
>  	}
>  
>  	idr_destroy(&local->conn_ids);
> +	spin_unlock(&local->conn_ids_lock);
>  }

[Severity: High]
The commit message says this "covers deferred final puts". Can teardown
still reach the BUG() here while a deferred final put still owns an IDR
entry?

The lock only orders the deferred put against idr_destroy(). If teardown
takes the lock first, the entry is still in the IDR.

One way this can happen is with a replaced client connection.
rxrpc_add_conn_to_bundle() takes a non-reusable connection out of its
slot without releasing its ID:

net/rxrpc/conn_client.c:rxrpc_add_conn_to_bundle() {
	old = bundle->conns[slot];
	if (old) {
		bundle->conns[slot] = NULL;
		bundle->conn_ids[slot] = 0;
		trace_rxrpc_client(old, -1, rxrpc_client_replace);
		rxrpc_put_connection(old, rxrpc_conn_put_noreuse);
	}
	...
}

So RXRPC_CONN_HAS_IDR stays set on the old connection. rxrpc_unbundle_conn()
only releases the ID if the connection still owns its slot:

	if (bundle->conns[bindex] == conn) {
		...
		rxrpc_put_client_connection_id(bundle->local, conn);

The old connection can still have an active call when it is replaced.
rxrpc_may_reuse_conn() marks it RXRPC_CONN_DONT_REUSE on ID drift from the
cursor, on a remote abort, or when the call counter runs out.

When the socket closes, that call is aborted and disconnected, and its
final put goes through rxrpc_cleanup_call():

	if (rcu_read_lock_held())
		schedule_work(&call->destroyer);
	else
		rxrpc_destroy_call(&call->destroyer);

Without CONFIG_DEBUG_LOCK_ALLOC, rcu_read_lock_held() always returns 1. The
destroyer then always goes to system_wq and holds the call->conn reference
until it runs. rxrpc_release_sock() flushes only rxrpc_workqueue.

The I/O thread can then run:

rxrpc_destroy_local()
  rxrpc_clean_up_local_conns()
    rxrpc_unbundle_conn()        <- skips the ID release, slot was replaced
    rxrpc_put_connection()       <- not final, the destroyer holds a ref
  rxrpc_purge_client_connections()
    rxrpc_destroy_client_conn_ids()
      IDR not empty -> "Leaked client conn" -> BUG()

rxrpc_put_connection() can also defer the final put itself:

	if (in_softirq() || work_busy(&conn->processor) ||
	    timer_pending(&conn->timer))
		schedule_work(&conn->destructor);

rxrpc_process_connection() can be the caller whose rxrpc_unuse_local()
drops active_users to zero and starts teardown while its own work item is
still running.

The missing ID release for replaced connections and this BUG() both
predate this patch. Still, the race between deferred final puts and IDR
destruction that the commit message says is covered can still crash.

Could the ID be released when a replaced connection is retired, even if
it no longer holds its bundle slot? Alternatively, teardown could wait
for pending ID releases before the empty-tree check, without holding
conn_ids_lock while it waits.

[ ... ]

> diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
> index 0ece717db0f85..bae95cadd72bb 100644
> --- a/net/rxrpc/conn_object.c
> +++ b/net/rxrpc/conn_object.c
> @@ -332,7 +332,8 @@ static void rxrpc_clean_up_connection(struct work_struct *work)
>  	rxrpc_purge_queue(&conn->rx_queue);
>  	rxrpc_free_skb(conn->tx_response, rxrpc_skb_put_response);
>  
> -	rxrpc_kill_client_conn(conn);
> +	if (rxrpc_conn_is_client(conn))
> +		rxrpc_kill_client_conn(conn);

[Severity: Low]
This isn't a bug, but is this part of the commit message accurate?

    This covers deferred final puts without changing connection
    expiry or the RCU lookup path.

rxrpc_kill_client_conn() calls atomic_dec(&rxnet->nr_client_conns) without
checking the connection type. The only increment is in
rxrpc_alloc_client_connection().

Before this patch, every destroyed service connection also decremented
the counter. On hosts that serve calls, it could therefore go negative.

rxrpc_discard_expired_client_conns() reads the counter into an unsigned
int:

	nr_conns = atomic_read(&local->rxnet->nr_client_conns);
	...
		expiry = rxrpc_conn_idle_client_expiry;
		if (nr_conns > rxrpc_reap_client_connections)
			expiry = rxrpc_conn_idle_client_fast_expiry;

A negative value wraps to a huge number, so the 2*HZ fast expiry is
always chosen. With the new rxrpc_conn_is_client() check, the counter
stays balanced and idle client connections get the normal 2*60*HZ expiry
again.

The new behaviour looks correct. Could the commit message mention the
nr_client_conns accounting fix and the expiry change that comes with it,
instead of saying expiry is unchanged?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030042129.73690-1-kylebot%40openai.com