[PATCH net v2] rxrpc: Serialize client connection IDR updates

[PATCH net v2] rxrpc: Serialize client connection IDR updates

[Kyle Zeng]

Service connections use peer-selected CIDs and do not own client IDR
entries, but the common destructor removes those IDs. This can delete a
live client connection or race allocation and corrupt the IDR and its
RCU callbacks. Client destruction can also repeat an earlier removal
and delete a reused ID.

Restrict client cleanup to clients. Restore a per-local writer lock and
an ownership flag so each ID is released once. Release IDs at idle
retirement even if the bundle slot was replaced; otherwise deferred
final puts can leave the IDR nonempty at endpoint teardown and trigger
BUG(). Keeping IDs until retirement preserves active calls and terminal
retransmissions. Client-only cleanup also prevents nr_client_conns
underflow and restores the intended expiry selection without changing
the timeout values.

The issue was found by an LLM.

Fixes: 9d35d880e0e4 ("rxrpc: Move client call connection to the I/O thread")
Cc: stable@vger.kernel.org
Signed-off-by: Kyle Zeng <kylebot@openai.com>
---
v2:
  - Release a retired client connection's ID even if its bundle slot
    has been replaced. Keep the ID until retirement so active calls
    and terminal retransmissions remain reachable.
  - Describe the nr_client_conns accounting correction and its effect
    on normal-versus-fast expiry selection.
  - Repeat the RxRPC module and targeted x86 allmodconfig/allyesconfig
    W=1 builds. The reproducer was not rerun for v2.

v1: https://lore.kernel.org/r/20261030042129.73690-1-kylebot@openai.com
Review: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261030042129.73690-1-kylebot%40openai.com

I have a reproducer for this issue and can share via DM

 net/rxrpc/ar-internal.h  |  2 ++
 net/rxrpc/conn_client.c  | 21 +++++++++++++++++----
 net/rxrpc/conn_object.c  |  3 ++-
 net/rxrpc/local_object.c |  1 +
 4 files changed, 22 insertions(+), 5 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..201a50caeaca 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);
@@ -682,6 +693,9 @@ static void rxrpc_unbundle_conn(struct rxrpc_connection *conn)
 	if (conn->flags & RXRPC_CONN_FINAL_ACK_MASK)
 		rxrpc_process_delayed_final_acks(conn, true);
 
+	/* A replaced connection is no longer in the bundle. */
+	rxrpc_put_client_connection_id(bundle->local, conn);
+
 	bindex = conn->bundle_shift / RXRPC_MAXCALLS;
 	if (bundle->conns[bindex] == conn) {
 		_debug("clear slot %u", bindex);
@@ -689,7 +703,6 @@ static void rxrpc_unbundle_conn(struct rxrpc_connection *conn)
 		bundle->conn_ids[bindex] = 0;
 		for (i = 0; i < RXRPC_MAXCALLS; i++)
 			clear_bit(conn->bundle_shift + i, &bundle->avail_chans);
-		rxrpc_put_client_connection_id(bundle->local, conn);
 		rxrpc_deactivate_bundle(bundle);
 		rxrpc_put_connection(conn, rxrpc_conn_put_unbundle);
 	}
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: b3dfbde62cf1981c4638939c159dfd087b7a72e9
-- 
2.55.0.openai.911.g06873a30d2f3

Re: [PATCH net v2] 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: 0 (1) · Medium: 1 · 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).

- [Medium] The patch makes rxrpc_destroy_client_conn_ids() take
  local->conn_ids_lock before its leak check, and BUG() is then called…
- [Low] rxrpc: missing Fixes tag for service conn cleanup in
  rxrpc_clean_up_connection()

Pre-existing issues:
- [High] This bug predates the patch, but it is High severity.

--

rxrpc: Serialize client connection IDR updates

This patch limits connection ID cleanup in the common destructor to client
connections, so service connections no longer remove IDs from the client
IDR. It adds a per-local conn_ids_lock and a RXRPC_CONN_HAS_IDR ownership
flag so that each ID is released only once. It also moves the ID release
in rxrpc_unbundle_conn() so the ID is dropped even when the bundle slot
has already been replaced.

> diff --git a/net/rxrpc/conn_client.c b/net/rxrpc/conn_client.c
> index 48519f0de1853..201a50caeacae 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);
>  }

[Severity: Medium]
If the leak check fires, BUG() now runs with local->conn_ids_lock held,
and the matching spin_unlock() is never reached. What happens to the
leaked connections after that?

