[PATCH net-next v3 00/13] net/rds: make connection lifetime reference-counted

[PATCH net-next v3 00/13] net/rds: make connection lifetime reference-counted

[Allison Henderson]

Hi all,

This is v3 of the connection-lifetime set (v1 at [1], v2 at [2]),
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) fixes rds_ib_conn_free() re-enabling interrupts under
  a caller's irqsave lock.

  Patch 2 (new) frees every path's transport data on the
  passive-connection exits of __rds_conn_create(), where only path 0 was
  freed.  Both are pre-existing and stand on their own; they are first
  in the series because the reference-counted teardown calls conn_free()
  from more places.

  Patch 3 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 4 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 5 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 6 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 7 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 8 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 9 (new) has rds_send_queue_rm() refuse, under cp_lock, to
  queue on a connection whose destroy has begun, and has the quiesce
  splice cp_send_queue away under the same lock: a message queued
  after the purge would hold a connection reference nothing ever
  drops.

  Patch 10 (new) has rds_tcp_accept_one() refuse to install an
  accepted socket on a connection whose destroy has begun.

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

  Patch 12 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 13 makes struct rds_incoming hold a reference on i_conn, the
  fix for the KASAN use-after-free Chengfeng Ye reported [3].
  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 5, 8, 6 and 13 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 v2 [2]:
 - New patches 1 and 2: pre-existing rds_ib_conn_free() interrupt
   state clobber and passive-path transport data leak, surfaced by
   review of the teardown changes.
 - Patch 6: rds_ib_destroy_nodev_conns() uses list_splice_init(), so
   the resweep from the unload wait cannot splice a stale list head.
 - Patch 8: SIOCRDSGETTOS reads rs_tos under rs_lock like SETTOS.
 - New patch 9: rds_send_queue_rm() refuses a connection whose destroy
   has begun, under cp_lock, and the quiesce purges cp_send_queue
   under cp_lock (list corruption against an in-flight sender, and a
   message that would pin the connection forever).
 - New patch 10: rds_tcp_accept_one() does not install a socket on a
   connection whose destroy has begun (socket left pointing at a freed
   path).

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/20260912035027.27447-1-achender@kernel.org/
[3] https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/

Allison


Allison Henderson (9):
  net/rds: ib: don't enable interrupts in rds_ib_conn_free()
  net/rds: free every path's transport data on the passive create paths
  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: refuse to queue a message on a connection being destroyed
  net/rds: tcp: don't attach an accepted socket to a connection being
    destroyed
  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         |  24 ++-
 net/rds/connection.c     | 326 +++++++++++++++++++++++++++++++++------
 net/rds/ib.c             |  22 ++-
 net/rds/ib_cm.c          |  28 +++-
 net/rds/ib_rdma.c        |  14 +-
 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           |  77 +++++++--
 net/rds/tcp.c            |  29 +++-
 net/rds/tcp_listen.c     |  25 ++-
 15 files changed, 602 insertions(+), 99 deletions(-)

-- 
2.25.1

[PATCH net-next v3 01/13] net/rds: ib: don't enable interrupts in rds_ib_conn_free()

[Allison Henderson]

rds_ib_conn_free() unlinks the connection from its device or nodev
list under spin_lock_irq()/spin_unlock_irq().  It is not only called
from the rmmod path, though: __rds_conn_create() calls
trans->conn_free() to undo a lost creation race while it still holds
rds_conn_lock, taken with spin_lock_irqsave().  The unconditional
spin_unlock_irq() then re-enables interrupts with rds_conn_lock held
and leaves them enabled when the caller's spin_unlock_irqrestore()
runs, defeating the irqsave the caller relied on.

Use the irqsave/irqrestore pair, as rds_tcp_conn_free() and
rds_loop_conn_free() already do.

Fixes: 745cbccac3fe ("RDS: Rewrite connection cleanup, fixing oops on rmmod")
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/ib_cm.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 4feb0edc360c..118e033229aa 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -1271,6 +1271,7 @@ void rds_ib_conn_free(void *arg)
 {
 	struct rds_ib_connection *ic = arg;
 	spinlock_t	*lock_ptr;
+	unsigned long flags;
 
 	rdsdebug("ic %p\n", ic);
 
@@ -1278,12 +1279,16 @@ void rds_ib_conn_free(void *arg)
 	 * Conn is either on a dev's list or on the nodev list.
 	 * A race with shutdown() or connect() would cause problems
 	 * (since rds_ibdev would change) but that should never happen.
+	 *
+	 * Callers may hold rds_conn_lock with interrupts disabled
+	 * (__rds_conn_create() undoing a lost creation race), so do not
+	 * re-enable interrupts unconditionally here.
 	 */
 	lock_ptr = ic->rds_ibdev ? &ic->rds_ibdev->spinlock : &ib_nodev_conns_lock;
 
-	spin_lock_irq(lock_ptr);
+	spin_lock_irqsave(lock_ptr, flags);
 	list_del(&ic->ib_node);
-	spin_unlock_irq(lock_ptr);
+	spin_unlock_irqrestore(lock_ptr, flags);
 
 	rds_ib_recv_free_caches(ic);
 
-- 
2.25.1

[PATCH net-next v3 02/13] net/rds: free every path's transport data on the passive create paths

[Allison Henderson]

trans->conn_alloc() may allocate transport data for every path of a
multipath connection - rds_tcp_conn_alloc() does - which is why the
lost-creation-race exit of __rds_conn_create() loops over all npaths
when it frees the connection it just built.  The passive-connection
exit right above it, taken when a loopback parent already has its
c_passive twin, frees only path 0 and leaks the transport data of
paths 1..npaths-1.  RDS/TCP loopback is exactly a multipath passive
connection, so this is reachable.

Move the loop into a helper and use it on both exits.

Fixes: 1c5113cf796b ("RDS: Initialize all RDS_MPATH_WORKERS in __rds_conn_create")
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/connection.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index b6c4beb50eaf..a96569a3ee9a 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -161,6 +161,22 @@ static void __rds_conn_path_init(struct rds_connection *conn,
 	cp->cp_flags = 0;
 }
 
+/* Undo trans->conn_alloc(): it may have allocated transport data for
+ * every path of a multipath connection, not just for path 0.
+ */
+static void rds_conn_free_transport_data(struct rds_connection *conn,
+					 int npaths)
+{
+	struct rds_conn_path *cp;
+	int i;
+
+	for (i = 0; i < npaths; i++) {
+		cp = &conn->c_path[i];
+		if (cp->cp_transport_data)
+			conn->c_trans->conn_free(cp->cp_transport_data);
+	}
+}
+
 /*
  * There is only every one 'conn' for a given pair of addresses in the
  * system at a time.  They contain messages to be retransmitted and so
@@ -316,7 +332,7 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 	if (parent) {
 		/* Creating passive conn */
 		if (parent->c_passive) {
-			trans->conn_free(conn->c_path[0].cp_transport_data);
+			rds_conn_free_transport_data(conn, npaths);
 			free_cp = conn->c_path;
 			kmem_cache_free(rds_conn_slab, conn);
 			conn = parent->c_passive;
@@ -332,18 +348,7 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 		found = rds_conn_lookup(net, head, laddr, faddr, trans,
 					tos, dev_if);
 		if (found) {
-			struct rds_conn_path *cp;
-			int i;
-
-			for (i = 0; i < npaths; i++) {
-				cp = &conn->c_path[i];
-				/* The ->conn_alloc invocation may have
-				 * allocated resource for all paths, so all
-				 * of them may have to be freed here.
-				 */
-				if (cp->cp_transport_data)
-					trans->conn_free(cp->cp_transport_data);
-			}
+			rds_conn_free_transport_data(conn, npaths);
 			free_cp = conn->c_path;
 			kmem_cache_free(rds_conn_slab, conn);
 			conn = found;
-- 
2.25.1

[PATCH net-next v3 03/13] 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 v3 04/13] 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 a96569a3ee9a..242ca0570a47 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -579,6 +579,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 v3 05/13] 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 242ca0570a47..a44aa4d2a5e8 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -231,6 +231,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);
@@ -471,9 +472,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.
 			 */
@@ -520,10 +522,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;
 
@@ -552,6 +556,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);
@@ -561,16 +575,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);
@@ -584,11 +634,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();
@@ -596,7 +658,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));
 	}
 
@@ -607,12 +669,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 v3 06/13] 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/ib_rdma.c    |  2 +-
 net/rds/loop.c       |  2 ++
 net/rds/rds.h        | 13 ++++++++++++
 net/rds/tcp.c        |  1 +
 6 files changed, 83 insertions(+), 1 deletion(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index a44aa4d2a5e8..c3b3d756c52e 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;
 
@@ -341,6 +343,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 */
@@ -359,6 +362,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);
@@ -584,6 +588,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;
 
@@ -596,7 +601,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/ib_rdma.c b/net/rds/ib_rdma.c
index db7e92e7bd29..a9b27f06cbfc 100644
--- a/net/rds/ib_rdma.c
+++ b/net/rds/ib_rdma.c
@@ -165,7 +165,7 @@ void rds_ib_destroy_nodev_conns(void)
 
 	/* avoid calling conn_destroy with irqs off */
 	spin_lock_irq(&ib_nodev_conns_lock);
-	list_splice(&ib_nodev_conns, &tmp_list);
+	list_splice_init(&ib_nodev_conns, &tmp_list);
 	spin_unlock_irq(&ib_nodev_conns_lock);
 
 	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
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 v3 07/13] 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 118e033229aa..26a32c1ec8f7 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -1287,7 +1287,9 @@ void rds_ib_conn_free(void *arg)
 	lock_ptr = ic->rds_ibdev ? &ic->rds_ibdev->spinlock : &ib_nodev_conns_lock;
 
 	spin_lock_irqsave(lock_ptr, flags);
-	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_irqrestore(lock_ptr, flags);
 
 	rds_ib_recv_free_caches(ic);
diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
index a9b27f06cbfc..91db43a0e7d7 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_init(&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 v3 08/13] 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     |  24 +++++++--
 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, 189 insertions(+), 19 deletions(-)

diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
index d5defe9172e3..1cc20b5cfd21 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,18 +276,23 @@ static int rds_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
 		else
 			return -ENOIOCTLCMD;
 
-		spin_lock_bh(&rds_sock_lock);
+		/* rs_conn is serialized by rs_lock (see rds_sendmsg());
+		 * hold it across the "no connection yet" check and the
+		 * rs_tos store so a racing sendmsg cannot cache a conn
+		 * whose c_tos then disagrees with rs_tos.
+		 */
+		spin_lock_irqsave(&rs->rs_lock, flags);
 		if (rs->rs_tos || rs->rs_conn) {
-			spin_unlock_bh(&rds_sock_lock);
+			spin_unlock_irqrestore(&rs->rs_lock, flags);
 			return -EINVAL;
 		}
 		rs->rs_tos = tos;
-		spin_unlock_bh(&rds_sock_lock);
+		spin_unlock_irqrestore(&rs->rs_lock, flags);
 		break;
 	case SIOCRDSGETTOS:
-		spin_lock_bh(&rds_sock_lock);
+		spin_lock_irqsave(&rs->rs_lock, flags);
 		tos = rs->rs_tos;
-		spin_unlock_bh(&rds_sock_lock);
+		spin_unlock_irqrestore(&rs->rs_lock, flags);
 		if (put_user(tos, (rds_tos_t __user *)arg))
 			return -EFAULT;
 		break;
diff --git a/net/rds/connection.c b/net/rds/connection.c
index c3b3d756c52e..7ef6fb9d352b 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;
 		}
@@ -215,7 +237,20 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 		 * We need a second connection object into which we
 		 * can stick the other QP. */
 		parent = conn;
-		conn = parent->c_passive;
+		/* The c_passive pointer holds a reference which is only
+		 * dropped one synchronize_rcu() after the pointer is
+		 * cleared, so within this RCU section a fetched pointer
+		 * is always safe to take a reference on.  A passive conn
+		 * whose own destroy has begun is not handed out, though:
+		 * it is quiesced and about to clear the parent's pointer
+		 * itself, and reusing it would re-arm a connection that
+		 * nothing will tear down again.
+		 */
+		conn = rcu_dereference(parent->c_passive);
+		if (conn && READ_ONCE(conn->c_destroy_in_prog))
+			conn = NULL;
+		if (conn)
+			rds_conn_get(conn);
 	}
 	rcu_read_unlock();
 	if (conn)
