[PATCH net-next v2 0/9] net/rds: make connection lifetime reference-counted

[PATCH net-next v2 0/9] net/rds: make connection lifetime reference-counted

[Allison Henderson]

Hi all,

This is v2 of the connection-lifetime set (v1 at [1]), following
"net/rds: own the fastpath locks across connection teardown", which is
in net-next.  It is targeted at net-next: although the series fixes
real use-after-frees (one syzbot report and one report from Chengfeng
Ye below), it does so by reworking connection lifetime rather than
patching the individual crash sites, and that rework is too invasive
for net.

rds_conn_destroy() frees the connection, its paths and its workqueues
on the spot, relying on the documented assumption that "no one else is
referencing the connection".  That assumption stopped being true a
long time ago: connections are destroyed on netns teardown and on a
protocol-version mismatch as well as on rmmod, while pointers to them
still live in socket rs_conn caches, congestion-map conn lists, CM
callbacks and workers, and - for as long as an application leaves data
unread - in every rds_incoming sitting on a receive queue.  No single
Fixes: commit covers the rot, so the series carries Reported-by tags
where there are concrete reports instead.

  Patch 1 (new) guards the five work-arming sites that never tested
  rds_destroy_pending(): two IB completion paths, the IB recv refill,
  the TCP accept kick and the multipath reconnect in sendmsg.

  Patch 2 gives the connection its own destroy-in-progress marker so
  that the predicate also covers the one single-connection destroy.
  Based on UEK commits:
     e2f5005adf63 net/rds: Add krefs to struct rds_connection
     https://github.com/oracle/linux-uek/commit/e2f5005adf63

     6c53ef92f46e net/rds: Merge uses of conn->c_destroy_in_prog & RDS_DESTROY_PENDING
     https://github.com/oracle/linux-uek/commit/6c53ef92f46e

  Patch 3 splits rds_conn_destroy() into a synchronous quiesce and a
  kref-governed free.
  Based on UEK commits:
     2c8569e4c880 ("net/rds: Add krefs to struct rds_connection").
     https://github.com/oracle/linux-uek/commit/2c8569e4c880

  Patch 4 makes each transport's exit path wait for its connections to
  actually be freed before the module goes away.  The wait is
  unbounded and warns every ten seconds: a connection reference can be
  held for an application-controlled time (unread data), so a timeout
  would only move the use-after-free from freed memory to unloaded
  module text.  rmmod blocking while data is queued and unread is the
  historical RDS contract.  For IB the wait re-sweeps the nodev list,
  since device connections migrate to it asynchronously.
  Based on UEK commits:
     ece4b4e39afa ("net/rds: wait_event_timeout until zero connections during rmmod")
     https://github.com/oracle/linux-uek/commit/ece4b4e39afa

     905ec90e6166 ("net/rds: Each RDS transport should keep its own connection count")
     https://github.com/oracle/linux-uek/commit/905ec90e6166

  Patch 5 (new) unlinks each transport node before its destroy, so a
  free deferred past the teardown loop cannot write into the loop's
  stack-local list head.

  Patch 6 hands out real references everywhere a connection pointer
  previously escaped bare, RCU-annotates parent->c_passive, refuses to
  revive a passive connection whose destroy has begun, and serializes
  the SIOCRDSSETTOS check with the rs_conn cache.
  Based on UEK commits:
     2c8569e4c880 ("net/rds: Add krefs to struct rds_connection")
     https://github.com/oracle/linux-uek/commit/2c8569e4c880

     0e9e3a72b7f7 ("net/rds: rds_sendmsg must use rs_conn only when not being destroyed").
     https://github.com/oracle/linux-uek/commit/0e9e3a72b7f7

  Patch 7 (new) pins the connection across the RDMA-CM event handler
  and rejects a connect request for a connection whose destroy has
  already quiesced it.

  Patch 8 drops the now-unused global rds_conn_count.
  Based on UEK commits:
     905ec90e6166 ("net/rds: Each RDS transport should keep its own connection count").
     https://github.com/oracle/linux-uek/commit/905ec90e6166

  Patch 9 makes struct rds_incoming hold a reference on i_conn, the
  fix for the KASAN use-after-free Chengfeng Ye reported [2].
  Based on UEK commits:
      99b9a3715419 ("net/rds: fix crash by expanding kref coverage to rds_incoming.i_conn").
      https://github.com/oracle/linux-uek/commit/99b9a3715419

Patches 3, 6, 4 and 9 are ports of the connection kref work Sharath
Srinivasan did for Oracle UEK, adapted to the upstream code.

The series has been validated with the RDS selftests over loopback-TCP
and RXE-RDMA, plus churn tests that delete network namespaces and
unload the modules under live rds-stress traffic.

Changes since v1 [1]:
 - New patch 1: guard the five work-arming sites that never tested
   rds_destroy_pending(); patch 2's changelog and comments narrowed
   to what it actually newly covers.
 - Patch 4 moved ahead of the reference holders, so no bisect point
   has references without the unload wait; wait made unbounded with a
   periodic warning instead of a 10 s timeout; IB exit re-sweeps the
   nodev list for connections that detach from their device late.
 - New patch 5: transport nodes unlinked before destroy (stack list
   head use-after-free from a deferred conn_free).
 - Patch 6: c_passive RCU-annotated; a destroyed passive child is
   refused by __rds_conn_create() and clears the parent's pointer
   itself; SIOCRDSSETTOS uses rs_lock; lookup comment reworded;
   explicit not-for-stable note.
 - New patch 7: reference across the CM event handler, and a
   destroy-pending re-check in rds_ib_cm_handle_connect().
 - Patch 9: changelog states what a lingering inc keeps alive and
   that it blocks module unload.
 - Changelog corrections throughout (netns teardown paths named as the
   non-rmmod destroyers, stale rds_conn_path_destroy() reference).

[1] https://lore.kernel.org/netdev/20260904070248.160384-1-achender@kernel.org/
[2] https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/

Allison


Allison Henderson (5):
  net/rds: guard every work-requeueing site with rds_destroy_pending()
  net/rds: make rds_destroy_pending() cover single-connection destroy
  net/rds: unlink transport nodes before a possibly deferred connection
    free
  net/rds: pin the connection across RDMA-CM event handling
  net/rds: drop rds_conn_count in favor of t_conn_count

Sharath Srinivasan (4):
  net/rds: split connection destroy into quiesce and kref-governed free
  net/rds: wait for connections to be freed on transport unload
  net/rds: hold connection references in lookup, sockets and c_passive
  net/rds: hold a connection reference from struct rds_incoming

 net/rds/af_rds.c         |  20 ++-
 net/rds/connection.c     | 274 +++++++++++++++++++++++++++++++++++----
 net/rds/ib.c             |  21 ++-
 net/rds/ib_cm.c          |  19 ++-
 net/rds/ib_rdma.c        |  12 +-
 net/rds/ib_recv.c        |   6 +-
 net/rds/ib_send.c        |  18 ++-
 net/rds/loop.c           |  39 ++++--
 net/rds/message.c        |  16 ++-
 net/rds/rdma_transport.c |  16 ++-
 net/rds/rds.h            |  40 +++++-
 net/rds/recv.c           |  21 ++-
 net/rds/send.c           |  56 ++++++--
 net/rds/tcp.c            |  29 ++++-
 net/rds/tcp_listen.c     |  15 ++-
 15 files changed, 527 insertions(+), 75 deletions(-)

-- 
2.25.1

[PATCH net-next v2 1/9] net/rds: guard every work-requeueing site with rds_destroy_pending()

[Allison Henderson]

rds_conn_destroy() cancels the path works and then destroys the
per-path workqueue.  The sites that can re-arm those works are
supposed to test rds_destroy_pending() under rcu_read_lock() first,
paired with the synchronize_rcu() in the destroy path, so that no new
work can be queued once the cancellation has begun.

Five arming sites never got that guard:

  - rds_ib_send_cqe_handler() and rds_ib_send_add_credits() re-arm
    cp_send_w when a send completion or a credit update clears
    RDS_LL_SEND_FULL,
  - rds_ib_recv_refill() re-arms cp_recv_w when the recv ring runs
    low,
  - rds_tcp_accept_one() kicks cp_recv_w on the freshly accepted
    socket, and
  - rds_sendmsg() arms cp_conn_w for a multipath connection whose
    path 0 is not up yet.

The IB completion sites are reachable from soft-irq at any point
before the QP is drained, so a completion landing in the window
between the cancel and destroy_workqueue() in rds_conn_path_destroy()
re-arms a work on a workqueue that is about to be destroyed: with
delay 0 the work is queued directly on the freed workqueue, and with
delay 1 the timer survives destroy_workqueue() unseen and fires
afterwards, queueing from a timer_list that lives in the freed c_path
array.

Wrap all five sites in the same rcu_read_lock() +
rds_destroy_pending() pattern the other arming sites already use.

Fixes: ebeeb1ad9b8a ("rds: tcp: use rds_destroy_pending() to synchronize netns/module teardown and rds connection/workq management")
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/ib_recv.c    |  6 +++++-
 net/rds/ib_send.c    | 18 ++++++++++++++----
 net/rds/send.c       | 11 ++++++++---
 net/rds/tcp_listen.c | 10 +++++++---
 4 files changed, 34 insertions(+), 11 deletions(-)

diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index bd6cb3ffaa57..7d45808544a0 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -458,7 +458,11 @@ void rds_ib_recv_refill(struct rds_connection *conn, int prefill, gfp_t gfp)
 	    (must_wake ||
 	    (can_wait && rds_ib_ring_low(&ic->i_recv_ring)) ||
 	    rds_ib_ring_empty(&ic->i_recv_ring))) {
-		queue_delayed_work(conn->c_path->cp_wq, &conn->c_recv_w, 1);
+		rcu_read_lock();
+		if (!rds_destroy_pending(conn))
+			queue_delayed_work(conn->c_path->cp_wq,
+					   &conn->c_recv_w, 1);
+		rcu_read_unlock();
 	}
 	if (can_wait)
 		cond_resched();
diff --git a/net/rds/ib_send.c b/net/rds/ib_send.c
index d6be95542119..bc411e96ad12 100644
--- a/net/rds/ib_send.c
+++ b/net/rds/ib_send.c
@@ -298,8 +298,13 @@ void rds_ib_send_cqe_handler(struct rds_ib_connection *ic, struct ib_wc *wc)
 	rds_ib_sub_signaled(ic, nr_sig);
 
 	if (test_and_clear_bit(RDS_LL_SEND_FULL, &conn->c_flags) ||
-	    test_bit(0, &conn->c_map_queued))
-		queue_delayed_work(conn->c_path->cp_wq, &conn->c_send_w, 0);
+	    test_bit(0, &conn->c_map_queued)) {
+		rcu_read_lock();
+		if (!rds_destroy_pending(conn))
+			queue_delayed_work(conn->c_path->cp_wq,
+					   &conn->c_send_w, 0);
+		rcu_read_unlock();
+	}
 
 	/* We expect errors as the qp is drained during shutdown */
 	if (wc->status != IB_WC_SUCCESS && rds_conn_up(conn)) {
@@ -420,8 +425,13 @@ void rds_ib_send_add_credits(struct rds_connection *conn, unsigned int credits)
 			test_bit(RDS_LL_SEND_FULL, &conn->c_flags) ? ", ll_send_full" : "");
 
 	atomic_add(IB_SET_SEND_CREDITS(credits), &ic->i_credits);
-	if (test_and_clear_bit(RDS_LL_SEND_FULL, &conn->c_flags))
-		queue_delayed_work(conn->c_path->cp_wq, &conn->c_send_w, 0);
+	if (test_and_clear_bit(RDS_LL_SEND_FULL, &conn->c_flags)) {
+		rcu_read_lock();
+		if (!rds_destroy_pending(conn))
+			queue_delayed_work(conn->c_path->cp_wq,
+					   &conn->c_send_w, 0);
+		rcu_read_unlock();
+	}
 
 	WARN_ON(IB_GET_SEND_CREDITS(credits) >= 16384);
 
diff --git a/net/rds/send.c b/net/rds/send.c
index 1afa981e5c06..32c411d10e3e 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -1378,9 +1378,14 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
 		 * outstanding.
 		 */
 		if (!test_and_set_bit(RDS_RECONNECT_PENDING,
-				      &conn->c_path[0].cp_flags))
-			queue_delayed_work(conn->c_path[0].cp_wq,
-					   &conn->c_path[0].cp_conn_w, 0);
+				      &conn->c_path[0].cp_flags)) {
+			rcu_read_lock();
+			if (!rds_destroy_pending(conn))
+				queue_delayed_work(conn->c_path[0].cp_wq,
+						   &conn->c_path[0].cp_conn_w,
+						   0);
+			rcu_read_unlock();
+		}
 		rds_send_ping(conn, 0);
 	}
 
diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index 13fa60c1985b..8a0c54aced5e 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -316,10 +316,14 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
 	 */
 	if (READ_ONCE(sk->sk_state) == TCP_CLOSE_WAIT ||
 	    READ_ONCE(sk->sk_state) == TCP_LAST_ACK ||
-	    READ_ONCE(sk->sk_state) == TCP_CLOSE)
+	    READ_ONCE(sk->sk_state) == TCP_CLOSE) {
 		rds_conn_path_drop(cp, 0);
-	else
-		queue_delayed_work(cp->cp_wq, &cp->cp_recv_w, 0);
+	} else {
+		rcu_read_lock();
+		if (!rds_destroy_pending(cp->cp_conn))
+			queue_delayed_work(cp->cp_wq, &cp->cp_recv_w, 0);
+		rcu_read_unlock();
+	}
 
 	sock_put(sk);
 
-- 
2.25.1

[PATCH net-next v2 2/9] net/rds: make rds_destroy_pending() cover single-connection destroy

[Allison Henderson]

rds_conn_destroy() cancels the path works and then destroys the
per-path workqueue.  However, nothing currently stops the
work-requeueing sites from queueing new work on the connection while
that happens.  The existing code would suggest that this protection
is supposed to come from rds_destroy_pending(), since all of those
sites guard the queueing with rds_destroy_pending() under
rcu_read_lock() (the last stragglers were converted by the previous
patch), and rds_conn_destroy() already issues a
synchronize_rcu() after unhashing the connection.  But the predicate
only tests for the two global teardown cases (netns destruction via
check_net(), module unload via ->t_unloading).  Because the conn
itself lacks any indication that a destroy is in progress, the
predicate does not cover the destruction of a single connection
outside these two cases.

rds_conn_destroy() is not limited to the global paths:
rds_ib_cm_connect_complete() destroys a single connection whose peer
negotiated an unsupported protocol version.  (The other per-transport
caller, rds_ib_destroy_nodev_conns(), is only reached from
rds_ib_exit(), where ->t_unloading already covers it.)  While that
destroy runs, a concurrent rds_cong_queue_updates() can
still find the connection on the congestion map's m_conn_list (the
conn is only removed from it after the paths are torn down) and call
queue_delayed_work() on a cp_wq that destroy_workqueue() has already
freed.  Additionally, the other requeueing sites can likewise re-arm
works that live in the about-to-be-freed connection unless
rds_destroy_pending() has something to guard it with.