Each leaked client connection still holds a ref on local, taken by
rxrpc_get_local() in rxrpc_alloc_client_connection(), so the lock memory
stays valid. When the final ref on one of them is dropped later, directly
or through schedule_work(), the path is:

rxrpc_put_connection()
  rxrpc_clean_up_connection()
    rxrpc_kill_client_conn()
      rxrpc_put_client_connection_id()
        spin_lock(&local->conn_ids_lock);

That spin_lock() would spin forever on the lock left behind by the dead
I/O thread. Before this patch, BUG() ran with no lock held.

The commit message says deferred final puts happened in practice at
endpoint teardown. Could one oops here turn into a soft lockup or a hung
kworker?

One option would be to do the check and the pr_err() output under the
lock, drop the lock, and then call BUG(). Another would be to WARN and
clean up instead.

[ ... ]

> @@ -682,6 +693,9 @@ static void rxrpc_unbundle_conn(struct rxrpc_connection *conn)
>  	if (conn->flags & RXRPC_CONN_FINAL_ACK_MASK)
>  		rxrpc_process_delayed_final_acks(conn, true);
>  
> +	/* A replaced connection is no longer in the bundle. */
> +	rxrpc_put_client_connection_id(bundle->local, conn);
> +

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Since
this patch now handles replaced connections in rxrpc_unbundle_conn(),
should the disconnect path handle them too?

rxrpc_add_conn_to_bundle() replaces any slot whose connection fails
rxrpc_may_reuse_conn(). It does not check whether the old connection
still has active calls (old->act_chans):

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);
	}
    ...
	conn->bundle_shift = shift;
	bundle->conns[slot] = conn;
    ...
	for (i = 0; i < RXRPC_MAXCALLS; i++)
		set_bit(shift + i, &bundle->avail_chans);
}

A connection can become non-reusable while calls are still running on it.
rxrpc_set_conn_aborted() sets RXRPC_CONN_DONT_REUSE on an abort, which a
remote peer can trigger. rxrpc_expose_client_call() sets it when
call_counter reaches INT_MAX. rxrpc_may_reuse_conn() sets it when the CID
is too far from the IDR cursor.

The new connection B gets the same bundle_shift as the old connection A.
New calls take B's channels. When A's still-active call on channel c
finishes, rxrpc_disconnect_client_call() runs:

	chan->call = NULL;
	set_bit(conn->bundle_shift + channel, &conn->bundle->avail_chans);

Nothing checks that bundle->conns[slot] == conn, so this marks B's
channel c as free while a call Y1 is still using it.
rxrpc_activate_channels() then picks that bit and runs:

		slot = channel / RXRPC_MAXCALLS;
		conn = bundle->conns[slot];
		...
		rxrpc_activate_one_channel(conn, channel);

This overwrites B->channels[c].call with a second call Y2 while Y1 is
still attached. If Y1 has not been exposed yet, Y2 gets the same call_id,
so two calls can go out with the same CID and callNumber. When Y1
disconnects later, it hits WARN_ON(chan->call != call) and returns early.

Does rxrpc_disconnect_client_call() need to skip the avail_chans update
when conn is no longer in bundle->conns[]?

>  	bindex = conn->bundle_shift / RXRPC_MAXCALLS;
>  	if (bundle->conns[bindex] == conn) {
>  		_debug("clear slot %u", bindex);

[ ... ]

> diff --git a/net/rxrpc/conn_object.c b/net/rxrpc/conn_object.c
> --- 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);

[Severity: Low]
The changelog blames only 9d35d880e0e4 ('rxrpc: Move client call
connection to the I/O thread'). That commit dropped the
RXRPC_CONN_HAS_IDR test and the lock from
rxrpc_put_client_connection_id(). That covers the double removal
and the IDR locking parts of this fix.

The service connection part seems to have a different origin. The
unconditional rxrpc_kill_client_conn() call in
rxrpc_clean_up_connection() looks older than that. It appears to
have come in when rxrpc_put_client_conn() and
rxrpc_put_service_conn() were merged into rxrpc_put_connection()
and the common destructor, in 3cec055c5695 ('rxrpc: Don't hold a
ref for connection workqueue').

Since that change, every service connection reaching the
destructor runs the client-only cleanup:

  - it decrements rxnet->nr_client_conns
  - it calls rxrpc_put_client_connection_id() on a CID the peer
    chose

The new rxrpc_conn_is_client() guard is what fixes this.

Could you check that history and, if it holds, add

  Fixes: 3cec055c5695 ("rxrpc: Don't hold a ref for connection workqueue")

alongside the existing tag? Then the stable maintainers can see
which trees need the rxrpc_conn_is_client() guard.

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