@@ -334,13 +369,44 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 	spin_lock_irqsave(&rds_conn_lock, flags);
 	if (parent) {
 		/* Creating passive conn */
-		if (parent->c_passive) {
+		if (READ_ONCE(parent->c_destroy_in_prog)) {
+			/* The parent's destroy has begun (it sets the
+			 * flag and snatches c_passive under this
+			 * lock); do not install a new passive conn
+			 * that nothing would ever destroy.
+			 */
+			rds_conn_free_transport_data(conn, npaths);
+			free_cp = conn->c_path;
+			kmem_cache_free(rds_conn_slab, conn);
+			conn = ERR_PTR(-ENETDOWN);
+		} else if (rcu_access_pointer(parent->c_passive)) {
+			struct rds_connection *passive;
+
+			passive = rds_conn_passive_locked(parent);
 			rds_conn_free_transport_data(conn, npaths);
 			free_cp = conn->c_path;
 			kmem_cache_free(rds_conn_slab, conn);
-			conn = parent->c_passive;
+			if (READ_ONCE(passive->c_destroy_in_prog)) {
+				/* 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);
@@ -359,6 +425,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++;
@@ -369,6 +439,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)
@@ -671,6 +743,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);
 
@@ -701,7 +776,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 */
@@ -718,6 +824,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 26a32c1ec8f7..5a4644c40be5 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 v3 09/13] net/rds: refuse to queue a message on a connection being destroyed

[Allison Henderson]

rds_conn_path_quiesce() tears down cp_send_queue by walking it with no
lock held.  That was tolerable while a connection could only be
destroyed with no sender in flight, but a sender now holds a
reference across rds_sendmsg(), and rds_conn_destroy() can quiesce
the connection underneath it.  rds_send_queue_rm() adds to
cp_send_queue under cp_lock, so the unlocked walk races the add and
can corrupt the list.  Worse, a message added after the purge sits on
the queue of a quiesced connection holding the connection reference
rds_send_queue_rm() took for it: the reference is only dropped when
the message is freed, the message is only freed when the queue is
torn down, and the queue is only torn down by the destroy that has
already run.  The connection would never be freed, and with it the
transport could never unload.

Splice the queue away under cp_lock in the quiesce, and have
rds_send_queue_rm() test rds_destroy_pending() under that same lock
before it touches either queue.  A sender that gets there first has
its message purged; one that gets there second is refused, and
rds_sendmsg() returns -EAGAIN for it, the same result the early
rds_destroy_pending() check in rds_sendmsg() already produces for a
connection whose destroy had begun before the send started.
rds_send_queue_rm()'s *queued becomes negative on refusal so that the
wait loop in rds_sendmsg() stops waiting for send room that will never
come.

Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/connection.c | 16 ++++++++++++----
 net/rds/send.c       | 20 +++++++++++++++++++-
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index 7ef6fb9d352b..e5a8534c23cf 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -606,6 +606,8 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
 static void rds_conn_path_quiesce(struct rds_conn_path *cp)
 {
 	struct rds_message *rm, *rtmp;
+	unsigned long flags;
+	LIST_HEAD(purge);
 
 	if (!cp->cp_transport_data)
 		return;
@@ -617,10 +619,16 @@ static void rds_conn_path_quiesce(struct rds_conn_path *cp)
 	rds_conn_path_drop(cp, true);
 	flush_work(&cp->cp_down_w);
 
-	/* tear down queued messages */
-	list_for_each_entry_safe(rm, rtmp,
-				 &cp->cp_send_queue,
-				 m_conn_item) {
+	/* Tear down queued messages.  Take the queue under cp_lock:
+	 * a sender that still holds a reference can be inside
+	 * rds_send_queue_rm() right now, and it tests
+	 * rds_destroy_pending() under the same lock, so after this
+	 * splice nothing is added behind our back.
+	 */
+	spin_lock_irqsave(&cp->cp_lock, flags);
+	list_splice_init(&cp->cp_send_queue, &purge);
+	spin_unlock_irqrestore(&cp->cp_lock, flags);
+	list_for_each_entry_safe(rm, rtmp, &purge, m_conn_item) {
 		list_del_init(&rm->m_conn_item);
 		BUG_ON(!list_empty(&rm->m_sock_item));
 		rds_message_put(rm);
diff --git a/net/rds/send.c b/net/rds/send.c
index 1ae1f24c24e8..94d6ac174dde 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -928,6 +928,19 @@ static int rds_send_queue_rm(struct rds_sock *rs, struct rds_connection *conn,
 	 * and poll() now knows no more data can be sent.
 	 */
 	if (rs->rs_snd_bytes < rds_sk_sndbuf(rs)) {
+		/* rds_conn_path_quiesce() empties cp_send_queue under
+		 * cp_lock once the connection's destroy has begun.  Test
+		 * for that under the same lock, before touching either
+		 * queue: a message added after the purge would hold a
+		 * connection reference nothing ever drops.
+		 */
+		spin_lock(&cp->cp_lock);
+		if (rds_destroy_pending(conn)) {
+			spin_unlock(&cp->cp_lock);
+			*queued = -EAGAIN;
+			goto unlock;
+		}
+
 		rs->rs_snd_bytes += len;
 
 		/* let recv side know we are close to send space exhaustion.
@@ -951,7 +964,6 @@ static int rds_send_queue_rm(struct rds_sock *rs, struct rds_connection *conn,
 		rm->m_inc.i_conn_path = cp;
 		rds_message_addref(rm);
 
-		spin_lock(&cp->cp_lock);
 		rm->m_inc.i_hdr.h_sequence = cpu_to_be64(cp->cp_next_tx_seq++);
 		list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
 		set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
@@ -964,6 +976,7 @@ static int rds_send_queue_rm(struct rds_sock *rs, struct rds_connection *conn,
 		*queued = 1;
 	}
 
+unlock:
 	spin_unlock_irqrestore(&rs->rs_lock, flags);
 out:
 	return *queued;
@@ -1474,6 +1487,11 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
 			ret = -ETIMEDOUT;
 		goto out;
 	}
+	/* rds_send_queue_rm() refused: the connection is being destroyed */
+	if (queued < 0) {
+		ret = queued;
+		goto out;
+	}
 
 	/*
 	 * By now we've committed to the send.  We reuse rds_send_worker()
-- 
2.25.1

[PATCH net-next v3 10/13] net/rds: tcp: don't attach an accepted socket to a connection being destroyed

[Allison Henderson]

rds_tcp_accept_one() looks the connection up, claims a path with a
DOWN -> CONNECTING transition and installs the accepted socket on it.
A connection whose destroy has begun is quiesced - its paths are DOWN
and its old sockets released - but stays allocated while a reference
holder is still around, and rds_conn_create() hands out exactly such
a connection with a reference of its own.  The path claim then
succeeds, the socket is installed, the accept drops its reference,
and when the last holder goes away the path is freed with the
socket's sk_user_data still pointing at it: the next byte from the
peer runs the socket callbacks against freed memory, and nothing ever
releases the socket.

Refuse the accept for a connection whose destroy has begun, the same
way an unexpected path state is refused: drop the path claim and
reset the new socket.  The peer reconnects with backoff and, by then,
either finds a fresh connection or nothing listening.

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

diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index dcac10a91a67..e22ea9ca8c1c 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -278,7 +278,15 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
 	cp = rs_tcp->t_cpath;
 	conn_state = rds_conn_path_state(cp);
 	WARN_ON(conn_state == RDS_CONN_UP);
-	if (conn_state != RDS_CONN_CONNECTING && conn_state != RDS_CONN_ERROR) {
+	/* A connection whose destroy has begun has been quiesced and is
+	 * only waiting for its last reference: its paths sit in
+	 * RDS_CONN_DOWN, which rds_tcp_accept_one_path() happily claims.
+	 * Installing a socket on it would leave sk_user_data pointing
+	 * at a path that is about to be freed.
+	 */
+	if (rds_destroy_pending(conn) ||
+	    (conn_state != RDS_CONN_CONNECTING &&
+	     conn_state != RDS_CONN_ERROR)) {
 		rds_conn_path_drop(cp, 0);
 		goto rst_nsk;
 	}
-- 
2.25.1

[PATCH net-next v3 11/13] 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 5a4644c40be5..257e03ed4861 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 v3 12/13] 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 e5a8534c23cf..f5f25e4a53b9 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];
@@ -332,12 +331,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;
 	}
@@ -408,7 +408,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 {
@@ -431,7 +430,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);
 		}
 	}
@@ -669,7 +667,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++)
@@ -678,10 +675,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 v3 13/13] 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 94d6ac174dde..f59a487b079d 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;
@@ -960,6 +962,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);
@@ -1577,6 +1580,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 v3 09/13] net/rds: refuse to queue a message on a connection being destroyed

[sashiko-bot]

> rds_conn_path_quiesce() tears down cp_send_queue by walking it with no
> lock held.  That was tolerable while a connection could only be
> destroyed with no sender in flight, but a sender now holds a
> reference across rds_sendmsg(), and rds_conn_destroy() can quiesce
> the connection underneath it.  rds_send_queue_rm() adds to
> cp_send_queue under cp_lock, so the unlocked walk races the add and
> can corrupt the list.  Worse, a message added after the purge sits on
> the queue of a quiesced connection holding the connection reference
> rds_send_queue_rm() took for it: the reference is only dropped when
> the message is freed, the message is only freed when the queue is
> torn down, and the queue is only torn down by the destroy that has
> already run.  The connection would never be freed, and with it the
> transport could never unload.
> 
> Splice the queue away under cp_lock in the quiesce, and have
> rds_send_queue_rm() test rds_destroy_pending() under that same lock
> before it touches either queue.  A sender that gets there first has
> its message purged; one that gets there second is refused, and
> rds_sendmsg() returns -EAGAIN for it, the same result the early
> rds_destroy_pending() check in rds_sendmsg() already produces for a
> connection whose destroy had begun before the send started.
> rds_send_queue_rm()'s *queued becomes negative on refusal so that the
> wait loop in rds_sendmsg() stops waiting for send room that will never
> come.
> 
> Assisted-by: Claude-Code:claude-fable-5
> Signed-off-by: Allison Henderson <achender@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914033719.138057-1-achender@kernel.org?part=9

Re: [PATCH net-next v3 04/13] net/rds: make rds_destroy_pending() cover single-connection destroy

[sashiko-bot]

> 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.
> 
> [ ... ]
> 
> 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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914033719.138057-1-achender@kernel.org?part=4

Re: [PATCH net-next v3 05/13] net/rds: split connection destroy into quiesce and kref-governed free

[sashiko-bot]

> 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
> [ ... ]
>  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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914033719.138057-1-achender@kernel.org?part=5

Re: [PATCH net-next v3 01/13] net/rds: ib: don't enable interrupts in rds_ib_conn_free()

[sashiko-bot]

> rds_ib_conn_free() unlinks the connection from its device or nodev
> list under spin_lock_irq()/spin_unlock_irq().  It is not only called
> from the rmmod path, though: __rds_conn_create() calls
> trans->conn_free() to undo a lost creation race while it still holds
> rds_conn_lock, taken with spin_lock_irqsave().  The unconditional
> spin_unlock_irq() then re-enables interrupts with rds_conn_lock held
> and leaves them enabled when the caller's spin_unlock_irqrestore()
> runs, defeating the irqsave the caller relied on.
> 
> Use the irqsave/irqrestore pair, as rds_tcp_conn_free() and
> rds_loop_conn_free() already do.
> 
> Fixes: 745cbccac3fe ("RDS: Rewrite connection cleanup, fixing oops on rmmod")
> Assisted-by: Claude-Code:claude-fable-5
> Signed-off-by: Allison Henderson <achender@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914033719.138057-1-achender@kernel.org?part=1

Re: [PATCH net-next v3 06/13] net/rds: wait for connections to be freed on transport unload

[sashiko-bot]

> 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
> [ ... ]
>  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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914033719.138057-1-achender@kernel.org?part=6

Re: [PATCH net-next v3 03/13] net/rds: guard every work-requeueing site with rds_destroy_pending()

[sashiko-bot]

> 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_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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914033719.138057-1-achender@kernel.org?part=3

Re: [PATCH net-next v3 07/13] net/rds: unlink transport nodes before a possibly deferred connection free

[sashiko-bot]

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

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914033719.138057-1-achender@kernel.org?part=7

Re: [PATCH net-next v3 10/13] net/rds: tcp: don't attach an accepted socket to a connection being destroyed

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] The `rds_destroy_pending(conn)` check introduced by the patch is lockless with respect to connection teardown, leaving a Time-Of-Check to Time-Of-Use (TOCTOU) race condition that renders the patch's attempt to fix the Use-After-Free ineffective.
--

commit 128ba6b33f50fd05797877892e71aaf3ccd2f612
Author: Allison Henderson <achender@kernel.org>

net/rds: tcp: don't attach an accepted socket to a connection being destroyed

This commit attempts to prevent a use-after-free by refusing to attach an
accepted socket to a connection whose teardown has already begun, doing so
by checking rds_destroy_pending().

> diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
> index dcac10a91a67f..e22ea9ca8c1ce 100644
> --- a/net/rds/tcp_listen.c
> +++ b/net/rds/tcp_listen.c
> @@ -278,7 +278,15 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
>  	cp = rs_tcp->t_cpath;
>  	conn_state = rds_conn_path_state(cp);
>  	WARN_ON(conn_state == RDS_CONN_UP);
> -	if (conn_state != RDS_CONN_CONNECTING && conn_state != RDS_CONN_ERROR) {
> +	/* A connection whose destroy has begun has been quiesced and is
> +	 * only waiting for its last reference: its paths sit in
> +	 * RDS_CONN_DOWN, which rds_tcp_accept_one_path() happily claims.
> +	 * Installing a socket on it would leave sk_user_data pointing
> +	 * at a path that is about to be freed.
> +	 */
> +	if (rds_destroy_pending(conn) ||
> +	    (conn_state != RDS_CONN_CONNECTING &&
> +	     conn_state != RDS_CONN_ERROR)) {

[Severity: Critical]
Does evaluating rds_destroy_pending() without an RCU read-side critical
section leave a Time-Of-Check to Time-Of-Use race condition?

Looking at rds_conn_destroy() in net/rds/connection.c, the teardown
sequence sets the c_destroy_in_prog flag and calls synchronize_rcu().
Because rds_tcp_accept_one() isn't holding rcu_read_lock() here,
synchronize_rcu() will not wait for it.

If rds_tcp_accept_one() gets preempted immediately after this check, could
rds_conn_destroy() run concurrently on another CPU, complete the teardown
while the socket is still NULL, and free the connection?

When rds_tcp_accept_one() resumes, it appears it would install the new
socket onto the connection and drop its reference via rds_conn_put().
This would leave the socket's callbacks and sk_user_data pointing to
freed memory, leading to the use-after-free this patch intends to prevent.

>  		rds_conn_path_drop(cp, 0);
>  		goto rst_nsk;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914033719.138057-1-achender@kernel.org?part=10

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

[sashiko-bot]

> 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.
> [ ... ]
>  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>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914033719.138057-1-achender@kernel.org?part=8

Re: [PATCH net-next v3 02/13] net/rds: free every path's transport data on the passive create paths

[sashiko-bot]

> trans->conn_alloc() may allocate transport data for every path of a
> multipath connection - rds_tcp_conn_alloc() does - which is why the
> lost-creation-race exit of __rds_conn_create() loops over all npaths
> when it frees the connection it just built.  The passive-connection
> exit right above it, taken when a loopback parent already has its
> c_passive twin, frees only path 0 and leaks the transport data of
> paths 1..npaths-1.  RDS/TCP loopback is exactly a multipath passive
> connection, so this is reachable.
> 
> Move the loop into a helper and use it on both exits.
> 
> Fixes: 1c5113cf796b ("RDS: Initialize all RDS_MPATH_WORKERS in __rds_conn_create")
> Assisted-by: Claude-Code:claude-fable-5
> Signed-off-by: Allison Henderson <achender@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914033719.138057-1-achender@kernel.org?part=2

Re: [PATCH net-next v3 11/13] net/rds: pin the connection across RDMA-CM event handling

[sashiko-bot]

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

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914033719.138057-1-achender@kernel.org?part=11

Re: [PATCH net-next v3 12/13] net/rds: drop rds_conn_count in favor of t_conn_count

[sashiko-bot]

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

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914033719.138057-1-achender@kernel.org?part=12

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Race condition in `rds_send_probe()` can leak `rds_message` and connection reference during connection teardown.
--

commit 9f78a5cbb0d8c28990a439e07491dbc084983be8
Author: Sharath Srinivasan <sharath.srinivasan@oracle.com>

net/rds: hold a connection reference from struct rds_incoming

This commit ensures that every holder of rds_incoming->i_conn takes a
reference to the connection. This prevents a use-after-free when the
connection is destroyed by device removal or module unload while messages
are still queued on the socket.

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

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 94d6ac174dde8..f59a487b079d5 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c
[ ... ]
> @@ -1577,6 +1580,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() */

[Severity: High]
Does this code allow a message and connection reference to be permanently
leaked during connection teardown?

In rds_send_queue_rm(), rds_destroy_pending() is checked while holding
cp->cp_lock before queueing a message. However, here in rds_send_probe(), the
message is added to the send queue and the connection reference is taken
without first checking rds_destroy_pending() under the lock.

If a concurrent network namespace teardown or module unload initiates
rds_conn_destroy(), it calls rds_conn_path_quiesce() which purges the
cp_send_queue under cp_lock. This probe message can then be inserted into
the queue right after the purge.

After releasing the lock, rds_send_probe() checks rds_destroy_pending()
outside the lock to decide whether to queue the worker:

	rcu_read_lock();
	if (!rds_destroy_pending(cp->cp_conn))
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 1);
	rcu_read_unlock();

If the teardown race described above occurs, rds_destroy_pending() will
evaluate to true here, and the worker will not be queued.

Since the message was already added to the purged cp_send_queue and the
worker is skipped, is the message left stranded indefinitely? This would
cause the connection reference to never be dropped, leading to an indefinite
hang during transport module unload, as rds_conn_wait_conns_freed() waits
forever for the connection count to reach zero.

>  	rm->m_inc.i_conn = cp->cp_conn;
>  	rm->m_inc.i_conn_path = cp;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914033719.138057-1-achender@kernel.org?part=13

Re: [PATCH net-next v3 02/13] net/rds: free every path's transport data on the passive create paths

[netdev-bot+sashiko]

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

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

- [Medium] The commit message of "net/rds: free every path's transport
  data on the passive create paths" claims that the passive-creation…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch
  encodes the correct create-time path count on the rollback exits while…
- [High] This problem wasn't introduced by this patch, but the patch
  extends the same wrong ordering to every path: on both rollback exits…

--

net/rds: free every path's transport data on the passive create paths

This moves the per-path cp_transport_data free loop out of the
lost-creation-race exit of __rds_conn_create() into a new helper,
rds_conn_free_transport_data(), and calls that helper from both the
passive-creation exit and the lost-race exit.

A few questions about the change and about the surrounding code below.

[Severity: Medium]

>     The passive-connection
>     exit right above it, taken when a loopback parent already has its
>     c_passive twin, frees only path 0 and leaks the transport data of
>     paths 1..npaths-1.  RDS/TCP loopback is exactly a multipath passive
>     connection, so this is reachable.

Can the passive-creation exit actually be reached with npaths > 1 on this
tree?

parent is only ever assigned in __rds_conn_create() here:

	conn = rds_conn_lookup(net, head, laddr, faddr, trans, tos, dev_if);
	if (conn &&
	    conn->c_loopback &&
	    conn->c_trans != &rds_loop_transport &&
	    ipv6_addr_equal(laddr, faddr) &&
	    !is_outgoing) {
		parent = conn;
		conn = parent->c_passive;
	}

rds_conn_lookup() only matches conn->c_trans == trans, and npaths comes
from that same trans:

	int npaths = (trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);

rds_tcp_transport is the only transport with t_mp_capable = 1, and it also
sets t_prefer_loopback = 1, so a hash-resident conn with
c_trans == &rds_tcp_transport and c_loopback == 1 does not seem
constructible:

	loop_trans = rds_trans_get_preferred(net, faddr, conn->c_dev_if);
	if (loop_trans) {
		rds_trans_put(loop_trans);
		conn->c_loopback = 1;
		if (trans->t_prefer_loopback) {
			if (likely(is_outgoing)) {
				trans = &rds_loop_transport;
			} else {
				...
				conn = ERR_PTR(-EOPNOTSUPP);
				goto out;
			}
		}
	}

For the outgoing case the stored conn's c_trans becomes the loop transport,
which the parent predicate explicitly excludes, and the incoming case never
creates a conn at all.  IB sets neither flag, so an IB parent gives
npaths == 1.

If that reasoning holds, the patched exit only ever runs with npaths == 1
and the new loop is equivalent to the single trans->conn_free() it
replaces.  Should the changelog describe this as hardening for future
multipath passive transports rather than a reachable leak, and should the
Fixes: 1c5113cf796b tag be dropped so this is not pulled into stable as a
leak fix?

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index b6c4beb50eaf0..a96569a3ee9ad 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -161,6 +161,22 @@ static void __rds_conn_path_init(struct rds_connection *conn,
>  	cp->cp_flags = 0;
>  }
>  
> +/* Undo trans->conn_alloc(): it may have allocated transport data for
> + * every path of a multipath connection, not just for path 0.
> + */
> +static void rds_conn_free_transport_data(struct rds_connection *conn,
> +					 int npaths)
> +{
> +	struct rds_conn_path *cp;
> +	int i;
> +
> +	for (i = 0; i < npaths; i++) {
> +		cp = &conn->c_path[i];
> +		if (cp->cp_transport_data)
> +			conn->c_trans->conn_free(cp->cp_transport_data);
> +	}
> +}
> +

[Severity: High]

This isn't a bug introduced by this patch, but while the rollback exits now
consistently use the create-time npaths, the teardown side still derives its
own count from conn->c_trans, which may no longer be the transport npaths
was computed from.

Creation computes npaths from the caller-supplied trans and allocates one
ordered workqueue per path:

	int npaths = (trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
	...
	for (i = 0; i < npaths; i++) {
		__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(...);

but for an outgoing RDS/TCP connection to a local address trans is replaced
before conn->c_trans is stored:

	if (trans->t_prefer_loopback) {
		if (likely(is_outgoing)) {
			trans = &rds_loop_transport;

rds_conn_destroy() then recomputes:

	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);

rds_loop_transport is not t_mp_capable, so npaths is 1 there while 8 paths
and 8 workqueues were created.  rds_conn_path_destroy() is the only caller
of

	if (cp->cp_wq != rds_wq) {
		destroy_workqueue(cp->cp_wq);
		cp->cp_wq = NULL;
	}

so does this leak the 7 remaining workqueue_structs (and their
pool_workqueues) before kfree(conn->c_path), and also skip the per-path
WARN_ON(delayed_work_pending())/BUG_ON(!list_empty(&cp->cp_retrans))
checks?

The reported trigger is: bind an RDS socket to a non-127 local address on
lo (so rds_trans_get_preferred() picks rds_tcp_transport), sendmsg to that
same address, then delete the netns so rds_loop_kill_conns() ->
rds_conn_destroy() runs.  Cycling namespaces repeats it.

rds_conn_message_info_cmn() derives the count the same way

	npaths = (conn->c_trans->t_mp_capable ?
		  RDS_MPATH_WORKERS : 1);

so does it also under-report paths for these connections?  Would it be
better to store the create-time path count in the connection (c_npaths)
and have all consumers use that?

> @@ -316,7 +332,7 @@ static struct rds_connection *__rds_conn_create(struct net *net,
>  	if (parent) {
>  		/* Creating passive conn */
>  		if (parent->c_passive) {
> -			trans->conn_free(conn->c_path[0].cp_transport_data);
> +			rds_conn_free_transport_data(conn, npaths);
>  			free_cp = conn->c_path;
>  			kmem_cache_free(rds_conn_slab, conn);
>  			conn = parent->c_passive;

[Severity: High]

This is a pre-existing ordering issue, but the patch now applies it to
every path on both exits, so it seems worth asking about here.

Both rollback exits free the transport data and the conn first, and the
per-path workqueues are only destroyed later at out:

	rds_conn_free_transport_data(conn, npaths);
	free_cp = conn->c_path;
	kmem_cache_free(rds_conn_slab, conn);
	...
out:
	if (free_cp) {
		for (i = 0; i < npaths; i++)
			if (free_cp[i].cp_wq != rds_wq)
				destroy_workqueue(free_cp[i].cp_wq);
		kfree(free_cp);
	}

destroy_workqueue() drains work already queued on those workqueues, and the
drained handlers dereference cp->cp_conn and cp->cp_transport_data, both
already freed.  Can work be pending at that point?

trans->conn_alloc() publishes the transport data on a transport-global list
before the creation race is resolved:

net/rds/ib_cm.c:rds_ib_conn_alloc() {
	...
	spin_lock_irqsave(&ib_nodev_conns_lock, flags);
	list_add_tail(&ic->ib_node, &ib_nodev_conns);
	spin_unlock_irqrestore(&ib_nodev_conns_lock, flags);
}

and an IB device add walks that list:

net/rds/ib.c:rds_ib_nodev_connect() {
	spin_lock(&ib_nodev_conns_lock);
	list_for_each_entry(ic, &ib_nodev_conns, ib_node)
		rds_conn_connect_if_down(ic->conn);
	spin_unlock(&ib_nodev_conns_lock);
}

For a conn still inside __rds_conn_create(), cp_state is RDS_CONN_DOWN,
cp_flags is 0 and rds_destroy_pending() is false, so
rds_conn_path_connect_if_down() queues immediately:

	if (rds_conn_path_state(cp) == RDS_CONN_DOWN &&
	    !test_and_set_bit(RDS_RECONNECT_PENDING, &cp->cp_flags))
		queue_delayed_work(cp->cp_wq, &cp->cp_conn_w, 0);

If the creating thread then loses the race and takes one of these exits,
destroy_workqueue() drains cp_conn_w and rds_connect_worker() reads
conn = cp->cp_conn from the freed slab object and calls
conn->c_trans->conn_path_connect(cp) through it.

Would it be safer to destroy (and thereby drain) the per-path workqueues
before freeing the transport data and the conn, so the order becomes
destroy_workqueue() -> conn_free() -> kmem_cache_free()?

Unlike rds_ib_destroy_nodev_conns() and rds_tcp_destroy_conns(), which are
ordered by set_unloading plus synchronize_rcu(), and
rds_tcp_sysctl_reset(), which skips conns with tc->t_sock == NULL,
rds_ib_nodev_connect() has neither an RCU barrier nor a state filter that
would exclude a mid-creation conn.  Is there something else that keeps it
out of that list walk?

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

Re: [PATCH net-next v3 03/13] net/rds: guard every work-requeueing site with rds_destroy_pending()

[netdev-bot+sashiko]

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

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

- [Medium] Incomplete coverage vs. the patch's own claim: the subject
  says "guard every work-requeueing site with rds_destroy_pending()",…

--

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

This adds the rcu_read_lock() + rds_destroy_pending() guard around five
path-work arming sites: the two cp_send_w re-arms in ib_send.c, the
cp_recv_w re-arm in rds_ib_recv_refill(), the cp_recv_w kick in
rds_tcp_accept_one(), and the cp_conn_w arm in rds_sendmsg().

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

[Severity: Medium]
Is "every work-requeueing site" accurate here?  After this patch four
arming sites in net/rds/threads.c still queue path works with no
rcu_read_lock() and no rds_destroy_pending() test.

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

rds_recv_worker():

	switch (ret) {
	case -EAGAIN:
		rds_stats_inc(s_recv_immediate_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_recv_w, 0);
		break;
	case -ENOMEM:
		rds_stats_inc(s_recv_delayed_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_recv_w, 2);
		break;

These four look functionally safe to me, since they are self-requeues
issued from inside the running work item and rds_conn_path_destroy()
disables the work before flushing it:

net/rds/connection.c:rds_conn_path_destroy() {
	/* make sure lingering queued work won't try to ref the conn */
	cancel_delayed_work_sync(&cp->cp_send_w);
	cancel_delayed_work_sync(&cp->cp_recv_w);
	...
}

__cancel_work_sync() grabs the pending bit (deleting any armed
delayed-work timer) and calls work_offqd_disable() before flushing, and
queue_delayed_work_on() then drops the queue attempt via
clear_pending_if_disabled(), so a requeue from the callback that is
being flushed is discarded and the callback cannot run again afterwards.

The following patch in the series also adds a comment on
struct rds_connection::c_destroy_in_prog that states the rule with no
exception:

	 * 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: ...

Could either the subject and changelog note the self-requeue exemption
(and the cancel_delayed_work_sync() reason it holds), or the four
threads.c sites be converted too, so the documented invariant and the
code agree?

Also, the changelog argues for the IB sites:

>     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.

The same delay-2 timer shape appears in the threads.c -ENOMEM cases, so
a reader may conclude those are equally exposed.  Would it help to say
explicitly why the threads.c requeues are not in the same category?

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

Re: [PATCH net-next v3 04/13] net/rds: make rds_destroy_pending() cover single-connection destroy

[netdev-bot+sashiko]

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

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

- [Medium] The new c_destroy_in_prog + synchronize_rcu() guard only
  covers requeue sites that acquire the struct rds_connection pointer…
- [Low] The two comments this patch adds overstate the invariant they
  document.

Pre-existing issues:
- [High] AB-BA deadlock on the single-connection destroy path that this
  patch is written to protect.
- [Low] The header comment above rds_conn_destroy()
  (net/rds/connection.c, immediately above the function this patch…

--

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 and synchronize_rcu(), and tests it
from rds_destroy_pending() so that the predicate also covers the destroy
of a single connection.  The now-unreferenced RDS_DESTROY_PENDING cp_flags
bit and its dead test in rds_ib_is_unloading() are removed.

No Link: tags in the commit header.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index a96569a3ee9ad..242ca0570a47e 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -579,6 +579,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);
> +

[Severity: Low]
Is the parenthetical "which all test rds_destroy_pending() under
rcu_read_lock()" accurate?  The same claim appears in the new
c_destroy_in_prog comment in net/rds/rds.h ("A site that arms a path work
must test the predicate and queue the work inside one rcu_read_lock()
section") and in the commit message ("all of those sites guard the queueing
with rds_destroy_pending() under rcu_read_lock() (the last stragglers were
converted by the previous patch)").

There appear to be four arming sites in net/rds/threads.c that do neither.
rds_send_worker() re-arms cp_send_w with no predicate and no RCU section:

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

and rds_recv_worker() does the same for cp_recv_w.

rds_conn_path_drop() is a fifth site, and it deliberately bypasses the
predicate:

	rcu_read_lock();
	if (!destroy && rds_destroy_pending(cp->cp_conn)) {
		rcu_read_unlock();
		return;
	}
	queue_work(cp->cp_wq, &cp->cp_down_w);

For the threads.c self-requeues the thing that keeps them off a destroyed
workqueue looks like the cancel/disable semantics of
cancel_delayed_work_sync() rejecting a requeue issued from inside the work
being cancelled, not the RCU and predicate pairing these comments name.
Would it make sense to either name these exceptions in the two comments, or
convert those sites like the others?

I checked the end of the series and both comments and all four threads.c
sites are unchanged there.

[Severity: Low]
This isn't a bug introduced by this patch, but the header comment a dozen
lines above the new store still reads:

	/*
	 * 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.
	 */

That contract is contradicted by rds_ib_cm_connect_complete() in
net/rds/ib_cm.c, which destroys a single connection from an RDMA CM event
on a protocol version mismatch, and by the comment this patch adds just
below, which documents the function as fencing off concurrent requeuers for
exactly that non-rmmod case.  Should the stale header comment be refreshed
in the same patch?

For the record, the next patch in the series, "net/rds: split connection
destroy into quiesce and kref-governed free", rewrites this comment block,
so the contradiction does not survive the series.

[Severity: High]
This is a pre-existing issue, but since this is the per-connection destroy
path the commit message relies on, is there an AB-BA deadlock on it?

rds_rdma_cm_event_handler_cmn() in net/rds/rdma_transport.c takes
mutex_lock(&conn->c_cm_lock) and holds it across the whole event switch:

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

The ESTABLISHED case reaches rds_ib_cm_connect_complete(), which for a peer
whose negotiated version is neither RDS_PROTOCOL_VERSION nor
RDS_PROTOCOL_COMPAT_VERSION calls rds_conn_destroy(conn).  The chain is:

rds_conn_destroy() -> rds_conn_path_destroy() ->
	rds_conn_path_drop(cp, true);
	flush_work(&cp->cp_down_w);

rds_conn_path_drop() with destroy == true sets cp_state to RDS_CONN_ERROR
and unconditionally queues cp_down_w, then the flush blocks the CM handler
thread while it still holds c_cm_lock.

The flushed worker runs rds_conn_shutdown(), where the DOWN to DOWN
transition fails because the state is RDS_CONN_ERROR, so it proceeds to:

	if (!rds_conn_path_transition(cp, RDS_CONN_DOWN, RDS_CONN_DOWN)) {
		/* ... else we may be deadlocking with the CM handler ... */
		mutex_lock(&cp->cp_cm_lock);

IB is not t_mp_capable, and net/rds/rds_single_path.h has:

	#define c_cm_lock		c_path[0].cp_cm_lock

so cp->cp_cm_lock is the very mutex the blocked CM handler thread holds.
Can either side make progress here?  Only the shutdown worker sets
RDS_CONN_DOWN, and its transition cannot succeed after the
atomic_set(RDS_CONN_ERROR), so mutex_lock() looks unavoidable.

The protocol major and minor come from the peer's rdma_cm private data via
rds_ib_set_protocol(), so a remote IB peer advertising an unsupported RDS
version reaches this.

As a secondary note, if the flush ever did return, the trailing
mutex_unlock(&conn->c_cm_lock) in rds_rdma_cm_event_handler_cmn() would
touch a conn that rds_conn_destroy() already kmem_cache_free()d.

The drop plus flush and the shutdown-side mutex_lock() are identical at the
baseline, and both are still present at the end of the series.

>  	/* 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/rds.h b/net/rds/rds.h
> index 2db49573dacd5..50b08c28ab865 100644
> --- a/net/rds/rds.h
> +++ b/net/rds/rds.h

[ ... ]

> @@ -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: Medium]
Does the READ_ONCE() here rely on the caller having obtained the conn
pointer inside the same rcu_read_lock() section as the check and the queue?

At this commit struct rds_connection has no reference count, and
rds_conn_destroy() runs destroy_workqueue(cp->cp_wq), kfree(conn->c_path)
and kmem_cache_free(rds_conn_slab, conn) synchronously after the
synchronize_rcu().  Callers that hold a long-lived, non-refcounted pointer
instead of looking the conn up under RCU do exist: rs->rs_conn cached by
rds_sendmsg(), and ic->conn used by the IB completion handlers.

For those holders the deferred-send requeue in net/rds/send.c looks like it
can read the flag from freed slab memory and still queue onto a freed
workqueue:

	rcu_read_lock();
	if (rds_destroy_pending(cpath->cp_conn))
		ret = -ENETUNREACH;
	else
		queue_delayed_work(cpath->cp_wq, &cpath->cp_send_w, 1);
	rcu_read_unlock();

The single-connection destroy this patch targets,
rds_ib_cm_connect_complete() on a version mismatch, is exactly where such
stale pointers exist, so does the RCU flag alone close the
requeue-onto-freed-workqueue class the commit message describes, or does
that need the connection refcounting?

Later patches in this series do close it: "net/rds: split connection
destroy into quiesce and kref-governed free" moves destroy_workqueue(),
kfree(c_path) and kmem_cache_free() into rds_conn_destroy_fini() driven by
kref_put(), and "net/rds: hold connection references in lookup, sockets and
c_passive" makes rds_conn_lookup(), __rds_conn_create() and the rs_conn
cache own references.  Since this commit carries a Fixes: tag and may be
backported on its own, would it be worth noting in the commit message that
the guard is complete only with those follow-ups?

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

Re: [PATCH net-next v3 05/13] net/rds: split connection destroy into quiesce and kref-governed free

[netdev-bot+sashiko]

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

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

- [High] The new double-destroy guard in rds_conn_destroy() is not backed
  by a reference and does not synchronize the losing caller, yet its…
- [Low] Concern (assessed INVALID for this series): the deferred
  rds_conn_destroy_fini() can run transport-module code (conn_free),…
- [Low] Concern (assessed INVALID): the new c_refcount comment in
  net/rds/rds.h ("Free of the connection memory (not the teardown of its…

Pre-existing issues:
- [High] Per-path ordered workqueues allocated in __rds_conn_create() are
  leaked for every connection that goes through the loopback-transport…
- [High] Destroying an IB connection from the RDMA-CM ESTABLISHED
  callback self-deadlocks.
- [Low] Concern (assessed INVALID for this series): because no holder
  takes a reference in this patch, the peer-triggered rds_conn_destroy()…

--

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

This adds a kref to struct rds_connection, initialized in
__rds_conn_create().  rds_conn_destroy() keeps quiescing the connection
synchronously, while the frees (the transport's conn_free, the path
workqueues, the c_path array and the slab object) move to the new
rds_conn_destroy_fini(), which runs from the last rds_conn_put().

A few questions below.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 242ca0570a47e..a44aa4d2a5e8b 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -520,10 +522,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;
>  

[Severity: High]

This isn't a bug introduced by this patch, but since the changelog calls
out the rds_ib_cm_connect_complete() destroy path explicitly: can that
path deadlock on the synchronous half that rds_conn_path_quiesce()
inherits?

rds_conn_path_quiesce() still does:

	rds_conn_path_drop(cp, true);
	flush_work(&cp->cp_down_w);

and rds_rdma_cm_event_handler_cmn() holds mutex_lock(&conn->c_cm_lock)
across trans->cm_connect_complete(), unlocking only at the out: label.
net/rds/rds_single_path.h has:

	#define c_cm_lock		c_path[0].cp_cm_lock

so that is the same mutex as cp->cp_cm_lock for the path being torn down.

rds_conn_path_drop() sets cp_state to RDS_CONN_ERROR before queueing
cp_down_w, so the flushed rds_shutdown_worker() -> rds_conn_shutdown()
takes the

	if (!rds_conn_path_transition(cp, RDS_CONN_DOWN, RDS_CONN_DOWN)) {

branch and calls mutex_lock(&cp->cp_cm_lock).  The chain looks like:

rds_rdma_cm_event_handler_cmn()          [holds c_cm_lock]
  rds_ib_cm_connect_complete()
    rds_conn_destroy()
      rds_conn_path_quiesce()
        flush_work(&cp->cp_down_w)       [waits for the worker]

cp_down_w -> rds_shutdown_worker() -> rds_conn_shutdown()
  mutex_lock(&cp->cp_cm_lock)            [waits for the handler]

The trigger is a remote peer completing a connection with c_version below
RDS_PROTOCOL_VERSION and not equal to RDS_PROTOCOL_COMPAT_VERSION.  Both
waits are unbounded, and one of the stuck threads is the RDMA-CM event
delivery context.  This is unchanged from the baseline
rds_conn_path_destroy() and is still present at the end of the series.

> @@ -552,6 +556,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);
> @@ -561,16 +575,52 @@ static void rds_conn_path_destroy(struct rds_conn_path *cp)
>  	cp->cp_conn->c_trans->conn_free(cp->cp_transport_data);
>  }

[Severity: Low]

conn_free() is transport module text and destroy_workqueue() plus
kmem_cache_free(rds_conn_slab, conn) now run from the deferred path, so a
reference dropped after rds_conn_destroy() returned can execute them.
rds_tcp_exit(), rds_ib_exit(), rds_loop_exit() and rds_conn_exit() at this
commit have no barrier that waits for outstanding references before
rds_trans_unregister() and kmem_cache_destroy().

This looks fine as of this commit, since the initial reference is the only
one and the release is still synchronous, and the following patch adds
struct rds_transport::t_conn_count plus rds_conn_wait_conns_freed() called
from rds_tcp_exit(), rds_ib_exit() and rds_loop_exit().  Would it help to
say in the changelog that the unload barrier arrives in the next patch, so
that this one is not read as already allowing deferred frees past module
unload?

>  
> +/* 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);

[Severity: High]

This is a pre-existing issue and not introduced by this patch, but the
split moves the workqueue destruction here, so it is worth asking: does
this leak the per-path workqueues of a loopback-substituted connection?

__rds_conn_create() computes npaths from the caller's transport:

	int npaths = (trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);

allocates that many ordered workqueues:

	for (i = 0; i < npaths; i++) {
		...
		conn->c_path[i].cp_wq =
			alloc_ordered_workqueue("krds_cp_wq#%lu/%d", 0,
						rds_conn_count, i);

but the loopback substitution before that loop can replace trans:

			trans = &rds_loop_transport;
	...
	conn->c_trans = trans;

rds_tcp_transport has .t_prefer_loopback = 1 and .t_mp_capable = 1, while
rds_loop_transport has t_mp_capable == 0.  So an outgoing RDS-TCP
connection to a local address allocates RDS_MPATH_WORKERS workqueues, and
rds_conn_destroy_fini() recomputes npaths from conn->c_trans, which is now
the loop transport, i.e. 1.  Paths 1 and up are never visited and
kfree(conn->c_path) then drops the only pointers to their cp_wq.

Separately, rds_conn_path_free() returns before destroy_workqueue():

	if (!cp->cp_transport_data)
		return;

	if (cp->cp_wq != rds_wq) {
		destroy_workqueue(cp->cp_wq);

so a path that got a workqueue but no transport data leaks it even when it
is visited.  The create-side error path in the later series patch loops
over the local npaths and destroys every cp_wq != rds_wq unconditionally,
which suggests the teardown side wants the same treatment.  Repeated
netns create/send/exit cycles would leak these workqueues without bound.

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

[ ... ]

> @@ -584,11 +634,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);

[Severity: High]

The comment says "Only the first caller proceeds" and "a looked-up conn
can never be quiesced twice", but the guard itself is not backed by a
reference.  Can reading conn->c_destroy_in_prog here be the
use-after-free it is meant to prevent?

Three of the four rds_conn_destroy() callers reach the conn through a
transport-private list whose node is unlinked only by the deferred
conn_free().  For example 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);

rds_tcp_kill_sock()/rds_tcp_destroy_conns() pass tc->t_cpath->cp_conn and
rds_loop_exit()/rds_loop_kill_conns() pass lc->conn with the same shape.

If a concurrent destroyer, i.e. rds_ib_cm_connect_complete() reacting to a
peer that advertised an unsupported protocol version, has already reached
rds_conn_destroy_fini(), the conn is gone:

rds_conn_destroy_fini()
	kmem_cache_free(rds_conn_slab, conn);

so the guard reads freed slab memory, and a recycled object whose
c_destroy_in_prog happens to read 0 would be quiesced again.

The second half is the losing caller.  It returns immediately while the
winner is still inside cancel_delayed_work_sync()/flush_work() and before
conn_free() and destroy_workqueue() have run.  The winner's conn_free()
then does:

rds_ib_conn_free()
	spin_lock_irqsave(lock_ptr, flags);
	list_del(&ic->ib_node);
	spin_unlock_irqrestore(lock_ptr, flags);
	...
	kfree(ic);

which list_del()s a node still linked on the loser's stack-local tmp_list
and frees ic while the loser's list_for_each_entry_safe() keeps walking
it.

Should the guard be paired with an acquire that cannot resurrect a dead
object, and with a wait for the in-progress destroy?  rds_conn_get() as
added here is a plain kref_get(), so a non-RCU list holder has no way to
take a reference safely.  The later patches "unlink transport nodes before
a possibly deferred connection free" and "wait for connections to be freed
on transport unload" cover part of this, but at the end of the series the
three transport walkers still pass a bare ic->conn, lc->conn and
tc->t_cpath->cp_conn with no reference, and rds_ib_destroy_nodev_conns()
is additionally re-invoked as the resweep callback of
rds_conn_wait_conns_freed(), precisely while conns are quiesced but not
yet freed.

>  
>  	/* 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();

[ ... ]

> @@ -607,12 +669,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);

[Severity: Low]

This is a pre-existing exposure and not introduced by this patch, noting
it only because the changelog names these holders.  With no other holders
yet, this put still frees the conn before rds_conn_destroy() returns, so
rds_ib_cm_connect_complete()'s destroy leaves
rds_rdma_cm_event_handler_cmn() to run

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

on freed memory, and rds_sendmsg() still reads the unreferenced
rs->rs_conn cache.

Both are closed later in the same series: "hold connection references in
lookup, sockets and c_passive" makes rs_conn own a reference, and "pin the
connection across RDMA-CM event handling" takes
rds_conn_get_unless_zero() at handler entry and puts it after
mutex_unlock().  Would it be worth a sentence here noting that the
existing bare-pointer holders remain uncovered until those patches?

> 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 the wording reads as if nothing about the transport
state is deferred, while conn_free() and destroy_workqueue() did move into
rds_conn_path_free().  The distinction the code makes is that the teardown,
conn->c_trans->conn_path_shutdown() driven from rds_conn_shutdown() via
rds_conn_path_drop(cp, true) plus flush_work(&cp->cp_down_w), stays
synchronous, while the free of that state is deferred.  Could this comment
spell that out, for instance by saying the transport state is shut down
synchronously but freed from rds_conn_destroy_fini()?

[ ... ]

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

Re: [PATCH net-next v3 06/13] net/rds: wait for connections to be freed on transport unload

[netdev-bot+sashiko]

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

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

- [High] rds_ib_exit()'s new comment and the commit message claim the
  resweep guarantees that "a late arrival is destroyed rather than…
- [Low] Assessed as a false positive for this patch and for the final
  state of the series (recorded for the pipeline, baseline decision =…

Pre-existing issues:
- [Low] Assessed as a false positive under the series rule (recorded for
  the pipeline, baseline decision = false).

--

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().  The transport exit paths
(rds_ib_exit(), rds_tcp_exit(), rds_loop_exit()) then wait, without a
bound, for that count to reach zero before letting the module text go
away, with an optional resweep callback used by IB.

A few questions below about the IB resweep scope, the comment on
rds_conn_wait_conns_freed(), and the nodev sweep list handling.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index a44aa4d2a5e8..c3b3d756c52e 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -596,7 +601,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.
> + */

[Severity: Low]

This is a pre-existing hole rather than something introduced here, but
does the comment above rds_conn_wait_conns_freed() describe more than
t_conn_count actually covers at this point in the series?

The comment names inc_free among the frees the wait protects, yet at this
commit a struct rds_incoming holds no connection reference, so an unread
datagram does not contribute to t_conn_count.  The wait in rds_ib_exit()
can therefore complete and rds_ib_recv_exit() can destroy
rds_ib_incoming_slab while an inc is still queued on a socket.

The commit message already flags the sequencing ("once the following
patches make incs hold a connection reference"), and the later patch
"net/rds: hold a connection reference from struct rds_incoming" makes it
true by taking a reference in rds_inc_init():

net/rds/recv.c:rds_inc_init() {
	...
	rds_conn_get(conn);	/* put in rds_inc_put() */
	inc->i_conn = conn;
	...
}

Would it be worth noting in this comment that the inc_free half only
becomes accurate with that later patch, so the intermediate state of the
tree is not read as a guarantee it does not yet provide?

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

[Severity: High]

Can the resweep here miss a connection entirely and turn this into an
rmmod that never returns?

rds_ib_destroy_nodev_conns() only looks at the global ib_nodev_conns
list:

net/rds/ib_rdma.c:rds_ib_destroy_nodev_conns() {
	spin_lock_irq(&ib_nodev_conns_lock);
	list_splice_init(&ib_nodev_conns, &tmp_list);
	spin_unlock_irq(&ib_nodev_conns_lock);
	...
}

A connection whose rds_ib_connection is still on an rds_ibdev->conn_list
is not on that list, and after ib_unregister_client() has returned
nothing appears to drop it again.  rds_ib_dev_shutdown() is a one-shot
sweep that already ran:

net/rds/ib.c:rds_ib_dev_shutdown() {
	spin_lock_irqsave(&rds_ibdev->spinlock, flags);
	list_for_each_entry(ic, &rds_ibdev->conn_list, ib_node)
		rds_conn_path_drop(&ic->conn->c_path[0], true);
	...
}

and every other drop site goes through rds_conn_path_drop(cp, false),
which bails out while rds_destroy_pending() is true - and
rds_ib_set_unloading() has made t_unloading true for the whole of
rds_ib_exit():

net/rds/connection.c:rds_conn_path_drop() {
	...
	if (!destroy && rds_destroy_pending(cp->cp_conn)) {
		rcu_read_unlock();
		return;
	}
	queue_work(cp->cp_wq, &cp->cp_down_w);
	...
}

Is there a window where an ic attaches to the device list after that
sweep?  rds_ib_setup_qp() resolves the device and then adds itself:

net/rds/ib_cm.c:rds_ib_setup_qp() {
	rds_ibdev = rds_ib_get_client_data(dev);
	if (!rds_ibdev)
		return -EOPNOTSUPP;
	...
	/* add the conn now so that connection establishment has the dev */
	rds_ib_add_conn(rds_ibdev, conn);
	...
}

and rds_ib_remove_one() clears the client data only after
rds_ib_dev_shutdown() has walked conn_list, so a connect already in
flight when rmmod starts can still get a device pointer and land on
conn_list.

Such a connection never migrates to ib_nodev_conns, the resweep never
sees it, and t_conn_count stays non-zero.  Since the wait is deliberately
unbounded and uninterruptible, rmmod would then block forever with the
ten-second warning repeating, and the module stays in MODULE_STATE_GOING.

Would it make sense for the resweep to also walk the rds_ib_devices
conn_lists, or otherwise cover connections that are still device
attached?  As written, the comment's claim that "a late arrival is
destroyed rather than waited on forever" seems to hold only for
connections that already reached ib_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/ib_rdma.c b/net/rds/ib_rdma.c
> index db7e92e7bd29..a9b27f06cbfc 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c
> @@ -165,7 +165,7 @@ void rds_ib_destroy_nodev_conns(void)
>  
>  	/* avoid calling conn_destroy with irqs off */
>  	spin_lock_irq(&ib_nodev_conns_lock);
> -	list_splice(&ib_nodev_conns, &tmp_list);
> +	list_splice_init(&ib_nodev_conns, &tmp_list);
>  	spin_unlock_irq(&ib_nodev_conns_lock);
>  
>  	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)

[Severity: Low]

Now that the free of a connection can outlive rds_conn_destroy(), is the
ib_node of each ic left linked on the stack-local tmp_list across the
destroy?  rds_ib_conn_free() is what unlinks ib_node, so a free that runs
after rds_ib_destroy_nodev_conns() has returned would do a list_del()
against a dead stack frame.  The same shape exists in
rds_tcp_destroy_conns():

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

At this commit rds_conn_get() has no callers, so the rds_conn_put() at
the end of rds_conn_destroy() is always the last reference and conn_free()
still runs inline inside the sweep loop, which means this cannot fire
yet.  The next patch in the series, "net/rds: unlink transport nodes
before a possibly deferred connection free", adds the per-entry
list_del_init() under the transport lock ahead of the reference-holding
patches.

Is it worth mentioning that ordering dependency in this commit message,
so the intermediate state is clearly safe by accident of there being no
extra reference holders yet rather than by design?

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

Re: [PATCH net-next v3 07/13] 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: 1 · Medium: 2 · Low: 1

- [High] The patch does not close the bug class its commit message claims
  to close ("Unlink each node under the transport lock right before its…
- [Medium] rds_ib_destroy_nodev_conns() (net/rds/ib_rdma.c:176-181) now
  leaves ic->ib_node in the empty state (list_del_init) while the…
- [Medium] rds_ib_destroy_nodev_conns() unlinks ic->ib_node while holding
  only ib_nodev_conns_lock (net/rds/ib_rdma.c:177-179), but the lock…
- [Low] Commit message overstates the change as a fix for a live
  corruption.

--

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

The transport teardown helpers gather per-connection transport nodes onto a
stack list and call rds_conn_destroy() for each one.  This patch moves the
unlink of each transport node out of the transport conn_free() callbacks and
into the teardown loops, so a conn_free() that runs after the helper has
returned no longer touches the helper's stack list.  The conn_free()
callbacks for IB and loopback now skip a node that is already empty.

A few questions on the reference counting assumptions below.

[Severity: Low]
The changelog describes an existing corruption:

  "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."

Can either of those holders exist at this commit?  rds_conn_get() in
net/rds/connection.c has no callers here - grep over net/rds only finds the
definition, the EXPORT_SYMBOL and the declaration in rds.h.  With the kref
count always at one, the rds_conn_put() at the end of rds_conn_destroy() is
always the final put, so conn_free() still runs synchronously inside the
teardown loop.

The rs_conn holder in net/rds/send.c and the inc references in
net/rds/recv.c appear later in the series, in "net/rds: hold connection
references in lookup, sockets and c_passive" and "net/rds: hold a
connection reference from struct rds_incoming".

Would it be worth saying that this patch prepares the teardown helpers for
the reference holders added by the following patches, so a reader does not
go looking for the defect (or a Fixes: tag) in the tree as it stands?

> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 118e033229aa..26a32c1ec8f7 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -1287,7 +1287,9 @@ void rds_ib_conn_free(void *arg)
>  	lock_ptr = ic->rds_ibdev ? &ic->rds_ibdev->spinlock : &ib_nodev_conns_lock;
>  
>  	spin_lock_irqsave(lock_ptr, flags);
> -	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_irqrestore(lock_ptr, flags);

[Severity: Medium]
Which lock serializes this new test-then-delete against the new
list_del_init() in rds_ib_destroy_nodev_conns()?

rds_ib_conn_free() picks the lock from ic->rds_ibdev:

	lock_ptr = ic->rds_ibdev ? &ic->rds_ibdev->spinlock : &ib_nodev_conns_lock;

but rds_ib_destroy_nodev_conns() always takes ib_nodev_conns_lock.  A node
gathered on tmp_list can be migrated by rds_ib_add_conn() before the sweep
reaches it:

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

	spin_lock(&rds_ibdev->spinlock);
	list_add_tail(&ic->ib_node, &rds_ibdev->conn_list);
	spin_unlock(&rds_ibdev->spinlock);
	spin_unlock_irq(&ib_nodev_conns_lock);

	ic->rds_ibdev = rds_ibdev;
}

After that, ib_node lives on rds_ibdev->conn_list, protected by
rds_ibdev->spinlock, which is also what rds_ib_dev_shutdown() holds while
walking that list.  Would the sweep's list_del_init() then mutate a live
device list under the wrong lock, and could the list_empty()/list_del()
pair above and the sweep's list_del_init() run concurrently and unlink the
same node twice?

The reachable window looks narrow: rds_ib_destroy_nodev_conns() runs from
rds_ib_exit() after rds_ib_unregister_client(), and once the per-device
client data is cleared rds_ib_setup_qp()'s rds_ib_get_client_data() returns
NULL and it returns -EOPNOTSUPP before rds_ib_add_conn().  A thread that
already fetched rds_ibdev and is then preempted until after the splice
would still get there, though - is there something that excludes it?

> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index a9b27f06cbfc..91db43a0e7d7 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_init(&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 this actually leave nothing on the stack list for a later free to
touch?  Only the entry about to be destroyed is detached here; every entry
the loop has not visited yet is still linked on tmp_list with a non-empty
ib_node, and no reference is taken on ic->conn.

With the reference holders added later in the series, a connection whose
initial reference was already dropped by rds_ib_cm_connect_complete() on a
protocol version mismatch:

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

stays alive and stays linked on ib_nodev_conns while a socket's rs_conn or
an inc on a receive queue still owns a kref.  If that holder drops the last
reference while this loop is running:

rds_conn_destroy_fini()
  rds_conn_path_free()
    rds_ib_conn_free()
      spin_lock_irqsave(lock_ptr, flags);
      if (!list_empty(&ic->ib_node))
              list_del(&ic->ib_node);	/* unlinks out of tmp_list */
      ...
      kfree(ic);

the list_del() writes through the neighbours of an entry that lives on this
stack list, and the ic is freed.  The loop's cached cursor _ic then points
into freed slab memory, list_del_init() writes into it, and
rds_conn_destroy() is called on a dangling conn.  For the entry currently
being processed, the same free after spin_unlock_irq() makes the ic->conn
read a use-after-free.

The window is not small - it spans the synchronize_rcu(),
cancel_delayed_work_sync() and flush_work() calls inside
rds_conn_destroy() - and rds_conn_wait_conns_freed() re-runs this sweep
precisely while deferred frees are outstanding:

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();
	...
}

Would taking a connection reference while gathering under the transport
lock, and dropping it after rds_conn_destroy(), or detaching and marking all
gathered nodes up front, close this?

The TCP and loopback helpers use the same one-entry-at-a-time detach.  They
look safe at the end of the series only because rds_conn_destroy() has no
other caller for those transports, so every gathered conn still holds its
initial reference for the whole walk - is that something the helpers should
be relying on?

[Severity: Medium]
Does this break the invariant that a live rds_ib_connection is always linked
on some list?  After list_del_init() the ic is still alive with an empty
ib_node, but both users of that node still assert otherwise:

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

If rds_ib_setup_qp() reaches rds_ib_add_conn() for a connection whose node
the sweep has already emptied, does the first BUG_ON() fire?  And in the
opposite order, if add_conn wins and relinks the node onto
rds_ibdev->conn_list, the sweep empties it again and the
rds_ib_conn_path_shutdown() -> rds_ib_remove_conn() that rds_conn_destroy()
drives would hit the second BUG_ON().

Before this patch the node stayed linked on tmp_list, so
list_empty(&ic->ib_node) was false in that window and only the weaker
BUG_ON(list_empty(&ib_nodev_conns)) could trigger.  Since rds_ib_conn_free()
is the only one of the three sites taught to tolerate an empty ib_node,
would an explicit detached flag - as the TCP side uses with
t_tcp_node_detached - keep the assertions and the linkage consistent?

[ ... ]

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

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

[netdev-bot+sashiko]

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

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

- [High] rds_send_probe() (net/rds/send.c:1579-1586) links a probe/pong
  message onto cp->cp_send_queue and takes a connection reference…
- [Medium] The commit message claims the new rs_lock coverage in
  rds_ioctl(SIOCRDSSETTOS) closes the window that "let a racing sendmsg…
- [Low] The comment block documenting rds_conn_lookup()'s contract now
  sits above the newly added rds_conn_passive_locked() helper, which has…
- [Low] struct rds_sock in net/rds/rds.h explicitly enumerates what
  rs_lock protects ("rs_lock protects all these adjacent members before…

--

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

This patch makes rds_conn_lookup() and __rds_conn_create() return a
connection with a reference held for the caller, gives the rs->rs_conn
sendmsg cache and parent->c_passive references of their own, and moves the
rs_conn/rs_tos serialization from the global rds_sock_lock to rs_lock.  It
also RCU-annotates c_passive and closes a KCSAN-reported data race between
concurrent rds_sendmsg() calls on the same socket.

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

A few questions below about the new rs_lock discipline, the misplaced
comment block in connection.c, and the interaction between the new
"sender keeps running against a quiesced conn" rule and rds_send_probe().

> diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
> index d5defe9172e3..1cc20b5cfd21 100644
> --- a/net/rds/af_rds.c
> +++ b/net/rds/af_rds.c

[ ... ]

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

[Severity: Medium]

Does this comment (and the matching claim in the commit message, "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") hold
for a sendmsg that is already past its own rs_lock section?

rds_sendmsg() drops rs_lock before it samples rs->rs_tos:

net/rds/send.c:rds_sendmsg() {
	spin_lock_irqsave(&rs->rs_lock, flags);
	conn = rs->rs_conn;
	...
	} else {
		conn = NULL;
	}
	spin_unlock_irqrestore(&rs->rs_lock, flags);

	if (!conn) {
		...
		conn = rds_conn_create_outgoing(sock_net(sock->sk),
						&rs->rs_bound_addr, &daddr,
						rs->rs_transport, rs->rs_tos,
	...
}

and the install re-takes rs_lock without re-checking conn->c_tos against
rs->rs_tos:

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

So with thread A between the two rs_lock sections (conn == NULL) and
thread B in SIOCRDSSETTOS, B sees rs_tos == 0 && rs_conn == NULL, succeeds
and stores the new tos; A then creates or looks up a connection with the
old tos and caches it.  Can that leave rs->rs_tos == X while
rs->rs_conn->c_tos == 0, and send the in-flight message over a connection
whose service level does not match the socket's configured ToS?

Would snapshotting rs_tos inside the first rs_lock section, or validating
conn->c_tos == rs->rs_tos when installing the cache, close this?

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index c3b3d756c52e..7ef6fb9d352b 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,

[Severity: Low]

Was the new helper meant to be inserted above rds_conn_lookup()'s comment
rather than below it?  As written, the "rcu read lock must be held or the
connection spinlock ... a reference is taken on the returned connection"
text now documents rds_conn_passive_locked(), which has neither property:
it asserts rds_conn_lock is held (RCU alone is not enough) and returns the
pointer without any reference.  __rds_conn_create() has to take the
reference itself right after calling it:

			passive = rds_conn_passive_locked(parent);
			...
				rds_conn_get(passive);

Meanwhile rds_conn_lookup(), whose contract this patch actually changes,
ends up with no comment describing that the caller now owns a reference.

> @@ -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;
>  		}

[ ... ]

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

[Severity: Low]

While touching this header, should the struct rds_sock declarations be
updated too?  rs_conn is now both rs_lock-serialized and reference-owning,
but its comment still only says:

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

and rs_lock's own annotation reads:

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

rs_conn sits before that block and rs_tos sits at the very end of the
struct, so the header currently tells a reader that neither is covered by
rs_lock, which is the opposite of the new rule in rds_ioctl() and
rds_sendmsg().

> 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

[ ... ]

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

[Severity: High]

The commit message says the connection "may still be destroyed while a
send is in flight ... but it is only quiesced; the free is held off by the
sender's reference."  Given that, can a sender that is already holding this
reference strand a probe message on a path that has already been purged?

rds_sendmsg() reaches the ping path before its own rds_destroy_pending()
re-check:

	if (conn->c_trans->t_mp_capable &&
	    !rds_conn_path_up(&conn->c_path[0])) {
		...
		rds_send_ping(conn, 0);
	}

and rds_send_probe() enqueues under cp_lock with no destroy test at all;
only the work queueing afterwards is guarded:

net/rds/send.c:rds_send_probe() {
	spin_lock_irqsave(&cp->cp_lock, flags);
	list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
	set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
	rds_message_addref(rm);
	rm->m_inc.i_conn = cp->cp_conn;
	...
	spin_unlock_irqrestore(&cp->cp_lock, flags);
	...
	rcu_read_lock();
	if (!rds_destroy_pending(cp->cp_conn))
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 1);
	rcu_read_unlock();
}

rds_conn_path_quiesce() drains cp_send_queue exactly once, and
rds_conn_destroy() runs that pass only once (c_destroy_in_prog guards it):

net/rds/connection.c:rds_conn_path_quiesce() {
	...
	/* tear down queued messages */
	list_for_each_entry_safe(rm, rtmp,
				 &cp->cp_send_queue,
				 m_conn_item) {
		list_del_init(&rm->m_conn_item);
		BUG_ON(!list_empty(&rm->m_sock_item));
		rds_message_put(rm);
	}
	...
}

If the cp_lock section in rds_send_probe() runs after that drain, nothing
reaps the message afterwards - rds_send_xmit() refuses destroy-pending
conns and there is no second purge pass.  Later in the series the same
enqueue also takes a connection reference released only from
rds_message_put() ("put in rds_message_put()"), so at that point does the
stranded probe keep c_refcount from reaching zero, leaving
rds_conn_destroy_fini() unreached and rds_conn_wait_conns_freed() waiting
forever at transport unload or netns teardown?

The sibling enqueue rds_send_queue_rm() does get exactly this guard later
in the series, with rds_conn_path_quiesce() splicing under cp_lock to pair
with it:

		spin_lock(&cp->cp_lock);
		if (rds_destroy_pending(conn)) {
			spin_unlock(&cp->cp_lock);
			*queued = -EAGAIN;
			goto unlock;
		}

Should rds_send_probe() get the same test under cp_lock?  Note the probe
enqueue is also reachable from the receive path, since a peer's probe ping
drives rds_recv_incoming() -> rds_send_pong() -> rds_send_probe().

> +
> +	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,

[ ... ]

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

Re: [PATCH net-next v3 09/13] net/rds: refuse to queue a message on a connection being destroyed

[netdev-bot+sashiko]

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

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

- [Critical] rds_conn_path_quiesce() keeps
  BUG_ON(!list_empty(&rm->m_sock_item)) (net/rds/connection.c:633) while…
- [High] The new comment in rds_conn_path_quiesce()
  (net/rds/connection.c:622-627) claims that after the cp_lock-protected…
- [High] The rds_destroy_pending() refusal is nested inside `if
  (rs->rs_snd_bytes < rds_sk_sndbuf(rs))` (net/rds/send.c:932, check at…
- [Medium] The changelog ("a message added after the purge sits on the
  queue of a quiesced connection holding the connection reference…
- [Low] This patch repairs a regression created by the immediately
  preceding patch of the same series, "net/rds: hold connection…

--

net/rds: refuse to queue a message on a connection being destroyed

This splices cp_send_queue away under cp_lock in rds_conn_path_quiesce()
rather than walking it unlocked, and makes rds_send_queue_rm() test
rds_destroy_pending() under cp_lock before it queues a message.  On
refusal *queued is set to -EAGAIN and rds_sendmsg() returns that to the
caller.

No Link: tags are present in the commit header.

A few questions on this one.

[Severity: Low]

> rds_conn_path_quiesce() tears down cp_send_queue by walking it with no
> lock held.  That was tolerable while a connection could only be
> destroyed with no sender in flight, but a sender now holds a
> reference across rds_sendmsg(), and rds_conn_destroy() can quiesce
> the connection underneath it.

The "now" here refers to the immediately preceding patch of this series,
"net/rds: hold connection references in lookup, sockets and c_passive",
which adds the rds_conn_get() that keeps the connection alive across
rds_sendmsg().

If that is right, then applying the series up to that commit leaves
rds_conn_path_quiesce() walking cp_send_queue unlocked while a referenced
sender can be adding to it under cp_lock, and a bisect landing there sees
the list corruption described above.

Would it make sense to fold this change into that patch, or to carry a
Fixes: tag or an explicit dependency note?

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 7ef6fb9d352b..e5a8534c23cf 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -606,6 +606,8 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
>  static void rds_conn_path_quiesce(struct rds_conn_path *cp)
>  {
>  	struct rds_message *rm, *rtmp;
> +	unsigned long flags;
> +	LIST_HEAD(purge);
>  
>  	if (!cp->cp_transport_data)
>  		return;
> @@ -617,10 +619,16 @@ static void rds_conn_shutdown(struct rds_conn_path *cp)
>  	rds_conn_path_drop(cp, true);
>  	flush_work(&cp->cp_down_w);
>  
> -	/* tear down queued messages */
> -	list_for_each_entry_safe(rm, rtmp,
> -				 &cp->cp_send_queue,
> -				 m_conn_item) {
> +	/* Tear down queued messages.  Take the queue under cp_lock:
> +	 * a sender that still holds a reference can be inside
> +	 * rds_send_queue_rm() right now, and it tests
> +	 * rds_destroy_pending() under the same lock, so after this
> +	 * splice nothing is added behind our back.
> +	 */
> +	spin_lock_irqsave(&cp->cp_lock, flags);
> +	list_splice_init(&cp->cp_send_queue, &purge);
> +	spin_unlock_irqrestore(&cp->cp_lock, flags);

[Severity: High]

Is "nothing is added behind our back" true for the other producer of
cp_send_queue?  rds_send_probe() takes cp_lock and adds unconditionally,
with no rds_destroy_pending() test before the add:

net/rds/send.c:rds_send_probe() {
	...
	spin_lock_irqsave(&cp->cp_lock, flags);
	list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
	set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
	rds_message_addref(rm);
	rm->m_inc.i_conn = cp->cp_conn;
	rm->m_inc.i_conn_path = cp;
	...
	spin_unlock_irqrestore(&cp->cp_lock, flags);

	/* schedule the send work on cp_wq */
	rcu_read_lock();
	if (!rds_destroy_pending(cp->cp_conn))
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 1);
	rcu_read_unlock();
	...
}

The destroy test there runs after the message is already linked, and only
suppresses the work that could have drained it.

This is reachable from the syscall being hardened here.  rds_sendmsg()
calls rds_send_ping() -> rds_send_probe() for an mp-capable connection
whose path 0 is down, before its own rds_destroy_pending() check:

net/rds/send.c:rds_sendmsg() {
	...
	if (conn->c_trans->t_mp_capable &&
	    !rds_conn_path_up(&conn->c_path[0])) {
		...
		rds_send_ping(conn, 0);
	}
	...
	if (rds_destroy_pending(conn)) {
		ret = -EAGAIN;
		goto out;
	}

rds_send_pong() reaches the same code from the receive path.

Since the quiesce splices once and never purges again, can a probe that
lands after the splice sit on cp_send_queue forever with no worker to
drain it?  That would leak the rds_message whose reference the queue
holds, and leave rm->m_conn_item and rm->m_inc.i_conn_path pointing into
the c_path array that rds_conn_destroy_fini() kfree()s.

At the end of this series rds_send_probe() also takes rds_conn_get(), so
would the same window then leak a connection reference and leave
rds_conn_wait_conns_freed() waiting for a count that never drops, blocking
transport unload?

> +	list_for_each_entry_safe(rm, rtmp, &purge, m_conn_item) {
>  		list_del_init(&rm->m_conn_item);
>  		BUG_ON(!list_empty(&rm->m_sock_item));
              ^^^^^^

[Severity: Critical]

Can this BUG_ON() fire in exactly the interleaving the changelog says is
now handled ("A sender that gets there first has its message purged")?

Every message rds_send_queue_rm() puts on cp_send_queue is linked on the
socket send queue in the same critical section:

net/rds/send.c:rds_send_queue_rm() {
	...
		list_add_tail(&rm->m_sock_item, &rs->rs_send_queue);
		set_bit(RDS_MSG_ON_SOCK, &rm->m_flags);
	...
		list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
		set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
}

m_sock_item is emptied only by the ack path via
rds_send_remove_from_sock(), or by rds_send_drop_to() / rds_release().
Nothing in the destroy path does it.  rds_conn_path_quiesce() itself runs
rds_conn_path_drop(cp, true) and flush_work(&cp->cp_down_w) first, and
rds_conn_shutdown() -> rds_conn_path_reset() -> rds_send_path_reset()
puts still socket-linked messages back on cp_send_queue:

net/rds/send.c:rds_send_path_reset() {
	...
	list_splice_init(&cp->cp_retrans, &cp->cp_send_queue);
	...
}

So an unacked message reaches this loop with a non-empty m_sock_item and
panics the kernel.

Independent of the panic, the loop drops the conn-side reference with
list_del_init(&rm->m_conn_item) + rds_message_put(), but never clears
RDS_MSG_ON_CONN and never undoes the socket-side state (the rs_snd_bytes
charge, RDS_MSG_ON_SOCK, the extra rds_message_addref() and the
sock_hold()).

Because the flag stays set, does a later rds_send_drop_to() still pass
here and issue a second put for the reference already released?

net/rds/send.c:rds_send_drop_to() {
	...
		if (!test_and_clear_bit(RDS_MSG_ON_CONN, &rm->m_flags)) {
			spin_unlock_irqrestore(&cp->cp_lock, flags);
			continue;
		}
		list_del_init(&rm->m_conn_item);
	...
}

That is a refcount_t underflow on rm->m_refcount and a premature free.
And since that path holds cp_lock while the quiesce walks its private
purge list with no lock at all, it can also list_del_init() an entry
living on the purge list, so does the splice really establish the
exclusive ownership the new comment claims?

Should the purge clear RDS_MSG_ON_CONN under cp_lock and detach the
message from its socket queue the way rds_send_drop_to() and
rds_send_remove_from_sock() do, instead of asserting m_sock_item is empty?

>  		rds_message_put(rm);
>  	}

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 1ae1f24c24e8..94d6ac174dde 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c
> @@ -928,6 +928,19 @@ static int rds_send_queue_rm(struct rds_sock *rs, struct rds_connection *conn,
>  	 * and poll() now knows no more data can be sent.
>  	 */
>  	if (rs->rs_snd_bytes < rds_sk_sndbuf(rs)) {
> +		/* rds_conn_path_quiesce() empties cp_send_queue under
> +		 * cp_lock once the connection's destroy has begun.  Test
> +		 * for that under the same lock, before touching either
> +		 * queue: a message added after the purge would hold a
> +		 * connection reference nothing ever drops.
> +		 */

[Severity: Medium]

Does the connection reference this comment describes exist yet?  The same
claim is in the changelog:

> Worse, a message added after the purge sits on
> the queue of a quiesced connection holding the connection reference
> rds_send_queue_rm() took for it: the reference is only dropped when
> the message is freed, the message is only freed when the queue is
> torn down, and the queue is only torn down by the destroy that has
> already run.  The connection would never be freed, and with it the
> transport could never unload.

At this commit rds_send_queue_rm() takes only a message reference:

net/rds/send.c:rds_send_queue_rm() {
	...
		rds_message_populate_header(&rm->m_inc.i_hdr, sport, dport, 0);
		rm->m_inc.i_conn = conn;
		rm->m_inc.i_conn_path = cp;
		rds_message_addref(rm);
	...
}

and rds_message_put() goes rds_message_purge() -> rds_message_free()
without touching rm->m_inc.i_conn, so nothing here pins the connection.
The rds_conn_get() with the "put in rds_message_put()" annotation only
appears with the last patch of the series, "net/rds: hold a connection
reference from struct rds_incoming".

The damage from a post-purge add in this tree state looks like a leaked
rds_message plus a list linkage dangling into the c_path array that
rds_conn_destroy_fini() kfree()s, not a connection that can never be
freed.

Could the changelog and the comment be adjusted for the tree state they
land in, or this patch ordered after the incoming-reference patch?

> +		spin_lock(&cp->cp_lock);
> +		if (rds_destroy_pending(conn)) {
> +			spin_unlock(&cp->cp_lock);
> +			*queued = -EAGAIN;
> +			goto unlock;
> +		}
> +

[Severity: High]

The refusal is nested inside if (rs->rs_snd_bytes < rds_sk_sndbuf(rs)),
so it only fires when send room exists.  Does that leave the blocked
sender, the one the changelog is about, still waiting forever?

With the send buffer full, rds_send_queue_rm() falls through and returns
0, and rds_sendmsg() re-arms the wait with timeo == MAX_SCHEDULE_TIMEOUT
on a blocking socket:

net/rds/send.c:rds_sendmsg() {
	...
		timeo = wait_event_interruptible_timeout(*sk_sleep(sk),
					rds_send_queue_rm(rs, conn, cpath, rm,
							  rs->rs_bound_port,
							  dport,
							  &queued),
					timeo);
		...
		if (timeo > 0 || timeo == MAX_SCHEDULE_TIMEOUT)
			continue;
	...
}

rs_snd_bytes is decremented only by rds_send_sndbuf_remove() from the ack
path via rds_send_remove_from_sock(), or from rds_send_drop_to().  After
the connection is quiesced no acks arrive, and the purge in
rds_conn_path_quiesce() calls neither rds_send_sndbuf_remove() nor
rds_wake_sk_sleep() - it only drops the conn-side message reference.

So the wake condition can never become non-zero, and the new
if (queued < 0) exit below is never evaluated.  Should the destroy test
move outside the sndbuf-room branch, and should the purge release the
sndbuf accounting and wake sk_sleep()?

>  		rs->rs_snd_bytes += len;
>  
>  		/* let recv side know we are close to send space exhaustion.
> @@ -951,7 +964,6 @@ static int rds_send_queue_rm(struct rds_sock *rs, struct rds_connection *conn,
>  		rm->m_inc.i_conn_path = cp;
>  		rds_message_addref(rm);
>  
> -		spin_lock(&cp->cp_lock);
>  		rm->m_inc.i_hdr.h_sequence = cpu_to_be64(cp->cp_next_tx_seq++);
>  		list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
>  		set_bit(RDS_MSG_ON_CONN, &rm->m_flags);

[ ... ]

> @@ -1474,6 +1487,11 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
>  			ret = -ETIMEDOUT;
>  		goto out;
>  	}
> +	/* rds_send_queue_rm() refused: the connection is being destroyed */
> +	if (queued < 0) {
> +		ret = queued;
> +		goto out;
> +	}

Thanks for looking at these.

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

Re: [PATCH net-next v3 10/13] net/rds: tcp: don't attach an accepted socket to a connection being destroyed

[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] Incomplete at this point in the series: the identical hole in the
  IB accept path (rds_ib_cm_handle_connect(), net/rds/ib_cm.c) is left…
- [Low] The commit message describes a present-tense use-after-free plus
  socket leak ('the next byte from the peer runs the socket callbacks…

--

net/rds: tcp: don't attach an accepted socket to a connection being destroyed

rds_tcp_accept_one() now refuses an incoming accept when the target
connection's destroy has already begun, instead of claiming a DOWN path
and installing the accepted socket on it.  The refusal reuses the existing
unexpected-path-state path: drop the path claim and reset the new socket.

> commit message

>     A connection whose destroy has begun is quiesced - its paths are DOWN
>     and its old sockets released - but stays allocated while a reference
>     holder is still around, and rds_conn_create() hands out exactly such
>     a connection with a reference of its own.

[Severity: Low]
The described reachability appears to depend on earlier patches in this
same series rather than on any released kernel.

The connection surviving its own destroy while a holder exists comes from
"net/rds: split connection destroy into quiesce and kref-governed free",
rds_conn_lookup()/__rds_conn_create() handing out a reference on such a
connection comes from "net/rds: hold connection references in lookup,
sockets and c_passive", and the c_destroy_in_prog term the new test relies
on comes from "net/rds: make rds_destroy_pending() cover single-connection
destroy":

net/rds/rds.h:rds_destroy_pending() {
	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));
}

Applied on its own to a tree without those changes, the new test reduces to
the netns and unloading cases and does not close the hole described here.

There is no Fixes: tag and no note about the in-series dependency, so the
message reads as a standalone fix.  Sibling patches in the series that
close the same class of in-series hole ("net/rds: unlink transport nodes
before a possibly deferred connection free", "net/rds: refuse to queue a
message on a connection being destroyed", "net/rds: pin the connection
across RDMA-CM event handling") also carry no Fixes: tag and no stable
note, so this matches the series convention.

Would it be worth stating in the message that the reachability is created
by earlier patches in this series, so an automatic stable pick does not
land a change that cannot achieve its stated purpose there?

> diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
> index dcac10a91a67..e22ea9ca8c1c 100644
> --- a/net/rds/tcp_listen.c
> +++ b/net/rds/tcp_listen.c
> @@ -278,7 +278,15 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
>  	cp = rs_tcp->t_cpath;
>  	conn_state = rds_conn_path_state(cp);
>  	WARN_ON(conn_state == RDS_CONN_UP);
> -	if (conn_state != RDS_CONN_CONNECTING && conn_state != RDS_CONN_ERROR) {
> +	/* A connection whose destroy has begun has been quiesced and is
> +	 * only waiting for its last reference: its paths sit in
> +	 * RDS_CONN_DOWN, which rds_tcp_accept_one_path() happily claims.
> +	 * Installing a socket on it would leave sk_user_data pointing
> +	 * at a path that is about to be freed.
> +	 */
> +	if (rds_destroy_pending(conn) ||
> +	    (conn_state != RDS_CONN_CONNECTING &&
> +	     conn_state != RDS_CONN_ERROR)) {
>  		rds_conn_path_drop(cp, 0);
>  		goto rst_nsk;
>  	}

[Severity: Low]
Should the IB accept path get the same guard in this commit?

rds_ib_cm_handle_connect() in net/rds/ib_cm.c reaches the same place by the
same route: it takes a connection from rds_conn_create(), makes the same
DOWN -> CONNECTING claim that a quiesced-but-still-referenced connection
satisfies, and then installs transport state on it:

net/rds/ib_cm.c:rds_ib_cm_handle_connect() {
	conn = rds_conn_create(&init_net, daddr6, saddr6,
			       &rds_ib_transport, dp_cmn->ricpc_dp_toss,
			       GFP_KERNEL, ifindex);
	...
	mutex_lock(&conn->c_cm_lock);
	if (!rds_conn_transition(conn, RDS_CONN_DOWN, RDS_CONN_CONNECTING)) {
	...
	ic->i_cm_id = cm_id;
	cm_id->context = conn;
	...
	err = rds_ib_setup_qp(conn);
}

At this commit there is no rds_destroy_pending() test anywhere in
net/rds/ib_cm.c, and the hashed rds_conn_lookup() branch of
__rds_conn_create() does not filter c_destroy_in_prog - only the c_passive
branch does:

net/rds/connection.c:__rds_conn_create() {
		conn = rcu_dereference(parent->c_passive);
		if (conn && READ_ONCE(conn->c_destroy_in_prog))
			conn = NULL;
}

So can an incoming RDMA_CM_EVENT_CONNECT_REQUEST attach a fresh cm_id and
QP to a connection that is only waiting for its last reference, leaving
cm_id->context pointing at freed memory and the QP untorn-down?

The very next patch in the series, "net/rds: pin the connection across
RDMA-CM event handling" (3e53d58bba13ea8bec2e328d358bbebdacca18ef), adds
exactly that guard under c_cm_lock:

	if (rds_destroy_pending(conn))
		goto out;

so the gap closes before the end of the series.  What remains is the
bisect window: the enabling reference behaviour arrives two patches earlier
in "net/rds: hold connection references in lookup, sockets and c_passive",
which leaves three commits where a bisect lands on a tree with the IB side
open.  Would folding the IB check in here, or reordering the two patches,
avoid that window?  A note in the message that the IB sibling is handled
separately would also help.

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

Re: [PATCH net-next v3 11/13] 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: 1 (1) · Medium: 0 · Low: 0

- [High] Self-deadlock: rds_ib_cm_connect_complete() destroys the
  connection synchronously from inside the RDMA-CM event callback, while…

Pre-existing issues:
- [High] Uninitialized function-pointer-bearing struct pointer
  dereference in rds_rdma_cm_event_handler_cmn()…

--

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

The patch takes a reference on the connection found in cm_id->context for
the duration of rds_rdma_cm_event_handler_cmn(), ignoring the event when
the connection is already on its way out, and adds an rds_destroy_pending()
re-check under c_cm_lock in rds_ib_cm_handle_connect().  A new helper,
rds_conn_get_unless_zero(), wraps kref_get_unless_zero() on c_refcount.

> 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 since the new hunk lands
directly underneath it: is trans guaranteed to be initialized here?  In
rds_rdma_cm_event_handler_cmn() the declaration is a plain

	struct rds_transport *trans;

and the only assignment is the RDMA_NODE_IB_CA test above, with no else
branch and no bail-out for other node types.  The CONNECT_REQUEST case then
dereferences it unconditionally:

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

On that path conn is NULL (the child cm_id inherits the listener's NULL
context), so none of the "if (conn)" guards cover the use.

Can a non-IB-CA device reach this handler?  RDS's own comment in
rds_ib_laddr_check() says so:

	/* rdma_bind_addr will only succeed for IB & iWARP devices */
	...
	/* due to this, we will claim to support iWARP devices unless we
	   check node_type. */

and rds_ib_add_one() rejects anything that is not RDMA_NODE_IB_CA, while
rds_rdma_listen_init() listens on INADDR_ANY, so the CM fans connect
requests out over every registered device.  With an iWARP or usnic device
present, would a remote connect request to RDS_PORT end up making an
indirect call through an indeterminate stack value?  The kernel build
disables -Wmaybe-uninitialized, so nothing flags it at compile time.

Still present unchanged at the end of the series.

>  
> +	/* 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]

The commit message says "a callback that drops the last reference other
than the handler's implicit one leaves the final mutex_unlock() running on
freed memory".  Which callback is that?

The one callback that destroys the connection is the ESTABLISHED path, and
on that path control does not seem to reach the new rds_conn_put() at all,
because the destroy blocks on the mutex the handler is still holding:

rds_rdma_cm_event_handler_cmn()
  mutex_lock(&conn->c_cm_lock);	/* == conn->c_path[0].cp_cm_lock */
  trans->cm_connect_complete(conn, event)
    rds_ib_cm_connect_complete()
      rds_conn_destroy(conn)
        rds_conn_path_quiesce()
          rds_conn_path_drop(cp, true)	/* always 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 caller */

Is this a circular wait?  The handler thread waits in flush_work() for
cp_down_w, and the flushed worker waits for cp_cm_lock, which the handler
thread holds.

rds_conn_shutdown() does enter that block, since rds_conn_path_drop() has
just moved the state to RDS_CONN_ERROR, so the DOWN -> DOWN transition
fails:

net/rds/connection.c:rds_conn_shutdown() {
	if (!rds_conn_path_transition(cp, RDS_CONN_DOWN, RDS_CONN_DOWN)) {
		/*
		 * ... We don't hold the mutex for the entire
		 * duration of the shutdown operation, else we may be
		 * deadlocking with the CM handler. Instead, the CM event
		 * handler is supposed to check for state DISCONNECTING
		 */
		mutex_lock(&cp->cp_cm_lock);
	...
}

The existing comment there states the assumption that the CM handler only
checks DISCONNECTING and returns.  Does the synchronous rds_conn_destroy()
from inside rds_ib_cm_connect_complete() break that assumption?

The trigger looks remote-controlled.  In rds_ib_cm_connect_complete():

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

A peer accepting with an older advertised version reaches it, and so does a
peer accepting with absent or short private data: the connection is
allocated with kmem_cache_zalloc() and only rds_ib_set_protocol() ever
writes c_version, so c_version stays 0 and compares below
RDS_PROTOCOL_VERSION.

There is a second effect worth checking: the RDMA core holds
id_priv->handler_mutex across the callback, so while the handler thread is
parked in flush_work(), CM events for that cm_id stop and a later
rdma_destroy_id() on it would block as well - and the shutdown worker's
rds_ib_conn_path_shutdown() needs handler_mutex itself.

The synchronous destroy from the callback is older than this patch, but
since that is the very callback the new reference is meant to make safe,
would deferring the destroy out of the CM callback be the change that
actually covers it?  Pinning the connection does not help if the
mutex_unlock() and rds_conn_put() at out: are never reached.

Verified still present at the end of the series.

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

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

Re: [PATCH net-next v3 12/13] 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] The commit message of "net/rds: drop rds_conn_count in favor of
  t_conn_count" opens with "The previous patch gave each transport its…

--

net/rds: drop rds_conn_count in favor of t_conn_count

This patch seeds the per-path workqueue names in __rds_conn_create() from
trans->t_conn_count instead of the global rds_conn_count, and removes
rds_conn_count along with the rds_conn_lock round-trip that
rds_conn_destroy_fini() took solely to decrement it.

No Link: tags are present in the commit header.

> net/rds: drop rds_conn_count in favor of t_conn_count
>
>     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.

[Severity: Low]
Is "the previous patch" the right reference here?

The immediate parent of this commit is 3e53d58bba13 ("net/rds: pin the
connection across RDMA-CM event handling"), which touches net/rds/ib_cm.c,
net/rds/rdma_transport.c and net/rds/rds.h (adding
rds_conn_get_unless_zero()) and does not mention t_conn_count.

t_conn_count and its comment in struct rds_transport were added six patches
earlier, by 45c2c8294f3d ("net/rds: wait for connections to be freed on
transport unload"):

    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()) [...]

whose porting note also says "t_conn_count did not exist upstream and is
introduced here".

The rest of the sentence, that the increments and decrements sit exactly
where rds_conn_count's did, matches the code.  Would it be clearer to name
45c2c8294f3d ("net/rds: wait for connections to be freed on transport
unload") explicitly, or to say "an earlier patch in this series"?

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

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

[netdev-bot+sashiko]

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

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

- [High] rds_send_probe() (net/rds/send.c) now takes a connection
  reference for rm->m_inc.i_conn (line 1583) while linking the probe…
- [Low] The cross-netns comment in rds_recv_incoming()
  (net/rds/recv.c:424-431) is now factually wrong: it justifies dropping…
- [Low] The commit fixes a KASAN-reported slab-use-after-free
  (rds6_inc_info_copy() via rds_info_getsockopt(), plus a second panic…

--

net/rds: hold a connection reference from struct rds_incoming

This makes every assignment of struct rds_incoming::i_conn take a
connection reference, dropping it from rds_inc_put() for received
messages and from a new rds_message_free() helper for m_inc.  The goal is
to keep a connection allocated while incs that point at it sit on socket
receive or send queues.

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

A question on the trailers before the diff:

> Reported-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> Closes: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/

[Severity: Low]
The changelog fixes a KASAN-reported slab-use-after-free (rds6_inc_info_copy()
via rds_info_getsockopt(), plus the rds_ib_inc_free()/rds_clear_recv_queue()
panic) and carries Reported-by:/Closes:, but there is no Fixes: tag.

The unreferenced i_conn assignment goes back to bdbe6fbc6a2f ("RDS:
recv.c").  Would adding

  Fixes: bdbe6fbc6a2f ("RDS: recv.c")

be worthwhile so backporters can see which releases are affected, even
with the "This is not a stable candidate" note in the changelog that
explains this particular fix depends on the connection reference counting
added earlier in the series?

> 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

[ ... ]

> @@ -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 the cross-netns comment further down in rds_recv_incoming() still
describe the code correctly after this change?  It currently reads:

	/*
	 * 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.
	 */

With i_conn now owning a reference from rds_inc_init()/rds_inc_path_init(),
transferred here and released only after c_trans->inc_free() in
rds_inc_put(), can cleanup_net() -> rds_conn_destroy() still free the
connection while the inc is queued?  The test itself may still be wanted
for netns isolation and because c_net would outlive the freed struct net,
but should the stated rationale be updated?

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 94d6ac174dde8..f59a487b079d5 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c

[ ... ]

> @@ -960,6 +962,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);
> @@ -1577,6 +1580,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;

[Severity: High]
Can this new reference in rds_send_probe() be stranded when the
connection is being destroyed?

rds_send_queue_rm() takes the same kind of reference for the same kind of
enqueue, but tests the destroy predicate under cp_lock first:

	spin_lock(&cp->cp_lock);
	if (rds_destroy_pending(conn)) {
		spin_unlock(&cp->cp_lock);
		*queued = -EAGAIN;
		goto unlock;
	}
	...
	rds_conn_get(conn);	/* put in rds_message_put() */

rds_send_probe() has no such test inside its cp_lock section, and
rds_conn_path_quiesce() drains cp_send_queue exactly once under that same
lock:

net/rds/connection.c:rds_conn_path_quiesce() {
	spin_lock_irqsave(&cp->cp_lock, flags);
	list_splice_init(&cp->cp_send_queue, &purge);
	spin_unlock_irqrestore(&cp->cp_lock, flags);
	list_for_each_entry_safe(rm, rtmp, &purge, m_conn_item) {
		...
		rds_message_put(rm);
	}
}

So if rds_send_probe() acquires cp_lock after that splice, the probe rm is
added to a queue nobody drains again, its RDS_MSG_ON_CONN reference is
never dropped, and the trailing

	rcu_read_lock();
	if (!rds_destroy_pending(cp->cp_conn))
		queue_delayed_work(cp->cp_wq, &cp->cp_send_w, 1);
	rcu_read_unlock();

skips the work, so rds_message_put() never reaches the new
rds_message_free().  Does that leak the struct rds_connection reference
taken above?

The window looks reachable from user context and from remote input:

rds_sendmsg() -> rds_send_ping() -> rds_send_probe()
rds_recv_incoming() -> rds_send_pong() -> rds_send_probe()
rds_tcp_accept_one() -> rds_send_probe()

with the rds_destroy_pending() check in rds_sendmsg() done before the
user data copy, well ahead of the rds_send_ping() call.

If the reference is stranded, does t_conn_count then stay non-zero and
leave rds_conn_wait_conns_freed() looping forever on module unload?

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))) {
	...
}

Would adding the same rds_destroy_pending(cp->cp_conn) test inside
rds_send_probe()'s cp_lock section, before the list_add_tail() and
rds_conn_get(), keep the six reference sites consistent?

The commit message says of the six assignment sites that "each of them now
takes a reference"; should it also say what happens when a probe races
teardown?

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