The version-mismatch path used to be covered: commit c90ecbfaf50d2
("rds: Use atomic flag to track connections being destroyed")
introduced the RDS_DESTROY_PENDING cp_flags bit for exactly this, and
after commit ebeeb1ad9b8ad ("rds: tcp: use rds_destroy_pending() to
synchronize netns/module teardown and rds connection/workq management")
it was set right before that rds_conn_destroy() call and
tested via rds_ib_is_unloading().  Commit cdc306a5c9cd3 ("rds: make
v3.1 as compat version") then removed the last set_bit while leaving
the test behind, so the bit has been dead ever since and per-conn
destroy has run unguarded.

Bring the protection back at the connection level: set
conn->c_destroy_in_prog before the unhash + synchronize_rcu() sequence
in rds_conn_destroy() and test it in rds_destroy_pending().  The
existing rcu_read_lock() around every check-and-queue site pairs with
that synchronize_rcu(): once it returns, every new reader observes the
flag and refuses to queue, and anything queued before it is flushed or
cancelled by the existing teardown.  Drop the now-unreferenced
RDS_DESTROY_PENDING bit and its dead test.

In the Oracle UEK kernel the equivalent conn->c_destroy_in_prog flag
is part of the larger connection refcounting rework ("net/rds: Add
krefs to struct rds_connection"), including ("net/rds: Merge uses of
conn->c_destroy_in_prog & RDS_DESTROY_PENDING").  This ports the
missing pieces of the requeue guard, which stand on their own.

Fixes: cdc306a5c9cd3 ("rds: make v3.1 as compat version")
Suggested-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/connection.c |  8 ++++++++
 net/rds/ib.c         |  5 +----
 net/rds/rds.h        | 12 ++++++++++--
 3 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index b6c4beb50eaf..50e1b6bfceea 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -574,6 +574,14 @@ void rds_conn_destroy(struct rds_connection *conn)
 		 "%pI4\n", conn, &conn->c_laddr,
 		 &conn->c_faddr);
 
+	/* Make rds_destroy_pending() true for this conn.  Together with
+	 * the synchronize_rcu() below this stops the work-requeueing
+	 * sites (which all test rds_destroy_pending() under
+	 * rcu_read_lock()) from queueing new work on the path
+	 * workqueues once we start cancelling and destroying them.
+	 */
+	WRITE_ONCE(conn->c_destroy_in_prog, true);
+
 	/* Ensure conn will not be scheduled for reconnect */
 	spin_lock_irq(&rds_conn_lock);
 	hlist_del_init_rcu(&conn->c_hash_node);
diff --git a/net/rds/ib.c b/net/rds/ib.c
index 786f39169bc1..9fe3b9951bd3 100644
--- a/net/rds/ib.c
+++ b/net/rds/ib.c
@@ -525,10 +525,7 @@ static void rds_ib_set_unloading(void)
 
 static bool rds_ib_is_unloading(struct rds_connection *conn)
 {
-	struct rds_conn_path *cp = &conn->c_path[0];
-
-	return (test_bit(RDS_DESTROY_PENDING, &cp->cp_flags) ||
-		atomic_read(&rds_ib_unloading) != 0);
+	return atomic_read(&rds_ib_unloading) != 0;
 }
 
 void rds_ib_exit(void)
diff --git a/net/rds/rds.h b/net/rds/rds.h
index 2db49573dacd..50b08c28ab86 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -89,7 +89,6 @@ enum {
 #define RDS_RECONNECT_PENDING	1
 #define RDS_IN_XMIT		2
 #define RDS_RECV_REFILL		3
-#define	RDS_DESTROY_PENDING	4
 
 /* Max number of multipaths per RDS connection. Must be a power of 2 */
 #define	RDS_MPATH_WORKERS	8
@@ -148,6 +147,14 @@ struct rds_connection {
 				c_pad_to_32:29;
 	int			c_npaths;
 	bool			c_with_sport_idx;
+	/* Set once, by rds_conn_destroy(), before it cancels the path
+	 * works; read through rds_destroy_pending().  A site that arms
+	 * a path work must test the predicate and queue the work inside
+	 * one rcu_read_lock() section: the synchronize_rcu() that
+	 * follows the store is what keeps a queue issued after the
+	 * cancellation from landing on a destroyed workqueue.
+	 */
+	bool			c_destroy_in_prog;
 	struct rds_connection	*c_passive;
 	struct rds_transport	*c_trans;
 
@@ -994,7 +1001,8 @@ void __rds_put_mr_final(struct kref *kref);
 
 static inline bool rds_destroy_pending(struct rds_connection *conn)
 {
-	return !check_net(rds_conn_net(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));
 }
 
-- 
2.25.1

[PATCH net-next v2 3/9] net/rds: split connection destroy into quiesce and kref-governed free

[Allison Henderson]

From: Sharath Srinivasan <sharath.srinivasan@oracle.com>

rds_conn_destroy() tears down the transport state and immediately
frees the connection, along with its paths and its workqueues.  This
relies on the assumption (documented in the rds_conn_destroy()
comments) that "no one else is referencing the connection", which "we
can only ensure ... in the rmmod path".  However, the callers stopped
honoring that long ago.  Today, connections are also destroyed on
network namespace teardown (rds_tcp_kill_sock() and
rds_loop_kill_conns()) and, one at a time, when a peer negotiates an
unsupported protocol version (rds_ib_cm_connect_complete()).

This leaves loose ends since references to these destroyed
connections still exist.  Sockets cache their connections in rs_conn,
and congestion updates will walk the maps' m_conn_list.  The CM
callbacks and workers may also still hold the pointer.

Prepare to close those holes by making the connection refcounted:

- kref_init() the connection in __rds_conn_create(); the initial
  reference belongs to whoever is responsible for destroying the
  connection.
- rds_conn_destroy() still quiesces synchronously exactly as before
  (workers cancelled, paths dropped and shut down, queued messages
  purged, congestion list removal), but the frees - the transport's
  conn_free, the path workqueues, the c_path array and the connection
  slab object - move to rds_conn_destroy_fini(), which runs when the
  last reference is dropped via rds_conn_put().
- Export rds_conn_get()/rds_conn_put() for the reference holders
  introduced in the following patches.

With no additional reference holders yet, this only sets up the
refcounting framework and is functionally equivalent to the current
code (the initial reference is the only one).  Subsequent patches will
take references at the places that today rely on bare pointers.

Based on the Oracle UEK commit "net/rds: Add krefs to struct
rds_connection".

Signed-off-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>
[achender: substantial reimplementation for net-next: UEK's
 rds_conn_destroy_init()/_fini() split redone against upstream's
 rds_conn_destroy()/rds_conn_path_destroy() (no heartbeat/reap/trace
 infrastructure, no rds_net, single conn hash); destroy keeps its
 one-call external interface; holder coverage split out into follow-up
 patches; rewrite commit message]
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/connection.c | 96 +++++++++++++++++++++++++++++++++++---------
 net/rds/rds.h        |  8 ++++
 2 files changed, 86 insertions(+), 18 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index 50e1b6bfceea..cc4e74b731df 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -215,6 +215,7 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 		goto out;
 	}
 
+	kref_init(&conn->c_refcount);
 	INIT_HLIST_NODE(&conn->c_hash_node);
 	conn->c_laddr = *laddr;
 	conn->c_isv6 = !ipv6_addr_v4mapped(laddr);
@@ -466,9 +467,10 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
 			 * Quiesce the reconnect timer before bailing
 			 * out, though.  When a pending destroy did
 			 * suppress the queue, no later pass runs, and
-			 * rds_conn_path_destroy() is about to flush
-			 * cp_down_w and free the path: it must not
-			 * find cp_conn_w still armed.  A successor
+			 * rds_conn_path_quiesce() is about to flush
+			 * cp_down_w, ahead of the path's deferred
+			 * free: it must not find cp_conn_w still
+			 * armed.  A successor
 			 * pass, when there is one, re-arms the
 			 * reconnect from its own tail.
 			 */
@@ -515,10 +517,12 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
 		conn->c_trans->conn_slots_available(conn, false);
 }
 
-/* destroy a single rds_conn_path. rds_conn_destroy() iterates over
- * all paths using rds_conn_path_destroy()
+/* quiesce a single rds_conn_path: shut it down and tear down any
+ * queued messages.  rds_conn_destroy() iterates over all paths using
+ * rds_conn_path_quiesce(); the transport state and the workqueue are
+ * freed later, from rds_conn_path_free().
  */
-static void rds_conn_path_destroy(struct rds_conn_path *cp)
+static void rds_conn_path_quiesce(struct rds_conn_path *cp)
 {
 	struct rds_message *rm, *rtmp;
 
@@ -547,6 +551,16 @@ static void rds_conn_path_destroy(struct rds_conn_path *cp)
 	WARN_ON(delayed_work_pending(&cp->cp_recv_w));
 	WARN_ON(delayed_work_pending(&cp->cp_conn_w));
 	WARN_ON(work_pending(&cp->cp_down_w));
+}
+
+/* free a quiesced rds_conn_path's transport state and workqueue; runs
+ * from rds_conn_destroy_fini() once the last connection reference is
+ * dropped.
+ */
+static void rds_conn_path_free(struct rds_conn_path *cp)
+{
+	if (!cp->cp_transport_data)
+		return;
 
 	if (cp->cp_wq != rds_wq) {
 		destroy_workqueue(cp->cp_wq);
@@ -556,16 +570,52 @@ static void rds_conn_path_destroy(struct rds_conn_path *cp)
 	cp->cp_conn->c_trans->conn_free(cp->cp_transport_data);
 }
 
+/* Free a connection.  This runs from rds_conn_put() when the last
+ * reference is dropped, after rds_conn_destroy() has quiesced the
+ * connection and dropped the initial reference.
+ */
+static void rds_conn_destroy_fini(struct kref *kref)
+{
+	struct rds_connection *conn = container_of(kref, struct rds_connection,
+						   c_refcount);
+	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
+	unsigned long flags;
+	int i;
+
+	for (i = 0; i < npaths; i++)
+		rds_conn_path_free(&conn->c_path[i]);
+
+	kfree(conn->c_path);
+	kmem_cache_free(rds_conn_slab, conn);
+
+	spin_lock_irqsave(&rds_conn_lock, flags);
+	rds_conn_count--;
+	spin_unlock_irqrestore(&rds_conn_lock, flags);
+}
+
+void rds_conn_get(struct rds_connection *conn)
+{
+	kref_get(&conn->c_refcount);
+}
+EXPORT_SYMBOL_GPL(rds_conn_get);
+
+void rds_conn_put(struct rds_connection *conn)
+{
+	kref_put(&conn->c_refcount, rds_conn_destroy_fini);
+}
+EXPORT_SYMBOL_GPL(rds_conn_put);
+
 /*
  * Stop and free a connection.
  *
- * This can only be used in very limited circumstances.  It assumes that once
- * the conn has been shutdown that no one else is referencing the connection.
- * We can only ensure this in the rmmod path in the current code.
+ * Quiesces the connection synchronously (workers cancelled, transport
+ * connections shut down, queued messages dropped) and drops the
+ * initial reference.  The memory - including the transport's
+ * per-connection state and the path workqueues - is freed once the
+ * last rds_conn_put() runs, which may be after this returns.
  */
 void rds_conn_destroy(struct rds_connection *conn)
 {
-	unsigned long flags;
 	int i;
 	struct rds_conn_path *cp;
 	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
@@ -579,11 +629,23 @@ void rds_conn_destroy(struct rds_connection *conn)
 	 * sites (which all test rds_destroy_pending() under
 	 * rcu_read_lock()) from queueing new work on the path
 	 * workqueues once we start cancelling and destroying them.
+	 *
+	 * Now that the transport state stays discoverable (e.g. on the
+	 * transports' connection lists) until the final rds_conn_put(),
+	 * a conn can be handed to rds_conn_destroy() more than once -
+	 * e.g. dropped for a protocol version mismatch and then found
+	 * again at module unload.  Only the first caller proceeds; the
+	 * unhash also happens under rds_conn_lock, so a looked-up conn
+	 * can never be quiesced twice.
 	 */
+	spin_lock_irq(&rds_conn_lock);
+	if (conn->c_destroy_in_prog) {
+		spin_unlock_irq(&rds_conn_lock);
+		return;
+	}
 	WRITE_ONCE(conn->c_destroy_in_prog, true);
 
 	/* Ensure conn will not be scheduled for reconnect */
-	spin_lock_irq(&rds_conn_lock);
 	hlist_del_init_rcu(&conn->c_hash_node);
 	spin_unlock_irq(&rds_conn_lock);
 	synchronize_rcu();
@@ -591,7 +653,7 @@ void rds_conn_destroy(struct rds_connection *conn)
 	/* shut the connection down */
 	for (i = 0; i < npaths; i++) {
 		cp = &conn->c_path[i];
-		rds_conn_path_destroy(cp);
+		rds_conn_path_quiesce(cp);
 		BUG_ON(!list_empty(&cp->cp_retrans));
 	}
 
@@ -602,12 +664,10 @@ void rds_conn_destroy(struct rds_connection *conn)
 	 */
 	rds_cong_remove_conn(conn);
 
-	kfree(conn->c_path);
-	kmem_cache_free(rds_conn_slab, conn);
-
-	spin_lock_irqsave(&rds_conn_lock, flags);
-	rds_conn_count--;
-	spin_unlock_irqrestore(&rds_conn_lock, flags);
+	/* drop the initial reference; the connection is freed from
+	 * rds_conn_destroy_fini() once every holder has dropped theirs
+	 */
+	rds_conn_put(conn);
 }
 EXPORT_SYMBOL_GPL(rds_conn_destroy);
 
diff --git a/net/rds/rds.h b/net/rds/rds.h
index 50b08c28ab86..49629108c22a 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -137,6 +137,12 @@ struct rds_conn_path {
 /* One rds_connection per RDS address pair */
 struct rds_connection {
 	struct hlist_node	c_hash_node;
+	/* Free of the connection memory (not the teardown of its
+	 * transport state - that stays synchronous in
+	 * rds_conn_destroy()) is deferred until the last reference is
+	 * dropped via rds_conn_put().
+	 */
+	struct kref		c_refcount;
 	struct in6_addr		c_laddr;
 	struct in6_addr		c_faddr;
 	int			c_dev_if; /* ifindex used for this conn */
@@ -826,6 +832,8 @@ struct rds_connection *rds_conn_create_outgoing(struct net *net,
 						u8 tos, gfp_t gfp, int dev_if);
 void rds_conn_shutdown(struct rds_conn_path *cpath);
 void rds_conn_destroy(struct rds_connection *conn);
+void rds_conn_get(struct rds_connection *conn);
+void rds_conn_put(struct rds_connection *conn);
 void rds_conn_drop(struct rds_connection *conn);
 void rds_conn_path_drop(struct rds_conn_path *cpath, bool destroy);
 void rds_conn_connect_if_down(struct rds_connection *conn);
-- 
2.25.1

[PATCH net-next v2 4/9] net/rds: wait for connections to be freed on transport unload

[Allison Henderson]

From: Sharath Srinivasan <sharath.srinivasan@oracle.com>

Since connection free became asynchronous, rds_conn_destroy() only
quiesces the connection; the actual free - including the transport's
conn_free, which lives in the transport module - runs when the last
reference is dropped.  The transports' exit paths destroy all of
their connections and then proceed to unload, so a free that is still
pending (a racing lookup-style holder, or simply the destroyer's own
put not yet run when destroy was invoked from another context earlier)
would execute transport module code after that module's text is gone.

Count each transport's live connections in t_conn_count (incremented
when a connection is published in __rds_conn_create(), decremented as
the last step of rds_conn_destroy_fini()) and make the transport exit
paths - rds_ib_exit(), rds_tcp_exit() and rds_loop_exit() - wait for
the count to drop to zero after destroying their connections.

A bound socket holds a module reference on its own transport
(rds_trans_get_preferred()), but that does not bound the wait: once
the following patches make incs hold a connection reference, an
unread datagram pins its connection for as long as the application
leaves it on the receive queue, and rds_find_bound() does not filter
on transport, so that socket may well belong to a different transport
than the connection and pin nothing that stops this unload.
Proceeding after a timeout would turn a leak into a use-after-free in
this module's text, so the wait is unbounded: it polls the count,
warns every ten seconds naming the transport and the number of
connections outstanding, and returns only when the count reaches
zero.  rmmod therefore blocks while data is queued and unread, which
is the historical RDS contract - teardown does not discard queued
data.

rds_ib_exit() has one more wrinkle.  rds_ib_dev_shutdown() only drops
the connections still attached to a device, and each of them moves
itself to ib_nodev_conns from its own shutdown work, asynchronously.
A connection that has not migrated by the time
rds_ib_destroy_nodev_conns() sweeps the list would never be
destroyed, and would hold the count up for good.  So the wait takes a
resweep callback, which rds_ib_exit() points at
rds_ib_destroy_nodev_conns(): late arrivals get destroyed on the next
poll instead of being waited on.

In rds_ib_exit(), tearing down the last connection can also drop the
final reference on a device, which defers rds_ib_dev_free() - again
this module's text - to rds_wq.  Flush the workqueue once after the
connections are gone; rds_ib_dev_free() queues nothing further on
rds_wq, so a single pass drains it.

Based on the Oracle UEK commits "net/rds: wait_event_timeout
until zero connections during rmmod" and "net/rds:
Each RDS transport should keep its own connection count".

Signed-off-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>
[achender: reimplementation for net-next: t_conn_count did not exist
 upstream and is introduced here; single global waitqueue instead of
 per-transport (the loop transport never goes through
 rds_trans_register()); unbounded wait with a periodic warning in
 place of UEK's wait_event_timeout() + WARN_ON(), plus the resweep for
 IB's asynchronous device detach; also cover rds_loop_exit(); rewrite
 commit message]
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/connection.c | 49 ++++++++++++++++++++++++++++++++++++++++++++
 net/rds/ib.c         | 17 +++++++++++++++
 net/rds/loop.c       |  2 ++
 net/rds/rds.h        | 13 ++++++++++++
 net/rds/tcp.c        |  1 +
 5 files changed, 82 insertions(+)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index cc4e74b731df..5848dd295911 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -48,6 +48,8 @@
 /* converting this to RCU is a chore for another day.. */
 static DEFINE_SPINLOCK(rds_conn_lock);
 static unsigned long rds_conn_count;
+/* woken whenever a transport's t_conn_count drops to zero */
+static DECLARE_WAIT_QUEUE_HEAD(rds_conn_freed_waitq);
 static struct hlist_head rds_conn_hash[RDS_CONNECTION_HASH_ENTRIES];
 static struct kmem_cache *rds_conn_slab;
 
@@ -325,6 +327,7 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 			parent->c_passive = conn;
 			rds_cong_add_conn(conn);
 			rds_conn_count++;
+			atomic_inc(&conn->c_trans->t_conn_count);
 		}
 	} else {
 		/* Creating normal conn */
@@ -354,6 +357,7 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 			hlist_add_head_rcu(&conn->c_hash_node, head);
 			rds_cong_add_conn(conn);
 			rds_conn_count++;
+			atomic_inc(&conn->c_trans->t_conn_count);
 		}
 	}
 	spin_unlock_irqrestore(&rds_conn_lock, flags);
@@ -579,6 +583,7 @@ static void rds_conn_destroy_fini(struct kref *kref)
 	struct rds_connection *conn = container_of(kref, struct rds_connection,
 						   c_refcount);
 	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
+	struct rds_transport *trans = conn->c_trans;
 	unsigned long flags;
 	int i;
 
@@ -591,7 +596,51 @@ static void rds_conn_destroy_fini(struct kref *kref)
 	spin_lock_irqsave(&rds_conn_lock, flags);
 	rds_conn_count--;
 	spin_unlock_irqrestore(&rds_conn_lock, flags);
+
+	/* only after everything the transport module owns has been
+	 * freed above may its unload proceed
+	 */
+	if (!atomic_dec_return(&trans->t_conn_count))
+		wake_up_all(&rds_conn_freed_waitq);
+}
+
+/* Wait for all of @trans's connections to be freed; the free runs
+ * asynchronously once rds_conn_destroy() has quiesced a connection.
+ * Called on transport module unload, after the transport has destroyed
+ * all of its connections.  A connection reference can be held for an
+ * application-controlled time - an unread datagram pins the inc that
+ * carries it, and thus the connection - so the wait is unbounded: the
+ * frees that run after unload call into this module's text (conn_free,
+ * inc_free) and free into its slabs, so proceeding while any remain
+ * would be a use-after-free, not a leak.  Warn periodically so a stuck
+ * count is diagnosable, but never stop waiting.  This matches the
+ * historical RDS contract that teardown does not discard queued data.
+ */
+void rds_conn_wait_conns_freed(struct rds_transport *trans,
+			       void (*resweep)(void))
+{
+	unsigned long warn_interval =
+			msecs_to_jiffies(RDS_CONN_FREE_WARN_INTERVAL_MS);
+	unsigned long warn_at = jiffies + warn_interval;
+
+	while (!wait_event_timeout(rds_conn_freed_waitq,
+				   !atomic_read(&trans->t_conn_count),
+				   msecs_to_jiffies(RDS_CONN_FREE_POLL_MS))) {
+		/* A transport whose teardown is asynchronous (IB moves a
+		 * connection off its device from the shutdown work) gives
+		 * us a resweep to destroy what has arrived since.
+		 */
+		if (resweep)
+			resweep();
+		if (time_after_eq(jiffies, warn_at)) {
+			pr_warn("RDS/%s: still waiting for %d connection(s) to be freed before unload\n",
+				trans->t_name,
+				atomic_read(&trans->t_conn_count));
+			warn_at = jiffies + warn_interval;
+		}
+	}
 }
+EXPORT_SYMBOL_GPL(rds_conn_wait_conns_freed);
 
 void rds_conn_get(struct rds_connection *conn)
 {
diff --git a/net/rds/ib.c b/net/rds/ib.c
index 9fe3b9951bd3..3fc2de9d19d5 100644
--- a/net/rds/ib.c
+++ b/net/rds/ib.c
@@ -537,7 +537,24 @@ void rds_ib_exit(void)
 	rds_info_deregister_func(RDS6_INFO_IB_CONNECTIONS, rds6_ib_ic_info);
 #endif
 	rds_ib_unregister_client();
+
+	/* rds_ib_dev_shutdown() only dropped the connections still
+	 * attached to a device; each moves itself to ib_nodev_conns
+	 * from its shutdown work.  Destroy what is there now and keep
+	 * sweeping the list while the wait sees connections outstanding,
+	 * so a late arrival is destroyed rather than waited on forever.
+	 */
 	rds_ib_destroy_nodev_conns();
+	rds_conn_wait_conns_freed(&rds_ib_transport,
+				  rds_ib_destroy_nodev_conns);
+
+	/* Tearing down the last connection may have dropped the final
+	 * reference on a device, deferring rds_ib_dev_free() to rds_wq.
+	 * Drain it before the module goes away; it queues nothing
+	 * further on rds_wq.
+	 */
+	flush_workqueue(rds_wq);
+
 	rds_ib_sysctl_exit();
 	rds_ib_recv_exit();
 	rds_trans_unregister(&rds_ib_transport);
diff --git a/net/rds/loop.c b/net/rds/loop.c
index e6b0750bbeda..fd774f8080d0 100644
--- a/net/rds/loop.c
+++ b/net/rds/loop.c
@@ -195,6 +195,8 @@ void rds_loop_exit(void)
 		WARN_ON(lc->conn->c_passive);
 		rds_conn_destroy(lc->conn);
 	}
+
+	rds_conn_wait_conns_freed(&rds_loop_transport, NULL);
 }
 
 static void rds_loop_kill_conns(struct net *net)
diff --git a/net/rds/rds.h b/net/rds/rds.h
index 49629108c22a..8a969444e698 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -557,6 +557,12 @@ struct rds_transport {
 	unsigned int		t_prefer_loopback:1,
 				t_mp_capable:1;
 	unsigned int		t_type;
+	/* Connections of this transport not yet freed; freeing runs
+	 * asynchronously once rds_conn_destroy() has quiesced a
+	 * connection, so transport module unload has to wait for this
+	 * to reach zero (rds_conn_wait_conns_freed()).
+	 */
+	atomic_t		t_conn_count;
 
 	int (*laddr_check)(struct net *net, const struct in6_addr *addr,
 			   __u32 scope_id);
@@ -834,6 +840,13 @@ void rds_conn_shutdown(struct rds_conn_path *cpath);
 void rds_conn_destroy(struct rds_connection *conn);
 void rds_conn_get(struct rds_connection *conn);
 void rds_conn_put(struct rds_connection *conn);
+/* transport unload waits for its connections to be freed, polling at
+ * the first interval and warning at the second
+ */
+#define RDS_CONN_FREE_POLL_MS		100
+#define RDS_CONN_FREE_WARN_INTERVAL_MS	10000
+void rds_conn_wait_conns_freed(struct rds_transport *trans,
+			       void (*resweep)(void));
 void rds_conn_drop(struct rds_connection *conn);
 void rds_conn_path_drop(struct rds_conn_path *cpath, bool destroy);
 void rds_conn_connect_if_down(struct rds_connection *conn);
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 774a71f88d37..826e4629e4ee 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -805,6 +805,7 @@ static void rds_tcp_exit(void)
 #endif
 	unregister_pernet_device(&rds_tcp_net_ops);
 	rds_tcp_destroy_conns();
+	rds_conn_wait_conns_freed(&rds_tcp_transport, NULL);
 	rds_trans_unregister(&rds_tcp_transport);
 	rds_tcp_recv_exit();
 	kmem_cache_destroy(rds_tcp_conn_slab);
-- 
2.25.1

[PATCH net-next v2 5/9] net/rds: unlink transport nodes before a possibly deferred connection free

[Allison Henderson]

The transport teardown helpers - rds_tcp_destroy_conns(),
rds_tcp_kill_sock(), rds_ib_destroy_nodev_conns(), rds_loop_exit() and
rds_loop_kill_conns() - gather the per-connection transport nodes onto
a list head on their own stack and call rds_conn_destroy() for each.
The node is unlinked much later, by the transport's conn_free():
rds_tcp_conn_free() and rds_loop_conn_free() list_del() it, and
rds_ib_conn_free() does so unconditionally.

That was fine while rds_conn_destroy() freed the connection before it
returned.  Once the free is governed by the connection's reference
count, a holder that outlives the teardown loop - a socket's cached
rs_conn, an inc parked on a receive queue - defers conn_free() until
after the helper has returned, and the list_del() then writes the
neighbours' pointers into a stack frame that no longer exists.

Unlink each node under the transport lock right before its
rds_conn_destroy() instead, so that nothing is left on the stack list
for a later free to touch.  TCP marks the node detached, as
rds_tcp_kill_sock() already does for the secondary paths of a
multipath connection; IB and loopback use list_del_init() and have
their conn_free() skip a node that is already empty.  The tmp_list
gathering itself is unchanged: it still exists so that
rds_conn_destroy() is not called with the transport lock held.

Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/ib_cm.c   |  4 +++-
 net/rds/ib_rdma.c | 12 +++++++++++-
 net/rds/loop.c    | 37 +++++++++++++++++++++++++++----------
 net/rds/tcp.c     | 28 ++++++++++++++++++++++++----
 4 files changed, 65 insertions(+), 16 deletions(-)

diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 4feb0edc360c..de5759c50b89 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -1282,7 +1282,9 @@ void rds_ib_conn_free(void *arg)
 	lock_ptr = ic->rds_ibdev ? &ic->rds_ibdev->spinlock : &ib_nodev_conns_lock;
 
 	spin_lock_irq(lock_ptr);
-	list_del(&ic->ib_node);
+	/* already unlinked if a transport teardown gathered us first */
+	if (!list_empty(&ic->ib_node))
+		list_del(&ic->ib_node);
 	spin_unlock_irq(lock_ptr);
 
 	rds_ib_recv_free_caches(ic);
diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
index db7e92e7bd29..b30f2a371587 100644
--- a/net/rds/ib_rdma.c
+++ b/net/rds/ib_rdma.c
@@ -168,8 +168,18 @@ void rds_ib_destroy_nodev_conns(void)
 	list_splice(&ib_nodev_conns, &tmp_list);
 	spin_unlock_irq(&ib_nodev_conns_lock);
 
-	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
+	/* rds_conn_destroy() can return before the connection is freed,
+	 * and it is the free - rds_ib_conn_free() - that unlinks ib_node.
+	 * tmp_list lives on this stack frame, so unlink each node before
+	 * its destroy; the free then finds it empty and leaves it alone.
+	 */
+	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node) {
+		spin_lock_irq(&ib_nodev_conns_lock);
+		list_del_init(&ic->ib_node);
+		spin_unlock_irq(&ib_nodev_conns_lock);
+
 		rds_conn_destroy(ic->conn);
+	}
 }
 
 void rds_ib_get_mr_info(struct rds_ib_device *rds_ibdev, struct rds_info_rdma_connection *iinfo)
diff --git a/net/rds/loop.c b/net/rds/loop.c
index fd774f8080d0..93be7832b11d 100644
--- a/net/rds/loop.c
+++ b/net/rds/loop.c
@@ -156,6 +156,28 @@ static int rds_loop_conn_alloc(struct rds_connection *conn, gfp_t gfp)
 	return 0;
 }
 
+/* Destroy the connections whose nodes were gathered on @tmp_list.
+ *
+ * rds_conn_destroy() can return before the connection is freed, and
+ * it is the free - rds_loop_conn_free() - that unlinks loop_node.
+ * @tmp_list lives on the caller's stack, so unlink each node before
+ * its destroy; the free then finds it empty and leaves it alone.
+ */
+static void rds_loop_destroy_gathered_conns(struct list_head *tmp_list)
+{
+	struct rds_loop_connection *lc, *_lc;
+
+	list_for_each_entry_safe(lc, _lc, tmp_list, loop_node) {
+		WARN_ON(lc->conn->c_passive);
+
+		spin_lock_irq(&loop_conns_lock);
+		list_del_init(&lc->loop_node);
+		spin_unlock_irq(&loop_conns_lock);
+
+		rds_conn_destroy(lc->conn);
+	}
+}
+
 static void rds_loop_conn_free(void *arg)
 {
 	struct rds_loop_connection *lc = arg;
@@ -163,7 +185,9 @@ static void rds_loop_conn_free(void *arg)
 
 	rdsdebug("lc %p\n", lc);
 	spin_lock_irqsave(&loop_conns_lock, flags);
-	list_del(&lc->loop_node);
+	/* already unlinked if a transport teardown gathered us first */
+	if (!list_empty(&lc->loop_node))
+		list_del(&lc->loop_node);
 	spin_unlock_irqrestore(&loop_conns_lock, flags);
 	kfree(lc);
 }
@@ -180,7 +204,6 @@ static void rds_loop_conn_path_shutdown(struct rds_conn_path *cp)
 
 void rds_loop_exit(void)
 {
-	struct rds_loop_connection *lc, *_lc;
 	LIST_HEAD(tmp_list);
 
 	rds_loop_set_unloading();
@@ -191,10 +214,7 @@ void rds_loop_exit(void)
 	INIT_LIST_HEAD(&loop_conns);
 	spin_unlock_irq(&loop_conns_lock);
 
-	list_for_each_entry_safe(lc, _lc, &tmp_list, loop_node) {
-		WARN_ON(lc->conn->c_passive);
-		rds_conn_destroy(lc->conn);
-	}
+	rds_loop_destroy_gathered_conns(&tmp_list);
 
 	rds_conn_wait_conns_freed(&rds_loop_transport, NULL);
 }
@@ -214,10 +234,7 @@ static void rds_loop_kill_conns(struct net *net)
 	}
 	spin_unlock_irq(&loop_conns_lock);
 
-	list_for_each_entry_safe(lc, _lc, &tmp_list, loop_node) {
-		WARN_ON(lc->conn->c_passive);
-		rds_conn_destroy(lc->conn);
-	}
+	rds_loop_destroy_gathered_conns(&tmp_list);
 }
 
 static void __net_exit rds_loop_exit_net(struct net *net)
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 826e4629e4ee..a71d6a4f0939 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -502,6 +502,28 @@ static bool rds_tcp_is_unloading(struct rds_connection *conn)
 	return atomic_read(&rds_tcp_unloading) != 0;
 }
 
+/* Destroy the connections whose nodes were gathered on @tmp_list.
+ *
+ * rds_conn_destroy() can return before the connection is freed, and
+ * it is the free - rds_tcp_conn_free() - that unlinks t_tcp_node.
+ * Since @tmp_list lives on the caller's stack, unlink each node here
+ * and mark it detached before its destroy, so that a free that runs
+ * after the caller has returned does not write into a dead frame.
+ */
+static void rds_tcp_destroy_gathered_conns(struct list_head *tmp_list)
+{
+	struct rds_tcp_connection *tc, *_tc;
+
+	list_for_each_entry_safe(tc, _tc, tmp_list, t_tcp_node) {
+		spin_lock_irq(&rds_tcp_conn_lock);
+		list_del_init(&tc->t_tcp_node);
+		tc->t_tcp_node_detached = true;
+		spin_unlock_irq(&rds_tcp_conn_lock);
+
+		rds_conn_destroy(tc->t_cpath->cp_conn);
+	}
+}
+
 static void rds_tcp_destroy_conns(void)
 {
 	struct rds_tcp_connection *tc, *_tc;
@@ -515,8 +537,7 @@ static void rds_tcp_destroy_conns(void)
 	}
 	spin_unlock_irq(&rds_tcp_conn_lock);
 
-	list_for_each_entry_safe(tc, _tc, &tmp_list, t_tcp_node)
-		rds_conn_destroy(tc->t_cpath->cp_conn);
+	rds_tcp_destroy_gathered_conns(&tmp_list);
 }
 
 static void rds_tcp_exit(void);
@@ -698,8 +719,7 @@ static void rds_tcp_kill_sock(struct net *net)
 		}
 	}
 	spin_unlock_irq(&rds_tcp_conn_lock);
-	list_for_each_entry_safe(tc, _tc, &tmp_list, t_tcp_node)
-		rds_conn_destroy(tc->t_cpath->cp_conn);
+	rds_tcp_destroy_gathered_conns(&tmp_list);
 }
 
 static void __net_exit rds_tcp_exit_net(struct net *net)
-- 
2.25.1

[PATCH net-next v2 6/9] net/rds: hold connection references in lookup, sockets and c_passive

[Allison Henderson]

From: Sharath Srinivasan <sharath.srinivasan@oracle.com>

Hand out real references everywhere a struct rds_connection pointer
previously escaped bare:

- rds_conn_lookup() takes a reference on the connection it returns
  (kref_get_unless_zero(), so that it only ever hands out a live
  reference), and
  __rds_conn_create() returns the connection with a reference held
  for the caller on every path: lookup hit, fresh creation, lost
  creation race, and the passive-loopback lookup, which now also
  holds the parent while it dereferences parent->c_passive.

- The rs->rs_conn sendmsg cache owns a reference, which is dropped
  when the cache is replaced or the socket is released.
  rds_sendmsg() itself holds a reference for the duration of the
  call, during which reads and updates of rs_conn are serialized by
  rs_lock.  So neither a concurrent rds_conn_destroy() nor another
  sender replacing the cache can free the connection under a sender.
  The connection may still be destroyed while a send is in flight -
  when its device is removed or its netns is torn down - but it is
  only quiesced; the free is held off by the sender's reference.  A
  cached connection whose destruction has begun is no longer reused.
  Instead, sendmsg drops it and looks up or creates a live one, so a
  socket cannot get stuck returning -EAGAIN forever against a
  quiesced connection.  rds_ioctl(SIOCRDSSETTOS) tests rs_conn under
  the same lock; it used the unrelated global rds_sock_lock before,
  which also let a racing sendmsg cache a connection whose c_tos
  disagrees with the rs_tos being set.

- parent->c_passive owns a reference, dropped when the parent is
  destroyed.  The pointer is read under rcu_read_lock() and written
  under rds_conn_lock, so it is RCU-annotated and accessed through
  rcu_dereference()/rcu_assign_pointer().  A passive connection whose
  own destroy has begun is neither handed out nor left dangling in
  the parent: __rds_conn_create() refuses it, and the child's destroy
  clears the parent's pointer and drops that reference itself, so a
  quiesced passive conn cannot be revived by a later connect request.

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:

  BUG: KCSAN: data-race in rds_sendmsg / rds_sendmsg

  write to 0xffff888101dec818 of 8 bytes by task 30904 on cpu 0:
   rds_sendmsg+0xc1f/0x1580 net/rds/send.c:1332

  write to 0xffff888101dec818 of 8 bytes by task 30905 on cpu 1:
   rds_sendmsg+0xc1f/0x1580 net/rds/send.c:1332

  value changed: 0x0000000000000000 -> 0xffff88811b61faf0

cm_id->context still carries no reference of its own after this
patch; the following patch pins the connection for the duration of
the CM event handler.

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.

Based on the Oracle UEK commits "net/rds: Add krefs to
struct rds_connection" and "net/rds: rds_sendmsg must
use rs_conn only when not being destroyed".

Reported-by: syzbot+879c1877016972360186@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=879c1877016972360186
Signed-off-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>
[achender: substantial reimplementation for net-next: upstream has no
 conn reaper, per-conn workers hold no references (destroy cancels
 them synchronously before the final put), and the sendmsg cache is
 serialized with rs_lock instead of UEK's socket flag; rewrite commit
 message]
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/af_rds.c     |  20 +++++--
 net/rds/connection.c | 125 +++++++++++++++++++++++++++++++++++++++++--
 net/rds/ib_cm.c      |   8 ++-
 net/rds/loop.c       |   2 +-
 net/rds/rds.h        |   2 +-
 net/rds/send.c       |  42 +++++++++++++--
 net/rds/tcp_listen.c |   5 +-
 7 files changed, 187 insertions(+), 17 deletions(-)

diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
index d5defe9172e3..63baac8aeb2d 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;
+	}
+
 	spin_lock_bh(&rds_sock_lock);
 	list_del_init(&rs->rs_item);
 	spin_unlock_bh(&rds_sock_lock);
@@ -255,6 +263,7 @@ static int rds_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 {
 	struct rds_sock *rs = rds_sk_to_rs(sock->sk);
 	rds_tos_t utos, tos = 0;
+	unsigned long flags;
 
 	switch (cmd) {
 	case SIOCRDSSETTOS:
@@ -267,13 +276,18 @@ 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;
 	case SIOCRDSGETTOS:
 		spin_lock_bh(&rds_sock_lock);
diff --git a/net/rds/connection.c b/net/rds/connection.c
index 5848dd295911..1ec141d6faa3 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -81,7 +81,18 @@ static struct hlist_head *rds_conn_bucket(const struct in6_addr *laddr,
 		var |= RDS_INFO_CONNECTION_FLAG_##suffix;	\
 } while (0)
 
-/* rcu read lock must be held or the connection spinlock */
+/* rcu read lock must be held or the connection spinlock.
+ * On success a reference is taken on the returned connection; the
+ * caller must drop it with rds_conn_put().
+ */
+/* c_passive is written under rds_conn_lock and read under RCU */
+static struct rds_connection *
+rds_conn_passive_locked(struct rds_connection *conn)
+{
+	return rcu_dereference_protected(conn->c_passive,
+					 lockdep_is_held(&rds_conn_lock));
+}
+
 static struct rds_connection *rds_conn_lookup(struct net *net,
 					      struct hlist_head *head,
 					      const struct in6_addr *laddr,
@@ -98,6 +109,17 @@ static struct rds_connection *rds_conn_lookup(struct net *net,
 		    conn->c_tos == tos &&
 		    net == rds_conn_net(conn) &&
 		    conn->c_dev_if == dev_if) {
+			/* Only ever hand out a live reference.
+			 * rds_conn_destroy() unhashes under
+			 * rds_conn_lock and waits a grace period
+			 * before dropping the initial reference, so
+			 * an entry this traversal reaches still holds
+			 * at least that one; the conditional get
+			 * documents the contract rather than
+			 * papering over a zero-refcount entry.
+			 */
+			if (!kref_get_unless_zero(&conn->c_refcount))
+				continue;
 			ret = conn;
 			break;
 		}
@@ -199,7 +221,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);
 	}
 	rcu_read_unlock();
 	if (conn)
@@ -318,13 +353,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.
+			 */
+			trans->conn_free(conn->c_path[0].cp_transport_data);
+			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);
 			trans->conn_free(conn->c_path[0].cp_transport_data);
 			free_cp = conn->c_path;
 			kmem_cache_free(rds_conn_slab, conn);
-			conn = parent->c_passive;
+			if (READ_ONCE(passive->c_destroy_in_prog)) {
+				/* 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);
@@ -354,6 +420,10 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 		} else {
 			conn->c_my_gen_num = rds_gen_num;
 			conn->c_peer_gen_num = 0;
+			/* the initial reference belongs to whoever
+			 * destroys the conn; take one for our caller
+			 */
+			rds_conn_get(conn);
 			hlist_add_head_rcu(&conn->c_hash_node, head);
 			rds_cong_add_conn(conn);
 			rds_conn_count++;
@@ -364,6 +434,8 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 	rcu_read_unlock();
 
 out:
+	if (parent)
+		rds_conn_put(parent);
 	if (free_cp) {
 		for (i = 0; i < npaths; i++)
 			if (free_cp[i].cp_wq != rds_wq)
@@ -666,6 +738,9 @@ EXPORT_SYMBOL_GPL(rds_conn_put);
 void rds_conn_destroy(struct rds_connection *conn)
 {
 	int i;
+	struct rds_connection *passive, *parent;
+	struct hlist_head *head;
+	bool was_passive = false;
 	struct rds_conn_path *cp;
 	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
 
@@ -696,7 +771,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();
 
 	/* shut the connection down */
@@ -713,6 +819,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);
+	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 de5759c50b89..01e4b4be979d 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -924,8 +924,14 @@ 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);
+		/* The conn stays reachable through cm_id->context
+		 * without a reference of its own: connection destroy
+		 * shuts the cm_id down before the conn is freed.
+		 */
+		rds_conn_put(conn);
+	}
 	if (err)
 		rdma_reject(cm_id, &err, sizeof(int),
 			    IB_CM_REJ_CONSUMER_DEFINED);
diff --git a/net/rds/loop.c b/net/rds/loop.c
index 93be7832b11d..42e6b841b42c 100644
--- a/net/rds/loop.c
+++ b/net/rds/loop.c
@@ -168,7 +168,7 @@ static void rds_loop_destroy_gathered_conns(struct list_head *tmp_list)
 	struct rds_loop_connection *lc, *_lc;
 
 	list_for_each_entry_safe(lc, _lc, tmp_list, loop_node) {
-		WARN_ON(lc->conn->c_passive);
+		WARN_ON(rcu_access_pointer(lc->conn->c_passive));
 
 		spin_lock_irq(&loop_conns_lock);
 		list_del_init(&lc->loop_node);
diff --git a/net/rds/rds.h b/net/rds/rds.h
index 8a969444e698..4608615e09e9 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -161,7 +161,7 @@ struct rds_connection {
 	 * cancellation from landing on a destroyed workqueue.
 	 */
 	bool			c_destroy_in_prog;
-	struct rds_connection	*c_passive;
+	struct rds_connection __rcu *c_passive;
 	struct rds_transport	*c_trans;
 
 	struct rds_cong_map	*c_lcong;
diff --git a/net/rds/send.c b/net/rds/send.c
index 32c411d10e3e..1ae1f24c24e8 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -1159,13 +1159,14 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
 	DECLARE_SOCKADDR(struct sockaddr_in *, usin, msg->msg_name);
 	__be16 dport;
 	struct rds_message *rm = NULL;
-	struct rds_connection *conn;
+	struct rds_connection *conn = NULL;
 	int ret = 0;
 	int queued = 0, allocated_mr = 0;
 	int nonblock = msg->msg_flags & MSG_DONTWAIT;
 	long timeo = sock_sndtimeo(sk, nonblock);
 	struct rds_conn_path *cpath;
 	struct in6_addr daddr;
+	unsigned long flags;
 	__u32 scope_id = 0;
 	size_t rdma_payload_len = 0;
 	bool zcopy = ((msg->msg_flags & MSG_ZEROCOPY) &&
@@ -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);
 	} 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,
@@ -1352,9 +1371,17 @@ 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;
 		}
+		/* hand the cache its own reference */
+		rds_conn_get(conn);
+		spin_lock_irqsave(&rs->rs_lock, flags);
+		old = rs->rs_conn;
 		rs->rs_conn = conn;
+		spin_unlock_irqrestore(&rs->rs_lock, flags);
+		if (old)
+			rds_conn_put(old);
 	}
 
 	if (conn->c_trans->t_mp_capable) {
@@ -1474,6 +1501,8 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
 		kfree(vct.vec[ind].iov);
 	kfree(vct.vec);
 
+	rds_conn_put(conn);
+
 	return payload_len;
 
 out:
@@ -1481,6 +1510,9 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
 		kfree(vct.vec[ind].iov);
 	kfree(vct.vec);
 
+	if (conn)
+		rds_conn_put(conn);
+
 	/* If the user included a RDMA_MAP cmsg, we allocated a MR on the fly.
 	 * If the sendmsg goes through, we keep the MR. If it fails with EAGAIN
 	 * or in any other way, we need to destroy the MR again */
diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index 8a0c54aced5e..dcac10a91a67 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -153,7 +153,7 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
 {
 	struct socket *listen_sock = rtn->rds_tcp_listen_sock;
 	struct socket *new_sock = NULL;
-	struct rds_connection *conn;
+	struct rds_connection *conn = NULL;
 	int ret;
 	struct inet_sock *inet;
 	struct rds_tcp_connection *rs_tcp = NULL;
@@ -229,6 +229,7 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
 
 	if (IS_ERR(conn)) {
 		ret = PTR_ERR(conn);
+		conn = NULL;
 		goto out;
 	}
 	/* An incoming SYN request came in, and TCP just accepted it.
@@ -347,6 +348,8 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
 		mutex_unlock(&rs_tcp->t_conn_path_lock);
 	if (new_sock)
 		sock_release(new_sock);
+	if (conn)
+		rds_conn_put(conn);
 
 	mutex_unlock(&rtn->rds_tcp_accept_lock);
 
-- 
2.25.1

[PATCH net-next v2 7/9] net/rds: pin the connection across RDMA-CM event handling

[Allison Henderson]

rds_rdma_cm_event_handler_cmn() picks the connection up from
cm_id->context, which carries no reference, and holds c_cm_lock - a
mutex that lives in the connection's path array - across the
transport callbacks.  Now that a connection is freed by its last
reference rather than by rds_conn_destroy() itself, a callback that
drops the last reference other than the handler's implicit one leaves
the final mutex_unlock() running on freed memory.

Take a reference for the duration of the handler, and ignore the event
if the connection is already being freed: its cm_id teardown is what
stops event delivery, so an event that still arrives belongs to a
connection whose shutdown has run and whose memory is on its way out.

rds_ib_cm_handle_connect() has the mirror-image hole: a connection
whose destroy has already quiesced it sits in RDS_CONN_DOWN with no
cm_id, which is exactly the state the DOWN -> CONNECTING transition
claims.  A connect request arriving then would install a new cm_id
and QP on a connection that is only waiting for its last reference to
go away, and nothing would tear them down again.  Re-check
rds_destroy_pending() under c_cm_lock and reject the request instead.

Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/ib_cm.c          |  7 +++++++
 net/rds/rdma_transport.c | 16 +++++++++++++++-
 net/rds/rds.h            |  5 +++++
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 01e4b4be979d..323c1eee2777 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -874,6 +874,13 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id,
 	 * see the comment above rds_queue_reconnect()
 	 */
 	mutex_lock(&conn->c_cm_lock);
+	/* A destroy that has already quiesced this conn leaves it in
+	 * RDS_CONN_DOWN with no cm_id, exactly what the transition
+	 * below would happily claim; nothing would tear the new cm_id
+	 * and QP down again before the conn is freed.  Reject instead.
+	 */
+	if (rds_destroy_pending(conn))
+		goto out;
 	if (!rds_conn_transition(conn, RDS_CONN_DOWN, RDS_CONN_CONNECTING)) {
 		if (rds_conn_state(conn) == RDS_CONN_UP) {
 			rdsdebug("incoming connect while connecting\n");
diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
index b15cf316b23a..584e9867810f 100644
--- a/net/rds/rdma_transport.c
+++ b/net/rds/rdma_transport.c
@@ -63,6 +63,18 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
 	if (cm_id->device->node_type == RDMA_NODE_IB_CA)
 		trans = &rds_ib_transport;
 
+	/* cm_id->context carries no reference of its own.  Pin the
+	 * connection for the duration of the handler: what the callbacks
+	 * below do may drop the last reference other than ours, and the
+	 * mutex released at out: lives in the connection's path array.
+	 * A connection already being freed gets no events handled.
+	 */
+	if (conn && !rds_conn_get_unless_zero(conn)) {
+		rdsdebug("conn %p id %p is being freed, ignoring event\n",
+			 conn, cm_id);
+		return 0;
+	}
+
 	/* Prevent shutdown from tearing down the connection
 	 * while we're executing. */
 	if (conn) {
@@ -171,8 +183,10 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
 	}
 
 out:
-	if (conn)
+	if (conn) {
 		mutex_unlock(&conn->c_cm_lock);
+		rds_conn_put(conn);
+	}
 
 	rdsdebug("id %p event %u (%s) handling ret %d\n", cm_id, event->event,
 		 rdma_event_msg(event->event), ret);
diff --git a/net/rds/rds.h b/net/rds/rds.h
index 4608615e09e9..06d48c2821ef 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -840,6 +840,11 @@ void rds_conn_shutdown(struct rds_conn_path *cpath);
 void rds_conn_destroy(struct rds_connection *conn);
 void rds_conn_get(struct rds_connection *conn);
 void rds_conn_put(struct rds_connection *conn);
+/* take a reference unless the connection is already being freed */
+static inline bool rds_conn_get_unless_zero(struct rds_connection *conn)
+{
+	return kref_get_unless_zero(&conn->c_refcount);
+}
 /* transport unload waits for its connections to be freed, polling at
  * the first interval and warning at the second
  */
-- 
2.25.1

[PATCH net-next v2 8/9] net/rds: drop rds_conn_count in favor of t_conn_count

[Allison Henderson]

The previous patch gave each transport its own connection count in
t_conn_count, incremented and decremented at exactly the points where
the global rds_conn_count is.  That leaves rds_conn_count with a
single remaining consumer: the seed of the per-path workqueue names
in __rds_conn_create().

Switch the name seed to t_conn_count, as UEK does, and remove
rds_conn_count.  The numbering becomes per-transport instead of
global, so connections of different transports can now receive the
same seed; workqueue names carry no uniqueness requirement, and the
seed was already reused as the count rose and fell.  Removing the
counter also removes the rds_conn_lock round-trip that
rds_conn_destroy_fini() took solely to decrement it, leaving the
free path lock-free.

Based on the Oracle UEK commit "net/rds: Each RDS transport
should keep its own connection count".

Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/connection.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index 1ec141d6faa3..11813f93961d 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -47,7 +47,6 @@
 
 /* converting this to RCU is a chore for another day.. */
 static DEFINE_SPINLOCK(rds_conn_lock);
-static unsigned long rds_conn_count;
 /* woken whenever a transport's t_conn_count drops to zero */
 static DECLARE_WAIT_QUEUE_HEAD(rds_conn_freed_waitq);
 static struct hlist_head rds_conn_hash[RDS_CONNECTION_HASH_ENTRIES];
@@ -316,12 +315,13 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 
 	init_waitqueue_head(&conn->c_hs_waitq);
 	for (i = 0; i < npaths; i++) {
+		int seq = atomic_read(&trans->t_conn_count);
+
 		__rds_conn_path_init(conn, &conn->c_path[i],
 				     is_outgoing);
 		conn->c_path[i].cp_index = i;
 		conn->c_path[i].cp_wq =
-			alloc_ordered_workqueue("krds_cp_wq#%lu/%d", 0,
-						rds_conn_count, i);
+			alloc_ordered_workqueue("krds_cp_wq#%d/%d", 0, seq, i);
 		if (!conn->c_path[i].cp_wq)
 			conn->c_path[i].cp_wq = rds_wq;
 	}
@@ -392,7 +392,6 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 			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);
 		}
 	} else {
@@ -426,7 +425,6 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 			rds_conn_get(conn);
 			hlist_add_head_rcu(&conn->c_hash_node, head);
 			rds_cong_add_conn(conn);
-			rds_conn_count++;
 			atomic_inc(&conn->c_trans->t_conn_count);
 		}
 	}
@@ -656,7 +654,6 @@ static void rds_conn_destroy_fini(struct kref *kref)
 						   c_refcount);
 	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
 	struct rds_transport *trans = conn->c_trans;
-	unsigned long flags;
 	int i;
 
 	for (i = 0; i < npaths; i++)
@@ -665,10 +662,6 @@ static void rds_conn_destroy_fini(struct kref *kref)
 	kfree(conn->c_path);
 	kmem_cache_free(rds_conn_slab, conn);
 
-	spin_lock_irqsave(&rds_conn_lock, flags);
-	rds_conn_count--;
-	spin_unlock_irqrestore(&rds_conn_lock, flags);
-
 	/* only after everything the transport module owns has been
 	 * freed above may its unload proceed
 	 */
-- 
2.25.1

[PATCH net-next v2 9/9] net/rds: hold a connection reference from struct rds_incoming

[Allison Henderson]

From: Sharath Srinivasan <sharath.srinivasan@oracle.com>

struct rds_incoming->i_conn stores a pointer to the connection a message
it belongs to, for both received messages and messages the socket sends.
But without taking a reference, nothing keeps that connection alive.
Embedded as the messages m_inc, an inc routinely outlives the connection
it points at, by sitting in the socket's receive queue until the
application reads it, while the connection is destroyed by device removal,
netns teardown or module unload - and every dereference of i_conn after
that point touches freed memory.

Chengfeng Ye reported one way to reach it, where the socket info
callbacks walk a receive queue after rmmod freed the connections:

  BUG: KASAN: slab-use-after-free in rds6_inc_info_copy+0x459/0x530 [rds]
  Read of size 1 at addr ffff888106031c50 by task poc/101
  Call Trace:
   rds6_inc_info_copy+0x459/0x530 [rds]
   rds6_sock_inc_info+0x2b9/0x3c0 [rds]
   rds_info_getsockopt+0x19d/0x380 [rds]
   do_sock_getsockopt+0x2ac/0x480
   __sys_getsockopt+0x128/0x210
  Freed by task 102:
   kmem_cache_free+0x1b5/0x3d0
   rds_conn_destroy+0x484/0x600 [rds]
   rds_loop_exit_net+0x32/0x50 [rds]
   unregister_pernet_device+0x2c/0x50
   rds_conn_exit+0x13/0xa0 [rds]
   rds_exit+0x1a/0xc40 [rds]
   __do_sys_delete_module+0x30a/0x4d0

Closing the socket gets there too, with no reader of i_conn other than
RDS itself: freeing an IB inc dereferences i_conn to hand the inc and
its fragments back to the connection's recycle cache, so draining the
receive queue of a socket whose connection is gone crashes in the
transport:

  panic
  ...
  rds_ib_recv_cache_put (net/rds/ib_recv.c:703)
  rds_ib_inc_free (net/rds/ib_recv.c:207)
  rds_clear_recv_queue (net/rds/recv.c:909)
  rds_release (net/rds/af_rds.c:212)
  __sock_release (net/socket.c:649)
  sock_close (net/socket.c:1336)

with the freed connection confirmed by its now-zero reference count:

  -trace[9]["inc"].i_conn.c_refcount
  (struct kref){
          .refcount = (refcount_t){
                  .refs = (atomic_t){
                          .counter = (int)0,
                  },
          },
  }

Now that connections are reference counted, make every holder of i_conn
own a reference.  The pointer is assigned in six places - rds_inc_init()
and rds_inc_path_init() for received messages, rds_recv_incoming() when
it re-points an inc at the connection it arrived on, and
rds_send_queue_rm(), rds_send_probe() and the congestion-map path of
rds_send_xmit() for m_inc - and each of them now takes a reference.  The
references are dropped from rds_inc_put() and rds_message_put(), which
are the points where the last user of the pointer goes away.
rds_recv_incoming() takes the new reference before dropping the old one,
so re-pointing an inc at the connection it already refers to cannot free
it.  rds_inc_put() drops its reference through a local copy, since
inc_free() may free the memory the inc lives in.

This keeps a connection allocated for as long as messages that arrived
over it are queued on sockets, which is longer than before.  What
lingers is the quiesced connection, its per-path workqueues (idle,
every work cancelled) and the transport's per-connection state,
including an IB connection's receive caches: rds_conn_destroy() still
quiesces the connection synchronously, so nothing runs on any of it.
It also means the transport cannot unload while such a datagram is
unread - rds_conn_wait_conns_freed() waits, warning every ten seconds,
until the application reads or closes.  That is the intended
behaviour: teardown does not discard queued data, and unloading over a
live reference would be a use-after-free in the transport's text.

The final rds_conn_put() runs the free path, which destroys the per-path
workqueues and therefore may sleep, so the last reference has to be
dropped from process context.  It always is.  While a connection is
alive its hash-table entry holds the initial reference, so a put from a
completion handler or tasklet can never be the last one; that reference
is dropped by rds_conn_destroy(), from process context, after the
connection has been quiesced and its queued messages freed.  The
references that survive that point are the ones held by incs on socket
receive queues and by messages on socket send queues, and those are
dropped from recvmsg and from close - process context in both cases.

This is not a stable candidate: reaching the use-after-free requires
freeing a connection out from under a live socket, which needs
CAP_SYS_MODULE, netns teardown or physical device removal, and the fix
depends on the connection reference counting introduced earlier in this
series.

Based on the Oracle UEK commit "net/rds: fix crash by
expanding kref coverage to rds_incoming.i_conn".

Reported-by: Chengfeng Ye <nicoyip.dev@gmail.com>
Closes: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/
Signed-off-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>
Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com>
[achender: port to net-next: same six assignment sites, but upstream
 splits the message free across rds_message_unpin_worker(), so the
 m_inc reference is dropped from a shared rds_message_free() helper
 that both paths call; rds_recv_incoming() takes the new reference
 before dropping the old; rewrite commit message]
Assisted-by: Claude-Code:claude-opus-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/message.c | 16 ++++++++++++++--
 net/rds/recv.c    | 21 ++++++++++++++++++++-
 net/rds/send.c    |  4 ++++
 3 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/net/rds/message.c b/net/rds/message.c
index f25f2592586f..29e95028e61e 100644
--- a/net/rds/message.c
+++ b/net/rds/message.c
@@ -182,6 +182,18 @@ static void rds_message_purge(struct rds_message *rm)
 		kref_put(&rm->atomic.op_rdma_mr->r_kref, __rds_put_mr_final);
 }
 
+static void rds_message_free(struct rds_message *rm)
+{
+	/* get in rds_send_queue_rm(), rds_send_probe() or the congestion
+	 * map path of rds_send_xmit().  Messages that were never queued on
+	 * a connection have no reference to drop.
+	 */
+	if (rm->m_inc.i_conn)
+		rds_conn_put(rm->m_inc.i_conn);
+
+	kfree(rm);
+}
+
 static void rds_message_unpin_worker(struct work_struct *work)
 {
 	struct rds_message *rm = container_of(work, struct rds_message,
@@ -192,7 +204,7 @@ static void rds_message_unpin_worker(struct work_struct *work)
 	if (rm->atomic.op_unpin_deferred)
 		rds_atomic_op_unpin_page(&rm->atomic);
 
-	kfree(rm);
+	rds_message_free(rm);
 }
 
 void rds_message_put(struct rds_message *rm)
@@ -217,7 +229,7 @@ void rds_message_put(struct rds_message *rm)
 			return;
 		}
 
-		kfree(rm);
+		rds_message_free(rm);
 	}
 }
 EXPORT_SYMBOL_GPL(rds_message_put);
diff --git a/net/rds/recv.c b/net/rds/recv.c
index 6204e577a90a..b031c0b43af8 100644
--- a/net/rds/recv.c
+++ b/net/rds/recv.c
@@ -46,6 +46,7 @@ void rds_inc_init(struct rds_incoming *inc, struct rds_connection *conn,
 {
 	refcount_set(&inc->i_refcount, 1);
 	INIT_LIST_HEAD(&inc->i_item);
+	rds_conn_get(conn);	/* put in rds_inc_put() */
 	inc->i_conn = conn;
 	inc->i_conn_path = NULL;
 	inc->i_saddr = *saddr;
@@ -61,6 +62,7 @@ void rds_inc_path_init(struct rds_incoming *inc, struct rds_conn_path *cp,
 {
 	refcount_set(&inc->i_refcount, 1);
 	INIT_LIST_HEAD(&inc->i_item);
+	rds_conn_get(cp->cp_conn);	/* put in rds_inc_put() */
 	inc->i_conn = cp->cp_conn;
 	inc->i_conn_path = cp;
 	inc->i_saddr = *saddr;
@@ -81,9 +83,19 @@ void rds_inc_put(struct rds_incoming *inc)
 {
 	rdsdebug("put inc %p ref %d\n", inc, refcount_read(&inc->i_refcount));
 	if (refcount_dec_and_test(&inc->i_refcount)) {
+		struct rds_connection *conn = inc->i_conn;
+
 		BUG_ON(!list_empty(&inc->i_item));
 
-		inc->i_conn->c_trans->inc_free(inc);
+		/* inc_free() can free the memory @inc lives in, so the
+		 * connection reference has to be dropped through the
+		 * copy taken above.
+		 */
+		conn->c_trans->inc_free(inc);
+		/* get in rds_inc_init(), rds_inc_path_init() or
+		 * rds_recv_incoming()
+		 */
+		rds_conn_put(conn);
 	}
 }
 EXPORT_SYMBOL_GPL(rds_inc_put);
@@ -325,6 +337,13 @@ void rds_recv_incoming(struct rds_connection *conn, struct in6_addr *saddr,
 	unsigned long flags;
 	struct rds_conn_path *cp;
 
+	/* every caller initialized @inc with rds_inc_init() or
+	 * rds_inc_path_init() first, so i_conn already holds a reference.
+	 * Take the new one before dropping the old, so that re-pointing an
+	 * inc at the connection it already refers to cannot free it.
+	 */
+	rds_conn_get(conn);
+	rds_conn_put(inc->i_conn);
 	inc->i_conn = conn;
 	inc->i_rx_jiffies = jiffies;
 	if (conn->c_trans->t_mp_capable)
diff --git a/net/rds/send.c b/net/rds/send.c
index 1ae1f24c24e8..dbda24470d21 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -290,6 +290,8 @@ int rds_send_xmit(struct rds_conn_path *cp)
 			}
 			rm->data.op_active = 1;
 			rm->m_inc.i_conn_path = cp;
+			/* put in rds_message_put() */
+			rds_conn_get(cp->cp_conn);
 			rm->m_inc.i_conn = cp->cp_conn;
 
 			cp->cp_xmit_rm = rm;
@@ -947,6 +949,7 @@ static int rds_send_queue_rm(struct rds_sock *rs, struct rds_connection *conn,
 		/* The code ordering is a little weird, but we're
 		   trying to minimize the time we hold c_lock */
 		rds_message_populate_header(&rm->m_inc.i_hdr, sport, dport, 0);
+		rds_conn_get(conn);	/* put in rds_message_put() */
 		rm->m_inc.i_conn = conn;
 		rm->m_inc.i_conn_path = cp;
 		rds_message_addref(rm);
@@ -1559,6 +1562,7 @@ rds_send_probe(struct rds_conn_path *cp, __be16 sport,
 	list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
 	set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
 	rds_message_addref(rm);
+	rds_conn_get(cp->cp_conn);	/* put in rds_message_put() */
 	rm->m_inc.i_conn = cp->cp_conn;
 	rm->m_inc.i_conn_path = cp;
 
-- 
2.25.1

Re: [PATCH net-next v2 1/9] net/rds: guard every work-requeueing site with rds_destroy_pending()

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] At this commit the five newly added `rcu_read_lock() +
  rds_destroy_pending()` guards (net/rds/ib_send.c:302-306 and :429-433,…
- [Low] The subject claims to guard "every work-requeueing site" and the
  body states exactly "Five arming sites never got that guard", but four…

--

net/rds: guard every work-requeueing site with rds_destroy_pending()

This adds rcu_read_lock() + rds_destroy_pending() guards around five
queue_delayed_work() sites that re-arm cp_send_w, cp_recv_w and cp_conn_w:
two in net/rds/ib_send.c, one in net/rds/ib_recv.c, one in
net/rds/tcp_listen.c and one in net/rds/send.c.  The stated goal is that no
new path work can be queued once rds_conn_path_destroy() has started
cancelling works and is about to call destroy_workqueue().

A couple of questions on the changelog and on the effect of the guards at
this commit.

> net/rds: guard every work-requeueing site with rds_destroy_pending()

[ ... ]

>     Five arming sites never got that guard:

[Severity: Low]
Are those five really all of the requeueing sites?  net/rds/threads.c has
four more queue_delayed_work() calls on the same works that stay unguarded.

rds_send_worker():

	switch (ret) {
	case -EAGAIN:
		rds_stats_inc(s_send_immediate_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 0);
		break;
	case -ENOMEM:
		rds_stats_inc(s_send_delayed_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 2);

rds_recv_worker() has the matching pair for cp_recv_w.

These four are self-requeues issued from inside the running work item, and
in this tree cancel_delayed_work_sync() -> __cancel_work_sync() disables the
work with WORK_CANCEL_DISABLE before __flush_work(), so a requeue from the
still-running callback is rejected while the cancel is in progress and no
use-after-free follows.

The subject says "every work-requeueing site" and the body gives an exact
count of five, and the following patch in the series repeats that "all of
those sites guard the queueing with rds_destroy_pending() under
rcu_read_lock() (the last stragglers were converted by the previous patch)".
Could the changelog either convert the threads.c sites too, or say that
in-callback self-requeues are exempt and why?

The Fixes: tag points at ebeeb1ad9b8a, and on trees before
cancel_delayed_work_sync() gained the disable semantics a self-requeue from
the callback was not rejected, so a backporter reading this message cannot
tell whether those sites were considered.

> diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
> index bd6cb3ffaa571..7d45808544a0d 100644
> --- a/net/rds/ib_recv.c
> +++ b/net/rds/ib_recv.c
> @@ -458,7 +458,11 @@ void rds_ib_recv_refill(struct rds_connection *conn, int prefill, gfp_t gfp)
>  	    (must_wake ||
>  	    (can_wait && rds_ib_ring_low(&ic->i_recv_ring)) ||
>  	    rds_ib_ring_empty(&ic->i_recv_ring))) {
> -		queue_delayed_work(conn->c_path->cp_wq, &conn->c_recv_w, 1);
> +		rcu_read_lock();
> +		if (!rds_destroy_pending(conn))
> +			queue_delayed_work(conn->c_path->cp_wq,
> +					   &conn->c_recv_w, 1);
> +		rcu_read_unlock();
>  	}

[Severity: Low]
Can these guards close the cancel -> destroy_workqueue() window as of this
commit?  rds_destroy_pending() in net/rds/rds.h carries no per-connection
destroy state here:

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

and rds_conn_destroy() publishes nothing that the guards can observe before
the grace period:

	spin_lock_irq(&rds_conn_lock);
	hlist_del_init_rcu(&conn->c_hash_node);
	spin_unlock_irq(&rds_conn_lock);
	synchronize_rcu();

For a single-connection teardown with a live netns and the module loaded,
for example the protocol version mismatch path
rds_ib_cm_connect_complete() -> rds_conn_destroy(), the predicate reads
false, so a completion landing in rds_ib_recv_refill() or
rds_ib_send_cqe_handler() can still arm cp_recv_w/cp_send_w between
cancel_delayed_work_sync() and destroy_workqueue() in
rds_conn_path_destroy().

The next patch in the series, "net/rds: make rds_destroy_pending() cover
single-connection destroy", adds conn->c_destroy_in_prog, sets it under
rds_conn_lock before hlist_del_init_rcu() and synchronize_rcu(), and tests
it first in rds_destroy_pending(), which is what gives these five guards
their effect, so at the end of the series the window is closed.

Given the Fixes: tag invites a standalone backport, would it help to state
in this changelog that the guards only take effect together with the
c_destroy_in_prog change, or to reorder the two patches?

[ ... ]

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

Re: [PATCH net-next v2 2/9] net/rds: make rds_destroy_pending() cover single-connection destroy

[netdev-bot+sashiko]

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

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

- [Low] The commit message and both new comments assert an invariant the
  tree does not hold.

Pre-existing issues:
- [High] Deadlock in the very single-connection destroy path this patch
  is written for.
- [High] Reported concern (judged resolved later in the series):
  rds_destroy_pending() now dereferences conn->c_destroy_in_prog, but…
- [Low] Reported concern (judged invalid at the end of the series):
  rds_conn_destroy()'s header comment at net/rds/connection.c:558-565…
- [Low] Reported concern (judged a false positive): that
  rds_conn_path_drop(cp, destroy=true) (net/rds/connection.c:997-1008)…

--

net/rds: make rds_destroy_pending() cover single-connection destroy

This adds a per-connection c_destroy_in_prog flag, set by
rds_conn_destroy() before the unhash plus synchronize_rcu() sequence, and
tests it in rds_destroy_pending() so that a single-connection destroy is
covered by the same predicate the work-requeueing sites already consult.
The now-unreferenced RDS_DESTROY_PENDING cp_flags bit and its dead test in
rds_ib_is_unloading() are removed.

A few questions below.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index b6c4beb50eaf0..50e1b6bfceeab 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -574,6 +574,14 @@ void rds_conn_destroy(struct rds_connection *conn)
>  		 "%pI4\n", conn, &conn->c_laddr,
>  		 &conn->c_faddr);
>  
> +	/* Make rds_destroy_pending() true for this conn.  Together with
> +	 * the synchronize_rcu() below this stops the work-requeueing
> +	 * sites (which all test rds_destroy_pending() under
> +	 * rcu_read_lock()) from queueing new work on the path
> +	 * workqueues once we start cancelling and destroying them.
> +	 */

[Severity: Low]
Is the "which all test rds_destroy_pending() under rcu_read_lock()" part
accurate for the current tree?  rds_send_worker() and rds_recv_worker() in
net/rds/threads.c re-arm their own works with no predicate test and no RCU
read-side section:

net/rds/threads.c:rds_send_worker() {
	...
	switch (ret) {
	case -EAGAIN:
		rds_stats_inc(s_send_immediate_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 0);
		break;
	case -ENOMEM:
		rds_stats_inc(s_send_delayed_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 2);
	...
}

rds_recv_worker() does the same for cp_recv_w.

Those two do look safe, but for a different reason than the one documented
here: rds_conn_path_destroy() runs cancel_delayed_work_sync(&cp->cp_send_w)
and cancel_delayed_work_sync(&cp->cp_recv_w), and __cancel_work_sync()
disables the work for the duration of the cancel, so a self-requeue from the
still-running worker is dropped and the worker cannot requeue once the sync
cancel has returned.

The commit message makes the same universal claim:

    since all of those sites guard the queueing with rds_destroy_pending()
    under rcu_read_lock() (the last stragglers were converted by the
    previous patch)

Could both the comment and the commit message be reworded so the two
self-requeue sites in threads.c are not covered by the statement?

> +	WRITE_ONCE(conn->c_destroy_in_prog, true);
> +
>  	/* Ensure conn will not be scheduled for reconnect */
>  	spin_lock_irq(&rds_conn_lock);
>  	hlist_del_init_rcu(&conn->c_hash_node);

[Severity: High]
This isn't a bug introduced by this patch, but the single-connection destroy
path the commit message names as the motivation looks like it deadlocks
before the new flag matters.

rds_ib_cm_connect_complete() calls rds_conn_destroy() synchronously from
inside the RDMA CM event handler, which still holds conn->c_cm_lock (that is
c_path[0].cp_cm_lock, see #define c_cm_lock c_path[0].cp_cm_lock in
net/rds/rds_single_path.h):

rds_rdma_cm_event_handler_cmn()
  mutex_lock(&conn->c_cm_lock);
  rds_ib_cm_connect_complete()
    rds_conn_destroy()
      rds_conn_path_destroy()
        rds_conn_path_drop(cp, true);   /* cp_state = RDS_CONN_ERROR,
                                           queue_work(cp_down_w) */
        flush_work(&cp->cp_down_w);

The queued pass then blocks on the mutex the flushing thread still holds:

rds_shutdown_worker()
  rds_conn_shutdown(cp)
    /* DOWN->DOWN transition fails, state is RDS_CONN_ERROR */
    mutex_lock(&cp->cp_cm_lock);

The handler cannot reach its mutex_unlock() until rds_conn_destroy() returns,
and rds_conn_destroy() cannot return until cp_down_w completes.  Can this
hang both the rdma_cm event worker and the path workqueue worker, with the
connection left in DISCONNECTING/ERROR?

There appears to be a second cycle on the same path: if the shutdown pass
ever got past the mutex, rds_ib_conn_path_shutdown() calls
rdma_destroy_id(ic->i_cm_id) on the very cm_id whose event is being handled,
which waits on id_priv->handler_mutex held by the same stuck thread.

The deciding input is peer supplied: the branch is taken when the peer's
accept private data decodes to a protocol version below 3.1, with
RDS_PROTOCOL_VERSION == RDS_PROTOCOL_COMPAT_VERSION == 0x0301.

> diff --git a/net/rds/rds.h b/net/rds/rds.h
> index 2db49573dacd5..50b08c28ab865 100644
> --- a/net/rds/rds.h
> +++ b/net/rds/rds.h

[ ... ]

> @@ -148,6 +147,14 @@ struct rds_connection {
>  				c_pad_to_32:29;
>  	int			c_npaths;
>  	bool			c_with_sport_idx;
> +	/* Set once, by rds_conn_destroy(), before it cancels the path
> +	 * works; read through rds_destroy_pending().  A site that arms
> +	 * a path work must test the predicate and queue the work inside
> +	 * one rcu_read_lock() section: the synchronize_rcu() that
> +	 * follows the store is what keeps a queue issued after the
> +	 * cancellation from landing on a destroyed workqueue.
> +	 */
> +	bool			c_destroy_in_prog;
>  	struct rds_connection	*c_passive;
>  	struct rds_transport	*c_trans;
>  
> @@ -994,7 +1001,8 @@ void __rds_put_mr_final(struct kref *kref);
>  
>  static inline bool rds_destroy_pending(struct rds_connection *conn)
>  {
> -	return !check_net(rds_conn_net(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));
>  }

[Severity: High]
This isn't a bug introduced by this patch, but rds_destroy_pending() now
dereferences the conn to read c_destroy_in_prog, while at this commit
rds_conn_destroy() still frees the conn synchronously right after the paths
are torn down:

net/rds/connection.c:rds_conn_destroy() {
	...
	kfree(conn->c_path);
	kmem_cache_free(rds_conn_slab, conn);
}

Several sites that evaluate the predicate hold raw, unreferenced conn
pointers: rs->rs_conn in rds_sendmsg(), tc->t_cpath->cp_conn reached from
sk->sk_user_data in the TCP callbacks, and ic->conn on the IB side.  Can
such a reader evaluate READ_ONCE(conn->c_destroy_in_prog) on freed slab
memory and, on a stale false, go on to queue work on a cp_wq that
destroy_workqueue() has already freed?

Later patches in this series do supply the liveness the new guard needs:
"net/rds: split connection destroy into quiesce and kref-governed free"
moves conn_free, destroy_workqueue, kfree(c_path) and kmem_cache_free into
rds_conn_destroy_fini() at the last rds_conn_put(); "net/rds: hold
connection references in lookup, sockets and c_passive" gives rs->rs_conn
its own reference; and "net/rds: pin the connection across RDMA-CM event
handling" pins the conn around the CM handler.  Would it be clearer to order
this patch after those, or to note the dependency in the commit message?

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

Re: [PATCH net-next v2 3/9] net/rds: split connection destroy into quiesce and kref-governed free

[netdev-bot+sashiko]

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

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

- [Low] The transport's conn_free() and destroy_workqueue() move out of
  the synchronous destroy path into the kref release callback…
- [Low] The new comment on struct rds_connection::c_refcount
  (net/rds/rds.h:140-144) states that only the connection memory free is…

Pre-existing issues:
- [High] Destroying an unsupported-version IB connection from the RDMA-CM
  ESTABLISHED callback deadlocks.

--

net/rds: split connection destroy into quiesce and kref-governed free

This adds a kref to struct rds_connection, keeps the quiesce half of
rds_conn_destroy() synchronous, and moves the frees (transport conn_free,
path workqueues, c_path array, slab object) into rds_conn_destroy_fini()
which runs on the last rds_conn_put().  rds_conn_get()/rds_conn_put() are
exported for holders added in later patches.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 50e1b6bfceeab..cc4e74b731df9 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -547,6 +551,16 @@ static void rds_conn_path_destroy(struct rds_conn_path *cp)
>  	WARN_ON(delayed_work_pending(&cp->cp_recv_w));
>  	WARN_ON(delayed_work_pending(&cp->cp_conn_w));
>  	WARN_ON(work_pending(&cp->cp_down_w));
> +}
> +
> +/* free a quiesced rds_conn_path's transport state and workqueue; runs
> + * from rds_conn_destroy_fini() once the last connection reference is
> + * dropped.
> + */
> +static void rds_conn_path_free(struct rds_conn_path *cp)
> +{
> +	if (!cp->cp_transport_data)
> +		return;
>  
>  	if (cp->cp_wq != rds_wq) {
>  		destroy_workqueue(cp->cp_wq);

[Severity: Low]
This moves the transport's conn_free() and destroy_workqueue() out of the
synchronous destroy path and into the kref release callback, and the same
patch exports rds_conn_get()/rds_conn_put().  Is there anything yet that
makes a transport module unload wait for outstanding references?

rds_tcp_exit() destroys the connections and then immediately proceeds to
rds_trans_unregister() and kmem_cache_destroy(rds_tcp_conn_slab), so a
deferred rds_conn_path_free() would call conn_free() in unloaded module
text and free into a destroyed slab cache.

The transport teardown helpers also splice their connection lists onto an
on-stack list and rely on conn_free() unlinking each node before the
helper returns:

net/rds/tcp.c:rds_tcp_destroy_conns() {
	...
	list_for_each_entry_safe(tc, _tc, &tmp_list, t_tcp_node)
		rds_conn_destroy(tc->t_cpath->cp_conn);
}

net/rds/ib_rdma.c:rds_ib_destroy_nodev_conns() {
	...
	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
		rds_conn_destroy(ic->conn);
}

with a deferred free, rds_tcp_conn_free()/rds_ib_conn_free() would
list_del() into a stack frame that no longer exists.

For what it is worth, this looks latent at this commit rather than
reachable: rds_conn_get() has no callers here and the only rds_conn_put()
is the one in rds_conn_destroy(), so kref_init()'s reference is always the
last and every conn_free()/destroy_workqueue() still completes before
rds_conn_destroy() returns.  The following patches in the series appear to
close both holes, with t_conn_count plus rds_conn_wait_conns_freed() in
rds_ib_exit()/rds_tcp_exit()/rds_loop_exit(), and with the transport nodes
being unlinked before the possibly deferred free.

Would it be worth mentioning in this commit message that the exported
get/put must not be used by anyone until those two patches land, or
ordering the unload barrier ahead of this split?

> @@ -556,16 +570,52 @@ static void rds_conn_path_destroy(struct rds_conn_path *cp)
>  	cp->cp_conn->c_trans->conn_free(cp->cp_transport_data);
>  }
>  
> +/* Free a connection.  This runs from rds_conn_put() when the last
> + * reference is dropped, after rds_conn_destroy() has quiesced the
> + * connection and dropped the initial reference.
> + */
> +static void rds_conn_destroy_fini(struct kref *kref)
> +{
> +	struct rds_connection *conn = container_of(kref, struct rds_connection,
> +						   c_refcount);
> +	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
> +	unsigned long flags;
> +	int i;
> +
> +	for (i = 0; i < npaths; i++)
> +		rds_conn_path_free(&conn->c_path[i]);
> +
> +	kfree(conn->c_path);
> +	kmem_cache_free(rds_conn_slab, conn);
> +
> +	spin_lock_irqsave(&rds_conn_lock, flags);
> +	rds_conn_count--;
> +	spin_unlock_irqrestore(&rds_conn_lock, flags);
> +}
> +
> +void rds_conn_get(struct rds_connection *conn)
> +{
> +	kref_get(&conn->c_refcount);
> +}
> +EXPORT_SYMBOL_GPL(rds_conn_get);
> +
> +void rds_conn_put(struct rds_connection *conn)
> +{
> +	kref_put(&conn->c_refcount, rds_conn_destroy_fini);
> +}
> +EXPORT_SYMBOL_GPL(rds_conn_put);
> +
>  /*
>   * Stop and free a connection.
>   *
> - * This can only be used in very limited circumstances.  It assumes that once
> - * the conn has been shutdown that no one else is referencing the connection.
> - * We can only ensure this in the rmmod path in the current code.
> + * Quiesces the connection synchronously (workers cancelled, transport
> + * connections shut down, queued messages dropped) and drops the
> + * initial reference.  The memory - including the transport's
> + * per-connection state and the path workqueues - is freed once the
> + * last rds_conn_put() runs, which may be after this returns.
>   */
>  void rds_conn_destroy(struct rds_connection *conn)
>  {

[Severity: High]
This isn't a bug introduced by this patch, but since the commit message
names rds_ib_cm_connect_complete() as one of the supported entry points
into rds_conn_destroy(), can that caller actually complete?

The RDMA-CM handler holds the mutex across the transport callback:

net/rds/rdma_transport.c:rds_rdma_cm_event_handler_cmn() {
	...
	if (conn) {
		mutex_lock(&conn->c_cm_lock);
	...
	case RDMA_CM_EVENT_ESTABLISHED:
		if (conn)
			trans->cm_connect_complete(conn, event);
	...
out:
	if (conn)
		mutex_unlock(&conn->c_cm_lock);
}

and c_cm_lock is c_path[0].cp_cm_lock (net/rds/rds_single_path.h), the
mutex the path-0 shutdown worker takes.  For a peer below
RDS_PROTOCOL_VERSION that is not RDS_PROTOCOL_COMPAT_VERSION:

net/rds/ib_cm.c:rds_ib_cm_connect_complete() {
	...
		rds_conn_destroy(conn);
		return;
}

runs with that mutex still held.  rds_conn_destroy() then reaches:

net/rds/connection.c:rds_conn_path_quiesce() {
	...
	rds_conn_path_drop(cp, true);
	flush_work(&cp->cp_down_w);
}

rds_conn_path_drop() sets cp_state to RDS_CONN_ERROR before queueing
cp_down_w, so the flushed worker always fails the first transition in
rds_conn_shutdown() and falls into:

net/rds/connection.c:rds_conn_shutdown() {
	if (!rds_conn_path_transition(cp, RDS_CONN_DOWN, RDS_CONN_DOWN)) {
		...
		mutex_lock(&cp->cp_cm_lock);
}

Is this a circular wait?  The flushing thread cannot drop c_cm_lock until
rds_conn_destroy() returns, and rds_conn_destroy() cannot return until the
worker acquires the same mutex.  The negotiated version comes from the
peer's private data, so a remote peer advertising an old version appears to
be enough to trigger it, with no timing window involved.

The same code is present in the baseline (879e280b) and still present at
the end of this series (rds_ib_cm_connect_complete() still calls
rds_conn_destroy() from the callback, and the handler still unlocks
c_cm_lock only after the callbacks return), so this predates the series.
Should this destroy be deferred out of the CM callback instead?

[ ... ]

> diff --git a/net/rds/rds.h b/net/rds/rds.h
> index 50b08c28ab865..49629108c22ad 100644
> --- a/net/rds/rds.h
> +++ b/net/rds/rds.h
> @@ -137,6 +137,12 @@ struct rds_conn_path {
>  /* One rds_connection per RDS address pair */
>  struct rds_connection {
>  	struct hlist_node	c_hash_node;
> +	/* Free of the connection memory (not the teardown of its
> +	 * transport state - that stays synchronous in
> +	 * rds_conn_destroy()) is deferred until the last reference is
> +	 * dropped via rds_conn_put().
> +	 */
> +	struct kref		c_refcount;

[Severity: Low]
This isn't a bug, but does this comment contradict the other two comments
added by the same patch?

Here the claim is that the teardown of the transport state "stays
synchronous in rds_conn_destroy()", while rds_conn_path_quiesce() says
"the transport state and the workqueue are freed later, from
rds_conn_path_free()", and rds_conn_destroy() says "The memory - including
the transport's per-connection state and the path workqueues - is freed
once the last rds_conn_put() runs".

rds_conn_path_free() does call c_trans->conn_free() and
destroy_workqueue(), and it runs from rds_conn_destroy_fini(), so the
rds.h wording looks like the odd one out.  The same wording is still
present at the end of the series, where the deferral of the transport
state is what motivates the new unload wait.  Could this comment be
reworded to match?

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

Re: [PATCH net-next v2 4/9] net/rds: wait for connections to be freed on transport unload

[netdev-bot+sashiko]

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

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

- [High] rds_ib_exit() installs rds_ib_destroy_nodev_conns() as the
  repeatedly-invoked `resweep` callback of the new…
- [Medium] rds_conn_wait_conns_freed() (net/rds/connection.c:619-642)
  loops on wait_event_timeout(), i.e. TASK_UNINTERRUPTIBLE sleep, with…
- [Medium] The new unload barrier is released from inside transport-
  module context.
- [Low] (Baseline decision: FALSE - refuted by code.) The concern was
  that the single flush_workqueue(rds_wq) added at the end of…

Pre-existing issues:
- [Low] (Baseline decision: FALSE - discarded as resolved later in the
  series.) The concern was that the new comment above…

--

net/rds: wait for connections to be freed on transport unload

This adds a per-transport live connection count (t_conn_count), bumped when
a connection is published in __rds_conn_create() and dropped as the last
step of rds_conn_destroy_fini().  A new helper,
rds_conn_wait_conns_freed(), polls that count so rds_ib_exit(),
rds_tcp_exit() and rds_loop_exit() do not unload while a connection free is
still pending, with an optional resweep callback for IB's asynchronous
device detach.

A few questions below about the resweep callback, the unbounded wait, and
where the wake happens.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index cc4e74b731df9..5848dd295911a 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -579,6 +583,7 @@ static void rds_conn_destroy_fini(struct kref *kref)
>  	struct rds_connection *conn = container_of(kref, struct rds_connection,
>  						   c_refcount);
>  	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
> +	struct rds_transport *trans = conn->c_trans;
>  	unsigned long flags;
>  	int i;
>  
> @@ -591,7 +596,51 @@ static void rds_conn_destroy_fini(struct kref *kref)
>  	spin_lock_irqsave(&rds_conn_lock, flags);
>  	rds_conn_count--;
>  	spin_unlock_irqrestore(&rds_conn_lock, flags);
> +
> +	/* only after everything the transport module owns has been
> +	 * freed above may its unload proceed
> +	 */
> +	if (!atomic_dec_return(&trans->t_conn_count))
> +		wake_up_all(&rds_conn_freed_waitq);

[Severity: Medium]

Is it safe for this wake to happen from inside transport module text?

The final rds_conn_put() is not always made by the unloading thread.
rds_ib_connect_complete() in net/rds/ib_cm.c calls rds_conn_destroy() when
the peer negotiated an unsupported protocol version, and later in the series
rds_rdma_cm_event_handler_cmn() drops its own connection reference in its
out: epilogue.  Both of those run in rds_rdma.ko text:

RDMA_CM_EVENT_ESTABLISHED
  rds_rdma_cm_event_handler_cmn()
    rds_ib_connect_complete()
      rds_conn_destroy()
        rds_conn_put()
          rds_conn_destroy_fini()
            atomic_dec_return(&trans->t_conn_count) == 0
            wake_up_all(&rds_conn_freed_waitq)

If that put is the last one, the rmmod thread parked in
rds_conn_wait_conns_freed() is released while the releasing thread is still
executing and returning through rds_rdma text (at this commit it still has a
mutex_unlock(&conn->c_cm_lock) to do).  Nothing appears to keep that thread
out of the module image, so can rds_ib_exit() finish and free_module()
unmap the text under the frame that is still unwinding?

Is a quiesce of the transport's own asynchronous contexts needed after the
count reaches zero, so the last decrement is guaranteed to be the last
transport instruction executed?

> +}
> +
> +/* Wait for all of @trans's connections to be freed; the free runs
> + * asynchronously once rds_conn_destroy() has quiesced a connection.
> + * Called on transport module unload, after the transport has destroyed
> + * all of its connections.  A connection reference can be held for an
> + * application-controlled time - an unread datagram pins the inc that
> + * carries it, and thus the connection - so the wait is unbounded: the
> + * frees that run after unload call into this module's text (conn_free,
> + * inc_free) and free into its slabs, so proceeding while any remain
> + * would be a use-after-free, not a leak.  Warn periodically so a stuck
> + * count is diagnosable, but never stop waiting.  This matches the
> + * historical RDS contract that teardown does not discard queued data.
> + */

[Severity: Low]

This isn't a bug introduced by this patch, but the comment describes a
property the tree does not have yet at this commit.  It says "an unread
datagram pins the inc that carries it, and thus the connection", and that
the frees covered by the wait include inc_free and the transport's slabs.

At this commit t_conn_count counts connections only, and no inc holds a
connection reference, so an outstanding inc does not keep the predicate
false.  rds_ib_exit() can therefore return from the wait and go on to
rds_ib_recv_exit(), which only warns before destroying the slabs:

net/rds/ib_recv.c:rds_ib_recv_exit() {
	WARN_ON(atomic_read(&rds_ib_allocation));

	kmem_cache_destroy(rds_ib_incoming_slab);
	kmem_cache_destroy(rds_ib_frag_slab);
}

The later patch "net/rds: hold a connection reference from struct
rds_incoming" adds rds_conn_get() to rds_inc_init()/rds_inc_path_init(),
which is what makes the comment true, so this is only a matter of the
comment running ahead of the code.  Would it read better with the inc part
moved to that patch?

> +void rds_conn_wait_conns_freed(struct rds_transport *trans,
> +			       void (*resweep)(void))
> +{
> +	unsigned long warn_interval =
> +			msecs_to_jiffies(RDS_CONN_FREE_WARN_INTERVAL_MS);
> +	unsigned long warn_at = jiffies + warn_interval;
> +
> +	while (!wait_event_timeout(rds_conn_freed_waitq,
> +				   !atomic_read(&trans->t_conn_count),
> +				   msecs_to_jiffies(RDS_CONN_FREE_POLL_MS))) {

[Severity: Medium]

wait_event_timeout() sleeps in TASK_UNINTERRUPTIBLE, and this loop has no
bound, no kill/signal check and no error return, so the only way out is
t_conn_count reaching zero.

The callers are module exit functions, reached after try_stop_module() has
already set MODULE_STATE_GOING:

delete_module()
  mod->exit()
    rds_tcp_exit()  -> rds_conn_wait_conns_freed(&rds_tcp_transport, NULL)
    rds_ib_exit()   -> rds_conn_wait_conns_freed(&rds_ib_transport, ...)
    rds_loop_exit() -> rds_conn_wait_conns_freed(&rds_loop_transport, NULL)

As the commit message itself notes, the count can be held up by an
unprivileged local process that simply leaves a datagram unread, on a socket
that may belong to a different transport and therefore pins no module
reference:

    "an unread datagram pins its connection for as long as the application
     leaves it on the receive queue, and rds_find_bound() does not filter
     on transport, so that socket may well belong to a different transport
     than the connection and pin nothing that stops this unload"

In that case the rmmod thread stays in unkillable D state, the remaining
exit steps (rds_trans_unregister(), rds_tcp_recv_exit(), rds_ib_mr_exit())
never run, the module cannot be reloaded because try_module_get() fails on a
GOING module, and the new pr_warn() repeats every ten seconds for as long as
the machine is up.  For rds_loop_exit() the module in question is the core
rds module.

Since trans->t_owner is already used with try_module_get()/module_put() in
rds_trans_get()/rds_trans_get_preferred(), would holding a module reference
per live connection or inc be preferable, so delete_module() fails with
-EBUSY instead of blocking forever?  Failing that, could the wait at least
be killable or freezer aware?

> +		/* A transport whose teardown is asynchronous (IB moves a
> +		 * connection off its device from the shutdown work) gives
> +		 * us a resweep to destroy what has arrived since.
> +		 */
> +		if (resweep)
> +			resweep();
> +		if (time_after_eq(jiffies, warn_at)) {
> +			pr_warn("RDS/%s: still waiting for %d connection(s) to be freed before unload\n",
> +				trans->t_name,
> +				atomic_read(&trans->t_conn_count));
> +			warn_at = jiffies + warn_interval;
> +		}
> +	}
>  }
> +EXPORT_SYMBOL_GPL(rds_conn_wait_conns_freed);
>  
>  void rds_conn_get(struct rds_connection *conn)
>  {
> diff --git a/net/rds/ib.c b/net/rds/ib.c
> index 9fe3b9951bd32..3fc2de9d19d55 100644
> --- a/net/rds/ib.c
> +++ b/net/rds/ib.c
> @@ -537,7 +537,24 @@ void rds_ib_exit(void)
>  	rds_info_deregister_func(RDS6_INFO_IB_CONNECTIONS, rds6_ib_ic_info);
>  #endif
>  	rds_ib_unregister_client();
> +
> +	/* rds_ib_dev_shutdown() only dropped the connections still
> +	 * attached to a device; each moves itself to ib_nodev_conns
> +	 * from its shutdown work.  Destroy what is there now and keep
> +	 * sweeping the list while the wait sees connections outstanding,
> +	 * so a late arrival is destroyed rather than waited on forever.
> +	 */
>  	rds_ib_destroy_nodev_conns();
> +	rds_conn_wait_conns_freed(&rds_ib_transport,
> +				  rds_ib_destroy_nodev_conns);

[Severity: High]

Is rds_ib_destroy_nodev_conns() safe to call more than once?  It moves the
global list with a plain list_splice() and never re-initialises the head:

net/rds/ib_rdma.c:rds_ib_destroy_nodev_conns() {
	/* avoid calling conn_destroy with irqs off */
	spin_lock_irq(&ib_nodev_conns_lock);
	list_splice(&ib_nodev_conns, &tmp_list);
	spin_unlock_irq(&ib_nodev_conns_lock);

	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
		rds_conn_destroy(ic->conn);
}

rds_loop_exit() in the same tree does the reset that is missing here:

	spin_lock_irq(&loop_conns_lock);
	list_splice(&loop_conns, &tmp_list);
	INIT_LIST_HEAD(&loop_conns);
	spin_unlock_irq(&loop_conns_lock);

After the first non-empty sweep, ib_nodev_conns.next and .prev still point
at entries that now live on the previous sweep's stack-local tmp_list, and
those rds_ib_connection objects are freed asynchronously by
rds_ib_conn_free() when the last connection reference goes away.

The resweep runs on every 100 ms poll for as long as t_conn_count is
non-zero, so a second call is the expected case here.  On that call
list_empty(&ib_nodev_conns) is false, so list_splice() writes through
first->prev and last->next into rds_ib_connection memory that may already be
kfree()d, and the walk then follows a chain whose tail points at the tmp_list
of a stack frame that has already returned.  Can this corrupt memory?

In the other direction, rds_ib_conn_free() does:

net/rds/ib_cm.c:rds_ib_conn_free() {
	spin_lock_irq(lock_ptr);
	list_del(&ic->ib_node);
	spin_unlock_irq(lock_ptr);

with ib_node.prev being the address of the returned sweep's stack frame,
which is the region rds_conn_wait_conns_freed() and the next sweep are now
using.

And a genuinely late migration does:

net/rds/ib_rdma.c:rds_ib_remove_conn() {
	list_add_tail(&ic->ib_node, &ib_nodev_conns);

which links through the stale head->prev, so the new entry can end up off
the chain reachable from head->next.  Would the resweep then never find it,
leaving the unbounded wait above running forever - the exact case the
resweep was added for?

Would list_splice_init() (or an INIT_LIST_HEAD() after the splice, as
rds_loop_exit() does) address this?

As an additional note, the later series patch "net/rds: unlink transport
nodes before a possibly deferred connection free" adds a per-entry
list_del_init() to this loop, which removes the stack-linkage part, but
ib_nodev_conns is still left pointing at the moved entries.  The second
sweep then re-splices self-linked detached nodes, where
tmp_list.next == &C->ib_node and C->ib_node.next == &C->ib_node, so does
list_for_each_entry_safe() loop on the same entry indefinitely, calling
rds_conn_destroy() on it each time?

> +
> +	/* Tearing down the last connection may have dropped the final
> +	 * reference on a device, deferring rds_ib_dev_free() to rds_wq.
> +	 * Drain it before the module goes away; it queues nothing
> +	 * further on rds_wq.
> +	 */
> +	flush_workqueue(rds_wq);
> +
>  	rds_ib_sysctl_exit();
>  	rds_ib_recv_exit();
>  	rds_trans_unregister(&rds_ib_transport);
> diff --git a/net/rds/loop.c b/net/rds/loop.c
> index e6b0750bbedab..fd774f8080d01 100644
> --- a/net/rds/loop.c
> +++ b/net/rds/loop.c
> @@ -195,6 +195,8 @@ void rds_loop_exit(void)
>  		WARN_ON(lc->conn->c_passive);
>  		rds_conn_destroy(lc->conn);
>  	}
> +
> +	rds_conn_wait_conns_freed(&rds_loop_transport, NULL);
>  }
>  
>  static void rds_loop_kill_conns(struct net *net)

[ ... ]

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

Re: [PATCH net-next v2 5/9] net/rds: unlink transport nodes before a possibly deferred connection free

[netdev-bot+sashiko]

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

Critical: 0 · High: 2 · Medium: 1 · Low: 1

- [High] rds_ib_destroy_nodev_conns() (net/rds/ib_rdma.c:161) moves the
  global nodev list with list_splice(&ib_nodev_conns, &tmp_list) at line…
- [High] The fix is incomplete: the teardown helpers still walk the
  gathered stack list and dereference the transport node without holding…
- [Medium] The new list_del_init(&ic->ib_node) in
  rds_ib_destroy_nodev_conns() (net/rds/ib_rdma.c:178) leaves a still-…
- [Low] Commit message describes a live memory-corruption bug that is not
  reachable at this commit.

--

net/rds: unlink transport nodes before a possibly deferred connection free

The transport teardown helpers gather per-connection transport nodes onto a
list head on their own stack and call rds_conn_destroy() for each.  This
patch moves the node unlink out of the transport conn_free() callbacks and
into the teardown loops, under the transport lock, so that nothing is left
on the stack list once the helper returns.

A few questions on the change and on the commit message below.

Starting with the commit message:

> That was fine while rds_conn_destroy() freed the connection before it
> returned.  Once the free is governed by the connection's reference
> count, a holder that outlives the teardown loop - a socket's cached
> rs_conn, an inc parked on a receive queue - defers conn_free() until
> after the helper has returned, and the list_del() then writes the
> neighbours' pointers into a stack frame that no longer exists.

[Severity: Low]
Is that holder reachable at this commit?  rds_conn_get() has no callers in
net/rds at this revision, so the reference taken by __rds_conn_create() is
the only one and the rds_conn_put() at the end of rds_conn_destroy():

net/rds/connection.c:rds_conn_destroy() {
	...
	/* drop the initial reference; the connection is freed from
	 * rds_conn_destroy_fini() once every holder has dropped theirs
	 */
	rds_conn_put(conn);
}

is always the last one.  rds_conn_destroy_fini() -> rds_conn_path_free() ->
trans->conn_free() therefore still runs inside the teardown loop here, and
no list_del() can land on a dead frame yet.

The named holders (a socket's cached rs_conn, an inc on a receive queue)
only appear with the later patches in the series that add rds_conn_get()
calls in recv.c and send.c.

Could the message say that this is a prerequisite for those patches rather
than a fix for a currently reachable corruption?  As written, a backporter
would read it as a standalone fix, and there is no Fixes: tag to anchor it.

> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 4feb0edc360c8..de5759c50b89a 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -1282,7 +1282,9 @@ void rds_ib_conn_free(void *arg)
>  	lock_ptr = ic->rds_ibdev ? &ic->rds_ibdev->spinlock : &ib_nodev_conns_lock;
>  
>  	spin_lock_irq(lock_ptr);
> -	list_del(&ic->ib_node);
> +	/* already unlinked if a transport teardown gathered us first */
> +	if (!list_empty(&ic->ib_node))
> +		list_del(&ic->ib_node);
>  	spin_unlock_irq(lock_ptr);

[Severity: Medium]
This makes an empty ib_node mean "already detached", but two assertions in
ib_rdma.c still treat an empty ib_node on a live connection as impossible:

net/rds/ib_rdma.c:rds_ib_add_conn() {
	spin_lock_irq(&ib_nodev_conns_lock);
	BUG_ON(list_empty(&ib_nodev_conns));
	BUG_ON(list_empty(&ic->ib_node));
	list_del(&ic->ib_node);
	...
}

net/rds/ib_rdma.c:rds_ib_remove_conn() {
	spin_lock_irq(&rds_ibdev->spinlock);
	BUG_ON(list_empty(&ic->ib_node));
	list_del(&ic->ib_node);
	...
}

After the sweep in rds_ib_destroy_nodev_conns() does list_del_init(), the ic
is still live: c_destroy_in_prog is only set once rds_conn_destroy() is
entered, and the path workers have not been quiesced yet.  A connect worker
already running on that connection's own cp_wq (allocated per connection in
__rds_conn_create(), so not drained by the flush_workqueue(rds_wq) in
rds_ib_unregister_client()) reaches rds_ib_conn_path_connect() ->
rds_ib_setup_qp() -> rds_ib_add_conn() and would hit the first BUG_ON.

If instead the connect wins and moves the node onto rds_ibdev->conn_list,
the sweep empties it again and the following rds_ib_conn_path_shutdown() ->
rds_ib_remove_conn() hits the second BUG_ON.

TCP sidesteps this by carrying an explicit t_tcp_node_detached flag rather
than overloading list emptiness.  Would an equivalent flag, or dropping the
two BUG_ONs, be preferable for IB?

> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index db7e92e7bd29f..b30f2a3715878 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c
> @@ -168,8 +168,18 @@ void rds_ib_destroy_nodev_conns(void)
>  	list_splice(&ib_nodev_conns, &tmp_list);
>  	spin_unlock_irq(&ib_nodev_conns_lock);
>  
> -	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
> +	/* rds_conn_destroy() can return before the connection is freed,
> +	 * and it is the free - rds_ib_conn_free() - that unlinks ib_node.
> +	 * tmp_list lives on this stack frame, so unlink each node before
> +	 * its destroy; the free then finds it empty and leaves it alone.
> +	 */
> +	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node) {
> +		spin_lock_irq(&ib_nodev_conns_lock);
> +		list_del_init(&ic->ib_node);
> +		spin_unlock_irq(&ib_nodev_conns_lock);
> +
>  		rds_conn_destroy(ic->conn);
> +	}
>  }

[Severity: High]
Does the splice here leave ib_nodev_conns pointing at the gathered nodes?
There is no INIT_LIST_HEAD(&ib_nodev_conns) after the list_splice(), unlike
the loopback side in this same patch:

net/rds/loop.c:rds_loop_exit() {
	spin_lock_irq(&loop_conns_lock);
	list_splice(&loop_conns, &tmp_list);
	INIT_LIST_HEAD(&loop_conns);
	spin_unlock_irq(&loop_conns_lock);
	...
}

list_splice() only relinks the entries, so ib_nodev_conns.next still points
at the first gathered ib_node and .prev at the last, and the new
list_del_init() repairs tmp_list only - it leaves each gathered node
self-linked and never touches the global head.

rds_ib_exit() then calls this helper again as the resweep callback:

net/rds/ib.c:rds_ib_exit() {
	...
	rds_ib_destroy_nodev_conns();
	rds_conn_wait_conns_freed(&rds_ib_transport,
				  rds_ib_destroy_nodev_conns);
	...
}

and rds_conn_wait_conns_freed() invokes resweep() on every poll timeout
while t_conn_count is non-zero:

net/rds/connection.c:rds_conn_wait_conns_freed() {
	while (!wait_event_timeout(rds_conn_freed_waitq,
				   !atomic_read(&trans->t_conn_count),
				   msecs_to_jiffies(RDS_CONN_FREE_POLL_MS))) {
		if (resweep)
			resweep();
	...
}

On that second call, does __list_splice() write first->prev / last->next
into rds_ib_connection objects that rds_ib_conn_free() may already have
kfree()d?  And with the first gathered node self-linked, does
list_for_each_entry_safe() compute n == pos on every pass, so the sweep
never terminates and keeps calling rds_conn_destroy(ic->conn) on a conn
pointer read out of freed memory?

Separately, a late arrival through the stale head:

net/rds/ib_rdma.c:rds_ib_remove_conn() {
	...
	list_add_tail(&ic->ib_node, &ib_nodev_conns);
	...
}

dereferences ib_nodev_conns.prev, which after the sweep points at a
gathered node.  Is that write safe once that node has been freed?

> diff --git a/net/rds/tcp.c b/net/rds/tcp.c
> index 826e4629e4eee..a71d6a4f0939c 100644
> --- a/net/rds/tcp.c
> +++ b/net/rds/tcp.c
> @@ -502,6 +502,28 @@ static bool rds_tcp_is_unloading(struct rds_connection *conn)
>  	return atomic_read(&rds_tcp_unloading) != 0;
>  }
>  
> +/* Destroy the connections whose nodes were gathered on @tmp_list.
> + *
> + * rds_conn_destroy() can return before the connection is freed, and
> + * it is the free - rds_tcp_conn_free() - that unlinks t_tcp_node.
> + * Since @tmp_list lives on the caller's stack, unlink each node here
> + * and mark it detached before its destroy, so that a free that runs
> + * after the caller has returned does not write into a dead frame.
> + */
> +static void rds_tcp_destroy_gathered_conns(struct list_head *tmp_list)
> +{
> +	struct rds_tcp_connection *tc, *_tc;
> +
> +	list_for_each_entry_safe(tc, _tc, tmp_list, t_tcp_node) {
> +		spin_lock_irq(&rds_tcp_conn_lock);
> +		list_del_init(&tc->t_tcp_node);
> +		tc->t_tcp_node_detached = true;
> +		spin_unlock_irq(&rds_tcp_conn_lock);
> +
> +		rds_conn_destroy(tc->t_cpath->cp_conn);
> +	}
> +}

[Severity: High]
Does this still leave the not-yet-visited entries exposed?  Only the node
the cursor is currently on gets detached; every other tc parked on tmp_list
still has t_tcp_node_detached == false, so a concurrent last put takes the
list_del() path here and frees the object:

net/rds/tcp.c:rds_tcp_conn_free() {
	spin_lock_irqsave(&rds_tcp_conn_lock, flags);
	if (!tc->t_tcp_node_detached)
		list_del(&tc->t_tcp_node);
	spin_unlock_irqrestore(&rds_tcp_conn_lock, flags);

	kmem_cache_free(rds_tcp_conn_slab, tc);
}

The walk holds no connection reference, so can the iterator then read
list_next_entry() out of freed slab memory, write list_del_init() and
tc->t_tcp_node_detached into it, and call rds_conn_destroy() on a conn
pointer fetched from it?

There also looks to be a window on the current node: after list_del_init()
nothing pins tc, so can a racing last put free it before
tc->t_cpath->cp_conn is evaluated on the next statement?

rds_loop_destroy_gathered_conns() in loop.c and the reworked
rds_ib_destroy_nodev_conns() in ib_rdma.c walk their stack lists the same
way, with lc->conn and ic->conn read after the unlink.

The freeing side seems reachable for a gathered connection whose destroy
already ran - rds_conn_destroy() notes this itself:

net/rds/connection.c:rds_conn_destroy() {
	...
	 * a conn can be handed to rds_conn_destroy() more than once -
	 * e.g. dropped for a protocol version mismatch and then found
	 * again at module unload.
	...
}

For such a connection the free is governed entirely by a remaining holder,
which can drop it at any point during the walk.  Would taking
rds_conn_get() on each gathered connection under the transport lock and
putting it after rds_conn_destroy() cover both the current and the pending
entries?

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

Re: [PATCH net-next v2 6/9] net/rds: hold connection references in lookup, sockets and c_passive

[netdev-bot+sashiko]

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

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

- [High] Linked-list corruption in cp_send_queue because
  rds_conn_path_quiesce() mutates it without holding cp_lock, racing…
- [Medium] rds_sendmsg() samples rs->rs_tos outside rs_lock when it
  creates a connection and then installs that connection into…
- [Low] The SIOCRDSSETTOS writer of rs->rs_tos was moved from the global
  rds_sock_lock to rs->rs_lock, but the SIOCRDSGETTOS reader a few lines…
- [Low] The reference-ownership contract comment for rds_conn_lookup()
  ends up documenting the wrong function.
- [Low] struct rds_sock in net/rds/rds.h still documents rs_conn only as
  "rds_sendmsg caches the conn it used the last time around.

Pre-existing issues:
- [Low] cm_id->context still carries no connection reference after this
  patch: rds_ib_cm_handle_connect() publishes conn through…

--

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

This makes rds_conn_lookup() and __rds_conn_create() hand back a held
reference, gives the rs->rs_conn sendmsg cache and parent->c_passive
references of their own, and serializes the rs_conn cache under rs_lock.
c_passive becomes RCU-annotated, and the SIOCRDSSETTOS ioctl is switched
from the global rds_sock_lock to rs_lock.

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

> diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
> index d5defe9172e36..63baac8aeb2d1 100644
> --- a/net/rds/af_rds.c
> +++ b/net/rds/af_rds.c

[ ... ]

> @@ -267,13 +276,18 @@ 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;
>  	case SIOCRDSGETTOS:
>  		spin_lock_bh(&rds_sock_lock);

[Severity: Low]
The SIOCRDSSETTOS store of rs->rs_tos moves to rs->rs_lock here, but the
SIOCRDSGETTOS branch just below still reads it under the global lock:

	case SIOCRDSGETTOS:
		spin_lock_bh(&rds_sock_lock);
		tos = rs->rs_tos;
		spin_unlock_bh(&rds_sock_lock);

After this change the writer holds only rs_lock and the reader holds only
rds_sock_lock, so the two ioctl branches no longer exclude each other and
the rds_sock_lock in the getter no longer protects anything.

rs_tos is a u8, so the value returned is either the old or the new one,
but should the getter take rs->rs_lock as well, or read rs_tos with
READ_ONCE() and drop the lock?

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 5848dd295911a..1ec141d6faa38 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -81,7 +81,18 @@ static struct hlist_head *rds_conn_bucket(const struct in6_addr *laddr,
>  		var |= RDS_INFO_CONNECTION_FLAG_##suffix;	\
>  } while (0)
>  
> -/* rcu read lock must be held or the connection spinlock */
> +/* rcu read lock must be held or the connection spinlock.
> + * On success a reference is taken on the returned connection; the
> + * caller must drop it with rds_conn_put().
> + */
> +/* c_passive is written under rds_conn_lock and read under RCU */
> +static struct rds_connection *
> +rds_conn_passive_locked(struct rds_connection *conn)
> +{
> +	return rcu_dereference_protected(conn->c_passive,
> +					 lockdep_is_held(&rds_conn_lock));
> +}
> +
>  static struct rds_connection *rds_conn_lookup(struct net *net,
>  					      struct hlist_head *head,
>  					      const struct in6_addr *laddr,

[Severity: Low]
Did the new helper end up in between the contract comment and the
function it describes?

As written, the comment saying "rcu read lock must be held or the
connection spinlock" and "On success a reference is taken on the returned
connection; the caller must drop it with rds_conn_put()" now sits directly
above rds_conn_passive_locked(), which takes no reference and requires
rds_conn_lock specifically via
rcu_dereference_protected(conn->c_passive, lockdep_is_held(&rds_conn_lock)).

rds_conn_lookup(), the function whose contract this patch actually changes
to return a held reference, is left with no comment of its own.

Both existing callers of rds_conn_passive_locked() hold rds_conn_lock and
do not put the result, i.e. the opposite of what the comment above it now
says. Would moving the reference-ownership comment back down to
rds_conn_lookup() be clearer?

[ ... ]

> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index de5759c50b89a..01e4b4be979d2 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -924,8 +924,14 @@ 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);
> +		/* The conn stays reachable through cm_id->context
> +		 * without a reference of its own: connection destroy
> +		 * shuts the cm_id down before the conn is freed.
> +		 */
> +		rds_conn_put(conn);
> +	}
>  	if (err)
>  		rdma_reject(cm_id, &err, sizeof(int),
>  			    IB_CM_REJ_CONSUMER_DEFINED);

[Severity: Low]
This isn't a bug introduced by this patch, but does this comment agree
with the commit message?

The comment states the conn is safe without a reference because
"connection destroy shuts the cm_id down before the conn is freed", while
the commit message says the opposite is still true for now:

    cm_id->context still carries no reference of its own after this
    patch; the following patch pins the connection for the duration of
    the CM event handler.

The bare cm_id->context pointer predates this commit, and this hunk
narrows the window by holding a reference for the body of
rds_ib_cm_handle_connect(). The remaining consumers,
rds_ib_cm_initiate_connect() and rds_rdma_cm_event_handler_cmn(), read
cm_id->context and hold conn->c_cm_lock (which lives in the connection's
c_path array) across the transport callbacks without pinning the conn, and
the following patch in the series adds rds_conn_get_unless_zero() /
rds_conn_put() around exactly that.

Could the comment be reworded as a known temporary gap closed by the next
patch, so that someone bisecting or backporting this commit alone is not
misled?

[ ... ]

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 32c411d10e3ef..1ae1f24c24e8c 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);
>  	} 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,

[Severity: High]
With rds_sendmsg() now holding its own reference for the duration of
the call, the commit message is explicit that a connection can be
destroyed under a live sender and that "it is only quiesced; the free
is held off by the sender's reference".  That is the window I am
worried about: the free is indeed held off, but the quiesce itself
mutates state the sender is still using.

rds_conn_destroy() calls rds_conn_path_quiesce() for each path, which
walks and empties cp->cp_send_queue with list_for_each_entry_safe() /
list_del_init(&rm->m_conn_item) and no lock at all, while
rds_send_queue_rm() in net/rds/send.c does

	spin_lock(&cp->cp_lock);
	...
	list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
	set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
	spin_unlock(&cp->cp_lock);

on the same list head.  Two writers to one list with no common lock is
linked-list corruption, and the rds_destroy_pending(conn) test
rds_sendmsg() does before queueing does not close it: nothing holds
cp_lock across that test and the later list_add_tail(), and a
non-blocking sender can sit in wait_event_interruptible_timeout()
waiting for queue space and only wake up to queue its rm after
rds_conn_path_quiesce() has already drained the queue.  The mildest
outcome is an rm stranded on a queue nobody will ever drain, leaking
the message and its RDS_MSG_ON_CONN reference; the
BUG_ON(!list_empty(&cp->cp_retrans)) that rds_conn_destroy() runs
immediately after the quiesce loop is one plausible way for the
damage to become visible.

This is pre-existing in the sense that rds_conn_destroy() has always
run without excluding senders, but before this patch a sender could
not legitimately keep operating on a conn whose destroy had begun -
this patch makes that a documented, expected state, so the locking
ought to be made to match.

Should rds_conn_path_quiesce() splice cp->cp_send_queue under
cp->cp_lock and drop the message references outside the lock, and does
rds_send_queue_rm() additionally need to re-test
rds_destroy_pending(cp->cp_conn) under cp_lock so that no rm can be
queued after the drain has run?  Note the quiesce also frees
cp->cp_xmit_rm, which is owned by whoever holds RDS_IN_XMIT, so some
coordination with the xmit path looks necessary as well.  What is the
intended locking rule for cp_send_queue during teardown?

[Severity: Medium]
Is the SIOCRDSSETTOS window really closed by the rs_lock conversion?

rs->rs_tos is sampled here with no lock held, as an argument to
rds_conn_create_outgoing(), and that call can sleep in
kmem_cache_zalloc() / alloc_ordered_workqueue().

Starting from a socket with rs_tos == 0 and rs_conn == NULL:

thread A, rds_sendmsg()
	spin_lock_irqsave(&rs->rs_lock, flags);
	conn = rs->rs_conn;		/* NULL */
	spin_unlock_irqrestore(&rs->rs_lock, flags);
	conn = rds_conn_create_outgoing(..., rs->rs_tos /* 0 */, ...);

thread B, rds_ioctl(SIOCRDSSETTOS)
	spin_lock_irqsave(&rs->rs_lock, flags);
	if (rs->rs_tos || rs->rs_conn) {	/* both still 0/NULL */
		...
	}
	rs->rs_tos = tos;		/* succeeds */

thread A then publishes and uses the conn it created with c_tos == 0:

	rds_conn_get(conn);
	spin_lock_irqsave(&rs->rs_lock, flags);
	old = rs->rs_conn;
	rs->rs_conn = conn;
	spin_unlock_irqrestore(&rs->rs_lock, flags);

The install path never re-reads rs->rs_tos, so userspace is told the new
ToS was accepted while the in-flight datagram goes out on the old traffic
class, and rs_conn transiently holds a connection whose c_tos disagrees
with rs_tos until the next sendmsg fails the cache-hit test.

That makes the comment added to rds_ioctl() ("so a racing sendmsg cannot
cache a conn whose c_tos then disagrees with rs_tos") and the matching
commit message claim read as stronger than what the code enforces. Would
re-checking rs_tos == conn->c_tos under rs_lock at install time (or
capturing rs_tos under rs_lock and rejecting the install on mismatch)
close this?

[Severity: Low]
Should struct rds_sock in net/rds/rds.h be updated along with this?

rs_conn is still described only as a lookup cache, and it sits above the
block that declares what rs_lock covers:

	/*
	 * rds_sendmsg caches the conn it used the last time around.
	 * This helps avoid costly lookups.
	 */
	struct rds_connection	*rs_conn;

	/* flag indicating we were congested or not */
	int			rs_congested;
	/* seen congestion (ENOBUFS) when sending? */
	int			rs_seen_congestion;

	/* rs_lock protects all these adjacent members before the newline */
	spinlock_t		rs_lock;

After this patch rs_conn is serialized by rs->rs_lock in rds_sendmsg() and
rds_ioctl(), and it owns a connection reference dropped in rds_release()
and when the cache is replaced, but neither fact is recorded in the
header. Following the annotation as it stands would suggest rs_conn is an
unlocked, non-owning cache.

[ ... ]

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

Re: [PATCH net-next v2 7/9] net/rds: pin the connection across RDMA-CM event handling

[netdev-bot+sashiko]

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

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

- [Low] Commit message misattributes a pre-existing use-after-free to the
  kref split, and never names the triggering callback.

Pre-existing issues:
- [High] Self-deadlock on the very callback path this patch relies on.
- [High] In the function this patch modifies, struct rds_transport
  *trans; (net/rds/rdma_transport.c:55) is assigned only under `if…

--

net/rds: pin the connection across RDMA-CM event handling

This adds a reference on the connection that rds_rdma_cm_event_handler_cmn()
picks up from cm_id->context, held for the duration of the handler, and skips
events for connections that are already being freed.  It also re-checks
rds_destroy_pending() under c_cm_lock in rds_ib_cm_handle_connect() so a
connect request cannot install a new cm_id and QP on a quiesced connection.

No Link: tags in the commit header.

A few questions below, mostly about the surrounding code that the new
reference is meant to protect.

First, on the changelog itself:

> Now that a connection is freed by its last reference rather than by
> rds_conn_destroy() itself, a callback that drops the last reference other
> than the handler's implicit one leaves the final mutex_unlock() running on
> freed memory.

[Severity: Low]

Is the hazard really new to this series?  In the baseline, rds_conn_destroy()
ended with:

	kfree(conn->c_path);
	kmem_cache_free(rds_conn_slab, conn);

so the connection was freed synchronously, and the handler's
mutex_unlock(&conn->c_cm_lock) at out: already ran on freed memory whenever a
callback destroyed the conn - unconditionally, not only when the last
reference happened to be dropped.

Also, which callback is meant here?  The only handler-reachable one seems to
be rds_ib_cm_connect_complete() on RDMA_CM_EVENT_ESTABLISHED, when the peer
advertises a protocol version below 3.1.  Could the changelog name that
trigger, so backporters know what to look for?

And what is "the handler's implicit one" reference?  Before this patch
cm_id->context carries no reference at all.

> diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> index b15cf316b23a2..584e9867810f4 100644
> --- a/net/rds/rdma_transport.c
> +++ b/net/rds/rdma_transport.c
> @@ -63,6 +63,18 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
>  	if (cm_id->device->node_type == RDMA_NODE_IB_CA)
>  		trans = &rds_ib_transport;
>  

[Severity: High]

This isn't a bug introduced by this patch, but while looking at these lines:
can trans stay uninitialized here?  In
rds_rdma_cm_event_handler_cmn() it is a plain local:

	struct rds_transport *trans;
	...
	if (cm_id->device->node_type == RDMA_NODE_IB_CA)
		trans = &rds_ib_transport;

There is no else branch and no early return, yet the CONNECT_REQUEST case
calls through it:

	case RDMA_CM_EVENT_CONNECT_REQUEST:
		ret = trans->cm_handle_connect(cm_id, event, isv6);

RDS never calls rdma_restrict_node_type(), so cma_listen_on_dev() keeps
restricted_node_type == RDMA_NODE_UNSPECIFIED and installs the RDS listener
on every RDMA device, including iWARP RNICs (siw, cxgb4, irdma, qedr and
erdma all set node_type = RDMA_NODE_RNIC).  An incoming connect on such a
device reaches iw_conn_req_handler(), which creates a child id inheriting the
listener's event_handler and NULL context, and delivers CONNECT_REQUEST here.

Since conn is NULL on that path, the new rds_conn_get_unless_zero() guard
does not change reachability, so this ends up as an indirect call through an
uninitialized stack slot driven by a remote peer.  Would an explicit else
that rejects non-IB node types be appropriate while touching this function?

> +	/* cm_id->context carries no reference of its own.  Pin the
> +	 * connection for the duration of the handler: what the callbacks
> +	 * below do may drop the last reference other than ours, and the
> +	 * mutex released at out: lives in the connection's path array.
> +	 * A connection already being freed gets no events handled.
> +	 */
> +	if (conn && !rds_conn_get_unless_zero(conn)) {
> +		rdsdebug("conn %p id %p is being freed, ignoring event\n",
> +			 conn, cm_id);
> +		return 0;
> +	}
> +

[Severity: High]

This is a pre-existing issue, not something this patch introduces, but it is
the exact callback path the comment above refers to: can the handler ever
return from that callback?

c_cm_lock is conn->c_path[0].cp_cm_lock, taken just below and held until
out:, and the ESTABLISHED case dispatches with it held:

	case RDMA_CM_EVENT_ESTABLISHED:
		if (conn)
			trans->cm_connect_complete(conn, event);

rds_ib_cm_connect_complete() takes the peer's protocol major/minor straight
from the REP private data with no validation, and since RDS_PROTOCOL_VERSION
and RDS_PROTOCOL_COMPAT_VERSION are both RDS_PROTOCOL_3_1, any advertised
version below 3.1 makes both tests true:

	if (conn->c_version < RDS_PROTOCOL_VERSION) {
		if (conn->c_version != RDS_PROTOCOL_COMPAT_VERSION) {
			...
			rds_conn_destroy(conn);

>From there the cycle looks like this:

rds_rdma_cm_event_handler_cmn()
  mutex_lock(&conn->c_cm_lock)
  trans->cm_connect_complete()
    rds_ib_cm_connect_complete()
      rds_conn_destroy()
        rds_conn_path_quiesce()
          rds_conn_path_drop(cp, true)   /* sets ERROR, queues cp_down_w */
          flush_work(&cp->cp_down_w)
            rds_shutdown_worker()
              rds_conn_shutdown()
                mutex_lock(&cp->cp_cm_lock)   /* held by the CM handler */

rds_conn_shutdown()'s first transition DOWN -> DOWN fails because
rds_conn_path_drop() left the state at RDS_CONN_ERROR, so it goes on to take
cp_cm_lock and blocks.  flush_work() then never returns, and the

	out:
		if (conn) {
			mutex_unlock(&conn->c_cm_lock);
			rds_conn_put(conn);
		}

that this patch adds is never reached, so the new pin can never be exercised
on this path.  The comment in rds_conn_shutdown() states the assumption that
is inverted here ("we may be deadlocking with the CM handler ... the CM event
handler is supposed to check for state DISCONNECTING") - here the CM handler
is the one starting the shutdown while holding the lock.

Should rds_ib_cm_connect_complete() defer the destroy (for example via
rds_conn_path_drop() without the synchronous flush) instead of calling
rds_conn_destroy() under c_cm_lock?

>  	/* Prevent shutdown from tearing down the connection
>  	 * while we're executing. */
>  	if (conn) {

[ ... ]

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

Re: [PATCH net-next v2 8/9] net/rds: drop rds_conn_count in favor of t_conn_count

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 1

- [Low] Changelog inaccuracy in "net/rds: drop rds_conn_count in favor of
  t_conn_count": the commit message states that removing the…

--

net/rds: drop rds_conn_count in favor of t_conn_count

This removes the global rds_conn_count now that each transport tracks its
own connections in t_conn_count, and seeds the per-path workqueue names
from t_conn_count instead.  The rds_conn_lock round-trip that existed only
to decrement rds_conn_count is dropped from rds_conn_destroy_fini().

> Removing the counter also removes the rds_conn_lock round-trip that
> rds_conn_destroy_fini() took solely to decrement it, leaving the
> free path lock-free.

[Severity: Low]
Is the free path really lock-free after this change?

rds_conn_destroy_fini() still calls rds_conn_path_free() for every path,
and that ends up in destroy_workqueue():

net/rds/connection.c:rds_conn_path_free() {
	if (cp->cp_wq != rds_wq) {
		destroy_workqueue(cp->cp_wq);
		cp->cp_wq = NULL;
	}
	...
}

destroy_workqueue() drains the queue and takes wq_pool_mutex, so it can
sleep.  The tail of rds_conn_destroy_fini() also takes the wait-queue
spinlock via wake_up_all(&rds_conn_freed_waitq).  What this patch removes
is the last rds_conn_lock acquisition, not all locking.

The concern with the current wording is that a later reader or a
backporter could take "lock-free" to mean the final rds_conn_put() may be
dropped from atomic, IRQ or softirq context, which this path cannot
tolerate because destroy_workqueue() may sleep.

The last patch of this series, "net/rds: hold a connection reference from
struct rds_incoming", states the opposite property:

  "The final rds_conn_put() runs the free path, which destroys the
   per-path workqueues and therefore may sleep, so the last reference has
   to be dropped from process context"

Would something like "removes the last rds_conn_lock round-trip from the
free path" be more accurate here?

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 1ec141d6faa38..11813f93961d1 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -656,7 +654,6 @@ static void rds_conn_destroy_fini(struct kref *kref)
>  						   c_refcount);
>  	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
>  	struct rds_transport *trans = conn->c_trans;
> -	unsigned long flags;
>  	int i;
>  
>  	for (i = 0; i < npaths; i++)
> @@ -665,10 +662,6 @@ static void rds_conn_destroy_fini(struct kref *kref)
>  	kfree(conn->c_path);
>  	kmem_cache_free(rds_conn_slab, conn);
>  
> -	spin_lock_irqsave(&rds_conn_lock, flags);
> -	rds_conn_count--;
> -	spin_unlock_irqrestore(&rds_conn_lock, flags);
> -
>  	/* only after everything the transport module owns has been
>  	 * freed above may its unload proceed
>  	 */

The code change itself looks fine; only the changelog wording seems to
overstate the result.

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

Re: [PATCH net-next v2 9/9] net/rds: hold a connection reference from struct rds_incoming

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 0 · Low: 2

- [Low] The commit fixes a reported, user-visible use-after-free (KASAN
  slab-use-after-free in rds6_inc_info_copy() via rds_info_getsockopt(),…
- [Low] In rds_recv_incoming() (net/rds/recv.c), the cross-netns
  rejection comment at lines 424-431 still states that netns teardown…

--

net/rds: hold a connection reference from struct rds_incoming

Every assignment of struct rds_incoming->i_conn now takes a connection
reference, so an inc parked on a socket receive queue (or an m_inc on a
send queue) keeps its connection allocated. The references are dropped
from rds_inc_put() and from a new rds_message_free() helper.

Closes: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/

Two questions, one about the tag block and one about a comment that the
patch appears to make stale.

>     Reported-by: Chengfeng Ye <nicoyip.dev@gmail.com>
>     Closes: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/
>     Signed-off-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>

[Severity: Low]
Should this carry a Fixes: tag?  The change closes a reported
slab-use-after-free (rds6_inc_info_copy() reached from
rds_info_getsockopt(), and the panic in rds_ib_recv_cache_put() reached
through rds_release()->rds_clear_recv_queue()->rds_ib_inc_free()), but no
originating commit is named.

git blame on the inc->i_conn = conn assignment in rds_inc_init() points at
bdbe6fbc6a2f ("RDS: recv.c"), so the origin looks identifiable.  Other
patches in this series do carry the tag, for example "net/rds: make
rds_destroy_pending() cover single-connection destroy" with Fixes:
cdc306a5c9cd3 and "net/rds: guard every work-requeueing site with
rds_destroy_pending()" with Fixes: ebeeb1ad9b8a.

The commit message says in prose:

    This is not a stable candidate: reaching the use-after-free requires
    freeing a connection out from under a live socket

That reasoning is fine, but it is not something backport tooling parses,
and the tag block on its own does not say how far back the defect reaches.

> diff --git a/net/rds/recv.c b/net/rds/recv.c
> index 6204e577a90ae..b031c0b43af83 100644
> --- a/net/rds/recv.c
> +++ b/net/rds/recv.c
> @@ -46,6 +46,7 @@ void rds_inc_init(struct rds_incoming *inc, struct rds_connection *conn,
>  {
>  	refcount_set(&inc->i_refcount, 1);
>  	INIT_LIST_HEAD(&inc->i_item);
> +	rds_conn_get(conn);	/* put in rds_inc_put() */
>  	inc->i_conn = conn;
>  	inc->i_conn_path = NULL;
>  	inc->i_saddr = *saddr;

[ ... ]

> @@ -325,6 +337,13 @@ void rds_recv_incoming(struct rds_connection *conn, struct in6_addr *saddr,
>  	unsigned long flags;
>  	struct rds_conn_path *cp;
>  
> +	/* every caller initialized @inc with rds_inc_init() or
> +	 * rds_inc_path_init() first, so i_conn already holds a reference.
> +	 * Take the new one before dropping the old, so that re-pointing an
> +	 * inc at the connection it already refers to cannot free it.
> +	 */
> +	rds_conn_get(conn);
> +	rds_conn_put(inc->i_conn);
>  	inc->i_conn = conn;
>  	inc->i_rx_jiffies = jiffies;
>  	if (conn->c_trans->t_mp_capable)

[Severity: Low]
Does this make the cross-netns comment further down in the same function
inaccurate?  That comment still reads:

net/rds/recv.c:rds_recv_incoming() {
    ...
	/*
	 * rds_find_bound() uses a global (netns-agnostic) hash table.
	 * An RDS connection created in netns A can match a socket bound
	 * in the init netns, delivering inc cross-netns with inc->i_conn
	 * pointing into netns A.  When cleanup_net() then frees that conn,
	 * any subsequent dereference of inc->i_conn is a use-after-free.
	 * Drop the inc if the receiving socket lives in a different netns.
	 */
	if (!net_eq(sock_net(rds_rs_to_sk(rs)), rds_conn_net(conn))) {
    ...
}

After this patch the inc owns a reference from rds_inc_init() or
rds_inc_path_init(), and rds_inc_put() releases it only after
->inc_free() has run:

		conn->c_trans->inc_free(inc);
		rds_conn_put(conn);

rds_conn_put() reaches kmem_cache_free() only when the kref hits zero, so
a cross-netns inc sitting on a receive queue keeps its connection
allocated across cleanup_net().

The check itself still looks wanted, since cross-netns delivery is wrong on
its own and an outliving conn leaves conn->c_net stale.  Could the comment
be reworded to state that reason rather than a use-after-free this patch
removes?

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