[PATCH net-next v4 00/12] net/rds: make connection lifetime reference-counted

[PATCH net-next v4 00/12] net/rds: make connection lifetime reference-counted

[Allison Henderson]

Hi all,

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

  Patch 2 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 has rds_send_queue_rm() and rds_send_probe() 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 pins the connection across the RDMA-CM event handler
  and rejects a connect request for a connection whose destroy has
  already quiesced it.

  Patch 11 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 12 makes struct rds_incoming hold a reference on i_conn, the
  fix for the KASAN use-after-free Chengfeng Ye reported [4].
  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 12 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 v3 [3]:
 - Rebased; the version-mismatch destroy that patch 10 also had to
   cope with is now an rds_conn_drop() in net (f97d8c7bab78), and
   patch 10's changelog says what remains for it to cover.
 - The uninitialised transport pointer in the CM event handler that
   the second v2 review pass raised turned out to be the bug Aohan
   Mei already posted a fix for (v2 at [5], stalled after review); it
   is carried forward separately for net rather than added here.
 - Patch 7: the teardown walks take a reference on each gathered
   connection, so a connection destroyed earlier and freed by a
   pending holder cannot vanish under the iterator, and the two IB
   list movers tolerate a node the sweep already unlinked instead of
   BUG_ON()ing; the nodev sweep gathers entry by entry, so the
   resweep from the unload wait no longer relies on list_splice_init().
 - Patch 8: SIOCRDSSETTOS check-then-act closed - the install in
   rds_sendmsg() re-checks the socket's ToS under rs_lock; rs_conn
   documented as a referenced, rs_lock-serialised cache; contract
   comment restored to rds_conn_lookup().
 - Patch 9: rds_send_probe() gets the same rds_destroy_pending()
   test under cp_lock as rds_send_queue_rm() (a probe queued after the
   purge pinned the connection for good).
 - v3 patch 10 (rds_tcp_accept_one() destroy check) dropped: the check
   was not serialised against the destroy, and the race it aimed at is
   not reachable - every TCP destroy path stops the listener, flushing
   the accept work, first.  A comment now records that ordering.
 - Changelog and comment corrections from the second v2 review pass
   (self-requeue exemption in patch 3, c_refcount comment in patch 5,
   the uninterruptible wait and the transport-text wake in patch 6,
   "lock-free" wording in patch 11, cross-netns comment in patch 12).

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/20260914033719.138057-1-achender@kernel.org/
[4] https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/
[5] https://lore.kernel.org/netdev/20260825021223.3483044-1-ljp1205831794@gmail.com/

Allison


Allison Henderson (8):
  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 on 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     | 327 +++++++++++++++++++++++++++++++++------
 net/rds/ib.c             |  22 ++-
 net/rds/ib_cm.c          |  29 +++-
 net/rds/ib_rdma.c        |  65 ++++++--
 net/rds/ib_recv.c        |   6 +-
 net/rds/ib_send.c        |  18 ++-
 net/rds/loop.c           |  61 ++++++--
 net/rds/message.c        |  16 +-
 net/rds/rdma_transport.c |  16 +-
 net/rds/rds.h            |  45 +++++-
 net/rds/recv.c           |  26 +++-
 net/rds/send.c           |  96 ++++++++++--
 net/rds/tcp.c            |  53 ++++++-
 net/rds/tcp_listen.c     |  21 ++-
 15 files changed, 707 insertions(+), 118 deletions(-)

-- 
2.25.1

[PATCH net-next v4 01/12] 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 v4 02/12] 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 v4 03/12] 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.

The four self-requeues in rds_send_worker() and rds_recv_worker() are
left alone on purpose: they run from inside the work item itself, and
cancel_delayed_work_sync() disables the work for the duration of the
cancel, so a requeue issued by the still-running callback is dropped
and none can follow once the cancel has returned.

With the predicate as it stands the guards cover the netns teardown and
module unload cases; the following patch extends it to the destroy of a
single connection.

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 v4 04/12] 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 - apart from the workers' own self-requeues, which the sync
cancel in the destroy path already rejects - 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 v4 05/12] 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(), plus an inline
  rds_conn_get_unless_zero() for holders that may find a connection
  already on its way out, 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), so every free still
completes inside rds_conn_destroy().  The next two patches make a
deferred free safe - the transport unload wait and the unlinking of
the transport nodes ahead of the destroy - and only then do the
patches that follow 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        | 13 ++++++
 2 files changed, 91 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..defda3ddefa3 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;
+	/* rds_conn_destroy() quiesces the connection synchronously;
+	 * freeing it - the connection memory, the path workqueues and
+	 * the transport's per-connection state - 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,13 @@ 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);
+/* 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);
+}
 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 v4 06/12] 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.
The wait is deliberately not interruptible: the exit function has
already passed the point of no return, and a killed rmmod would leave
the module half torn down and unloadable, which is worse than a
blocked one.  The polling wakes every 100 ms, so the hung task detector
does not fire.

The wake at the end of rds_conn_destroy_fini() can run from a thread
executing transport module text, but never as that thread's last use
of it: a CM event handler's reference is always dropped before the
rdma_destroy_id() in the connection's own shutdown returns, and
rds_rdma_exit() stops the listener - waiting for any handler running
on it - before rds_ib_exit() starts to wait.

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 | 50 ++++++++++++++++++++++++++++++++++++++++++++
 net/rds/ib.c         | 17 +++++++++++++++
 net/rds/ib_rdma.c    |  2 +-
 net/rds/loop.c       |  2 ++
 net/rds/rds.h        | 14 +++++++++++++
 net/rds/tcp.c        |  1 +
 6 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index a44aa4d2a5e8..1d48da1a794f 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,52 @@ 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 - once incs hold one, 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 defda3ddefa3..b3cc0804156e 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);
@@ -839,6 +845,14 @@ 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
+ */
+#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 v4 07/12] 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 is fine for as long as rds_conn_destroy() frees the connection
before it returns, which is still the case at this point in the
series: the initial reference is the only one.  The following patches
hand out references that outlive the teardown loop - a socket's
cached rs_conn, an inc parked on a receive queue - and with those, a
conn_free() deferred until after the helper has returned would
list_del() the node from a stack frame that no longer exists.  Make
the helpers ready for that first.

Unlink each node under the transport lock right before its
rds_conn_destroy(), 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 two IB list
movers, rds_ib_add_conn() and rds_ib_remove_conn(), likewise leave an
already-unlinked node alone instead of asserting that it is linked: a
connect or shutdown worker can still be running for a connection the
sweep has just unlinked, and such a connection is about to be
destroyed anyway.  rds_ib_add_conn() also loses its assertion that the
nodev list is non-empty, which is not true while a sweep holds the
entries.

The walk itself must not lose the entries either.  A connection that
was destroyed earlier - dropped for a protocol version mismatch, then
found again at module unload - is kept alive only by whatever
reference is still pending, and that can be dropped at any point
during the walk, freeing the transport node the iterator is about to
read.  So the gather takes a reference on each connection it moves
onto the stack list, under the transport lock, and drops it after the
destroy; a connection whose free is already running gets no reference
and is left where it is, since that free unlinks the node itself once
the lock is released.  The tmp_list gathering itself remains: it is
what keeps rds_conn_destroy() from being 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 | 65 ++++++++++++++++++++++++++++++++++++-----------
 net/rds/loop.c    | 59 +++++++++++++++++++++++++++++++++---------
 net/rds/tcp.c     | 52 ++++++++++++++++++++++++++++++++-----
 4 files changed, 146 insertions(+), 34 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..bdd66c9b93c7 100644
--- a/net/rds/ib_rdma.c
+++ b/net/rds/ib_rdma.c
@@ -123,15 +123,18 @@ void rds_ib_add_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *con
 {
 	struct rds_ib_connection *ic = conn->c_transport_data;
 
-	/* conn was previously on the nodev_conns_list */
+	/* conn was previously on the nodev_conns_list, unless a teardown
+	 * sweep already unlinked it ahead of destroying it: then it is
+	 * on its way out and stays off every list.
+	 */
 	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);
+	if (!list_empty(&ic->ib_node)) {
+		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_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;
@@ -142,15 +145,22 @@ void rds_ib_remove_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *
 {
 	struct rds_ib_connection *ic = conn->c_transport_data;
 
-	/* place conn on nodev_conns_list */
+	bool detached;
+
+	/* place conn on nodev_conns_list - unless a teardown sweep
+	 * already unlinked it ahead of destroying it, in which case it
+	 * stays off every list
+	 */
 	spin_lock(&ib_nodev_conns_lock);
 
 	spin_lock_irq(&rds_ibdev->spinlock);
-	BUG_ON(list_empty(&ic->ib_node));
-	list_del(&ic->ib_node);
+	detached = list_empty(&ic->ib_node);
+	if (!detached)
+		list_del(&ic->ib_node);
 	spin_unlock_irq(&rds_ibdev->spinlock);
 
-	list_add_tail(&ic->ib_node, &ib_nodev_conns);
+	if (!detached)
+		list_add_tail(&ic->ib_node, &ib_nodev_conns);
 
 	spin_unlock(&ib_nodev_conns_lock);
 
@@ -163,13 +173,38 @@ void rds_ib_destroy_nodev_conns(void)
 	struct rds_ib_connection *ic, *_ic;
 	LIST_HEAD(tmp_list);
 
-	/* avoid calling conn_destroy with irqs off */
+	struct rds_connection *conn;
+
+	/* Gather the connections and take a reference on each, so that
+	 * none is freed under the walk below (a connection destroyed
+	 * earlier, for a protocol version mismatch, can be on this list
+	 * with only a socket's reference still pending).  One whose free
+	 * is already running gets no reference: its free unlinks the
+	 * node itself, under this lock, once we drop it.  Avoid calling
+	 * conn_destroy with irqs off.
+	 */
 	spin_lock_irq(&ib_nodev_conns_lock);
-	list_splice_init(&ib_nodev_conns, &tmp_list);
+	list_for_each_entry_safe(ic, _ic, &ib_nodev_conns, ib_node) {
+		if (rds_conn_get_unless_zero(ic->conn))
+			list_move_tail(&ic->ib_node, &tmp_list);
+	}
 	spin_unlock_irq(&ib_nodev_conns_lock);
 
-	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
-		rds_conn_destroy(ic->conn);
+	/* rds_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) {
+		conn = ic->conn;
+
+		spin_lock_irq(&ib_nodev_conns_lock);
+		list_del_init(&ic->ib_node);
+		spin_unlock_irq(&ib_nodev_conns_lock);
+
+		rds_conn_destroy(conn);
+		rds_conn_put(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..71f760ccd458 100644
--- a/net/rds/loop.c
+++ b/net/rds/loop.c
@@ -156,6 +156,45 @@ 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;
+	struct rds_connection *conn;
+
+	list_for_each_entry_safe(lc, _lc, tmp_list, loop_node) {
+		conn = lc->conn;
+		WARN_ON(conn->c_passive);
+
+		spin_lock_irq(&loop_conns_lock);
+		list_del_init(&lc->loop_node);
+		spin_unlock_irq(&loop_conns_lock);
+
+		rds_conn_destroy(conn);
+		rds_conn_put(conn);
+	}
+}
+
+/* Gather @lc's connection for destruction: move the node to the
+ * caller's @tmp_list and take a reference that keeps the connection,
+ * and so the node, alive until rds_loop_destroy_gathered_conns() has
+ * dealt with it.  Called with loop_conns_lock held.  A connection
+ * whose free is already running gets no reference; its free unlinks
+ * the node itself, under the same lock, once we drop it.
+ */
+static void rds_loop_gather_conn(struct rds_loop_connection *lc,
+				 struct list_head *tmp_list)
+{
+	if (rds_conn_get_unless_zero(lc->conn))
+		list_move_tail(&lc->loop_node, tmp_list);
+}
+
 static void rds_loop_conn_free(void *arg)
 {
 	struct rds_loop_connection *lc = arg;
@@ -163,7 +202,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);
 }
@@ -187,14 +228,11 @@ void rds_loop_exit(void)
 	synchronize_rcu();
 	/* avoid calling conn_destroy with irqs off */
 	spin_lock_irq(&loop_conns_lock);
-	list_splice(&loop_conns, &tmp_list);
-	INIT_LIST_HEAD(&loop_conns);
+	list_for_each_entry_safe(lc, _lc, &loop_conns, loop_node)
+		rds_loop_gather_conn(lc, &tmp_list);
 	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);
 }
@@ -210,14 +248,11 @@ static void rds_loop_kill_conns(struct net *net)
 
 		if (net != c_net)
 			continue;
-		list_move_tail(&lc->loop_node, &tmp_list);
+		rds_loop_gather_conn(lc, &tmp_list);
 	}
 	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..552b32278e30 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -502,6 +502,48 @@ static bool rds_tcp_is_unloading(struct rds_connection *conn)
 	return atomic_read(&rds_tcp_unloading) != 0;
 }
 
+/* Gather @tc's connection for destruction: move the node to the
+ * caller's @tmp_list and take a reference that keeps the connection,
+ * and so the node, alive until rds_tcp_destroy_gathered_conns() has
+ * dealt with it.  Called with rds_tcp_conn_lock held.  A connection
+ * whose free is already running gets no reference; its free unlinks
+ * the node itself, under the same lock, once we drop it.
+ */
+static void rds_tcp_gather_conn(struct rds_tcp_connection *tc,
+				struct list_head *tmp_list)
+{
+	if (rds_conn_get_unless_zero(tc->t_cpath->cp_conn))
+		list_move_tail(&tc->t_tcp_node, tmp_list);
+}
+
+/* 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.
+ * Every entry holds a reference taken by rds_tcp_gather_conn(), so
+ * none can be freed under the walk; each is dropped after its destroy.
+ */
+static void rds_tcp_destroy_gathered_conns(struct list_head *tmp_list)
+{
+	struct rds_tcp_connection *tc, *_tc;
+	struct rds_connection *conn;
+
+	list_for_each_entry_safe(tc, _tc, tmp_list, t_tcp_node) {
+		conn = tc->t_cpath->cp_conn;
+
+		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(conn);
+		rds_conn_put(conn);
+	}
+}
+
 static void rds_tcp_destroy_conns(void)
 {
 	struct rds_tcp_connection *tc, *_tc;
@@ -511,12 +553,11 @@ static void rds_tcp_destroy_conns(void)
 	spin_lock_irq(&rds_tcp_conn_lock);
 	list_for_each_entry_safe(tc, _tc, &rds_tcp_conn_list, t_tcp_node) {
 		if (!list_has_conn(&tmp_list, tc->t_cpath->cp_conn))
-			list_move_tail(&tc->t_tcp_node, &tmp_list);
+			rds_tcp_gather_conn(tc, &tmp_list);
 	}
 	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);
@@ -691,15 +732,14 @@ static void rds_tcp_kill_sock(struct net *net)
 		if (net != c_net)
 			continue;
 		if (!list_has_conn(&tmp_list, tc->t_cpath->cp_conn)) {
-			list_move_tail(&tc->t_tcp_node, &tmp_list);
+			rds_tcp_gather_conn(tc, &tmp_list);
 		} else {
 			list_del(&tc->t_tcp_node);
 			tc->t_tcp_node_detached = true;
 		}
 	}
 	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 v4 08/12] 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.  Both ToS ioctls use the same lock now (they
  used the unrelated global rds_sock_lock before), and since the
  create in rds_sendmsg() samples rs_tos without the lock, the install
  re-checks under it that the new connection's ToS still matches the
  socket's and returns -EAGAIN if a SIOCRDSSETTOS slipped in between,
  rather than sending on, and caching, a connection with the old ToS.

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

rds_tcp_accept_one() needs no destroy check of its own even though it
now gets a referenced connection back from rds_conn_create(): TCP
connections are only destroyed on netns teardown and module unload,
and both stop the listener - flushing the accept work and clearing
the listen socket the accept tests first - before destroying anything.

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      |   9 +++-
 net/rds/loop.c       |   2 +-
 net/rds/rds.h        |   6 ++-
 net/rds/send.c       |  53 ++++++++++++++++--
 net/rds/tcp_listen.c |  11 +++-
 7 files changed, 210 insertions(+), 20 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 1d48da1a794f..965d68e51a1c 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -81,7 +81,10 @@ 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().
+ */
 static struct rds_connection *rds_conn_lookup(struct net *net,
 					      struct hlist_head *head,
 					      const struct in6_addr *laddr,
@@ -98,6 +101,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;
 		}
@@ -163,6 +177,14 @@ static void __rds_conn_path_init(struct rds_connection *conn,
 	cp->cp_flags = 0;
 }
 
+/* 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));
+}
+
 /* Undo trans->conn_alloc(): it may have allocated transport data for
  * every path of a multipath connection, not just for path 0.
  */
@@ -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)
@@ -672,6 +744,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);
 
@@ -702,7 +777,38 @@ void rds_conn_destroy(struct rds_connection *conn)
 
 	/* Ensure conn will not be scheduled for reconnect */
 	hlist_del_init_rcu(&conn->c_hash_node);
+
+	/* Snatch c_passive while holding the lock:
+	 * __rds_conn_create() dereferences it under rcu_read_lock()
+	 * (and refuses to install a new one once c_destroy_in_prog is
+	 * set, which it checks under this lock).  After the
+	 * synchronize_rcu() below no one can pick the pointer up any
+	 * more and its reference can be dropped.
+	 */
+	passive = rds_conn_passive_locked(conn);
+	RCU_INIT_POINTER(conn->c_passive, NULL);
+
+	/* If we are a parent's passive twin, invalidate its pointer to
+	 * us as well, so that __rds_conn_create() cannot hand out a
+	 * connection whose teardown has begun.  The parent is the
+	 * hashed connection for our key (a passive conn is never
+	 * hashed, and we unhashed ourselves above); it holds its
+	 * initial reference for as long as it is hashed, so the lookup
+	 * reference dropped below cannot be its last.
+	 */
+	head = rds_conn_bucket(&conn->c_laddr, &conn->c_faddr);
+	rcu_read_lock();
+	parent = rds_conn_lookup(rds_conn_net(conn), head, &conn->c_laddr,
+				 &conn->c_faddr, conn->c_trans, conn->c_tos,
+				 conn->c_dev_if);
+	rcu_read_unlock();
+	if (parent && rds_conn_passive_locked(parent) == conn) {
+		RCU_INIT_POINTER(parent->c_passive, NULL);
+		was_passive = true;
+	}
 	spin_unlock_irq(&rds_conn_lock);
+	if (parent)
+		rds_conn_put(parent);
 	synchronize_rcu();
 
 	/* shut the connection down */
@@ -719,6 +825,15 @@ void rds_conn_destroy(struct rds_connection *conn)
 	 */
 	rds_cong_remove_conn(conn);
 
+	/* drop the reference our c_passive pointer held, if any, and
+	 * the one a parent's c_passive pointer held on us; neither can
+	 * be the last, since the initial reference is dropped below
+	 */
+	if (passive)
+		rds_conn_put(passive);
+	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..98f34b494237 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -924,8 +924,15 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id,
 		rds_ib_conn_error(conn, "rdma_accept failed\n");
 
 out:
-	if (conn)
+	if (conn) {
 		mutex_unlock(&conn->c_cm_lock);
+		/* Drop the reference rds_conn_create() handed us.  The
+		 * conn stays reachable through cm_id->context without a
+		 * reference of its own for now; the CM event handler is
+		 * given one of its own by a following patch.
+		 */
+		rds_conn_put(conn);
+	}
 	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 71f760ccd458..5bac858df87e 100644
--- a/net/rds/loop.c
+++ b/net/rds/loop.c
@@ -170,7 +170,7 @@ static void rds_loop_destroy_gathered_conns(struct list_head *tmp_list)
 
 	list_for_each_entry_safe(lc, _lc, tmp_list, loop_node) {
 		conn = lc->conn;
-		WARN_ON(conn->c_passive);
+		WARN_ON(rcu_access_pointer(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 b3cc0804156e..321f2da9e76d 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;
@@ -669,7 +669,9 @@ struct rds_sock {
 
 	/*
 	 * rds_sendmsg caches the conn it used the last time around.
-	 * This helps avoid costly lookups.
+	 * This helps avoid costly lookups.  The cache owns a connection
+	 * reference, dropped when it is replaced or the socket is
+	 * released, and is read and written under rs_lock.
 	 */
 	struct rds_connection	*rs_conn;
 
diff --git a/net/rds/send.c b/net/rds/send.c
index 32c411d10e3e..2d7839438abd 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,28 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
 						scope_id);
 		if (IS_ERR(conn)) {
 			ret = PTR_ERR(conn);
+			conn = NULL;
 			goto out;
 		}
+		/* rs_tos was sampled without rs_lock for the create above,
+		 * and SIOCRDSSETTOS only refuses a change once rs_conn is
+		 * set, so it can have changed underneath us.  Do not
+		 * install - or send on - a connection whose ToS no longer
+		 * matches the socket's; the retry uses the new one.
+		 */
+		spin_lock_irqsave(&rs->rs_lock, flags);
+		if (conn->c_tos != rs->rs_tos) {
+			spin_unlock_irqrestore(&rs->rs_lock, flags);
+			ret = -EAGAIN;
+			goto out;
+		}
+		/* hand the cache its own reference */
+		rds_conn_get(conn);
+		old = rs->rs_conn;
 		rs->rs_conn = conn;
+		spin_unlock_irqrestore(&rs->rs_lock, flags);
+		if (old)
+			rds_conn_put(old);
 	}
 
 	if (conn->c_trans->t_mp_capable) {
@@ -1474,6 +1512,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 +1521,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..7fea5501d756 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.
@@ -277,6 +278,12 @@ 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);
+	/* A connection whose destroy has begun cannot be found here:
+	 * TCP connections are only destroyed on netns teardown and on
+	 * module unload, and both run rds_tcp_listen_stop() - which
+	 * flushes this work and clears the listen socket that the top
+	 * of this function tests - before any connection is destroyed.
+	 */
 	if (conn_state != RDS_CONN_CONNECTING && conn_state != RDS_CONN_ERROR) {
 		rds_conn_path_drop(cp, 0);
 		goto rst_nsk;
@@ -347,6 +354,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 v4 09/12] net/rds: refuse to queue 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.  rds_send_probe() adds to
cp_send_queue under cp_lock as well, for pings and pongs, and gets the
same test: a probe queued after the purge would pin the connection
just the same.  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       | 28 +++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 5 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index 965d68e51a1c..5699f45e4c37 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 2d7839438abd..f7bc4c5446d6 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;
@@ -1485,6 +1498,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()
@@ -1567,6 +1585,14 @@ rds_send_probe(struct rds_conn_path *cp, __be16 sport,
 		goto out;
 
 	spin_lock_irqsave(&cp->cp_lock, flags);
+	/* Same rule as rds_send_queue_rm(): once the destroy has purged
+	 * cp_send_queue under this lock, nothing may be added behind it.
+	 */
+	if (rds_destroy_pending(cp->cp_conn)) {
+		spin_unlock_irqrestore(&cp->cp_lock, flags);
+		ret = -EAGAIN;
+		goto out;
+	}
 	list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
 	set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
 	rds_message_addref(rm);
-- 
2.25.1

[PATCH net-next v4 10/12] 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.  Before this series that was already a use-after-free
whenever a callback destroyed the connection, since rds_conn_destroy()
freed it synchronously and the handler's mutex_unlock() ran on freed
memory; the one such callback, rds_ib_cm_connect_complete() on a
protocol version below 3.1, has meanwhile been switched to
rds_conn_drop() by commit f97d8c7bab78 ("rds: ib: use rds_conn_drop()
on protocol version mismatch"), which also removed the deadlock that
destroy took on c_cm_lock.

Now that a connection is freed by its last reference, the remaining
exposure is a callback that drops the last reference other than the
handler's - which has none - and the reference handed out by
rds_conn_create() to rds_ib_cm_handle_connect() and dropped at its
end.  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 +++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 98f34b494237..019956048873 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);
-- 
2.25.1

[PATCH net-next v4 11/12] 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.  (The free path
still sleeps in destroy_workqueue(), so the last reference has to be
dropped from process context as before; only that one lock goes.)

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 5699f45e4c37..546961bb2e12 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 v4 12/12] 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    | 26 +++++++++++++++++++++++---
 net/rds/send.c    |  4 ++++
 3 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/net/rds/message.c b/net/rds/message.c
index 47d5e9ab9b10..cea48ad745ca 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..1fcd4483d3be 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)
@@ -406,8 +425,9 @@ void rds_recv_incoming(struct rds_connection *conn, struct in6_addr *saddr,
 	 * 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.
+	 * pointing into netns A.  Cross-netns delivery is wrong on its
+	 * own, and the inc's connection reference would keep a
+	 * connection of a dead netns around, with a stale c_net.
 	 * Drop the inc if the receiving socket lives in a different netns.
 	 */
 	if (!net_eq(sock_net(rds_rs_to_sk(rs)), rds_conn_net(conn))) {
diff --git a/net/rds/send.c b/net/rds/send.c
index f7bc4c5446d6..afb22779ff97 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);
@@ -1596,6 +1599,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 v4 07/12] net/rds: unlink transport nodes before a possibly deferred connection free

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] rds: list corruption in rds_ib_destroy_nodev_conns() due to lockless tmp_list iteration
--

commit 74390d0096652a342da459a13d451c2fb0ee6d1f
Author: Allison Henderson <achender@kernel.org>
Subject: net/rds: unlink transport nodes before a possibly deferred connection free

This patch modifies transport teardown helpers in RDS to unlink transport nodes
right before calling rds_conn_destroy(). It ensures that nodes don't remain on
stack-allocated list heads when connections are kept alive by external
references.

> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index a9b27f06cbfcf..bdd66c9b93c70 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c
> @@ -123,15 +123,18 @@ void rds_ib_add_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *con
>  {
>  	struct rds_ib_connection *ic = conn->c_transport_data;
>  
> -	/* conn was previously on the nodev_conns_list */
> +	/* conn was previously on the nodev_conns_list, unless a teardown
> +	 * sweep already unlinked it ahead of destroying it: then it is
> +	 * on its way out and stays off every list.
> +	 */
>  	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);
> +	if (!list_empty(&ic->ib_node)) {
> +		list_del(&ic->ib_node);

[Severity: High]
Does this correctly identify if a node was already unlinked by a teardown
sweep? 

Since the teardown sweep in rds_ib_destroy_nodev_conns() moves the node to a
local tmp_list, list_empty(&ic->ib_node) will evaluate to false here.

Could this cause list_del(&ic->ib_node) to inadvertently unlink the node
from tmp_list while the teardown sweep is actively iterating over it?

>  
> -	spin_lock(&rds_ibdev->spinlock);
> -	list_add_tail(&ic->ib_node, &rds_ibdev->conn_list);
> -	spin_unlock(&rds_ibdev->spinlock);
> +		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);

[ ... ]

> @@ -163,13 +173,38 @@ void rds_ib_destroy_nodev_conns(void)
>  	struct rds_ib_connection *ic, *_ic;
>  	LIST_HEAD(tmp_list);
>  
> -	/* avoid calling conn_destroy with irqs off */
> +	struct rds_connection *conn;
> +
> +	/* Gather the connections and take a reference on each, so that
> +	 * none is freed under the walk below (a connection destroyed
> +	 * earlier, for a protocol version mismatch, can be on this list
> +	 * with only a socket's reference still pending).  One whose free
> +	 * is already running gets no reference: its free unlinks the
> +	 * node itself, under this lock, once we drop it.  Avoid calling
> +	 * conn_destroy with irqs off.
> +	 */
>  	spin_lock_irq(&ib_nodev_conns_lock);
> -	list_splice_init(&ib_nodev_conns, &tmp_list);
> +	list_for_each_entry_safe(ic, _ic, &ib_nodev_conns, ib_node) {
> +		if (rds_conn_get_unless_zero(ic->conn))
> +			list_move_tail(&ic->ib_node, &tmp_list);
> +	}
>  	spin_unlock_irq(&ib_nodev_conns_lock);
>  
> -	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
> -		rds_conn_destroy(ic->conn);
> +	/* rds_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) {
> +		conn = ic->conn;
> +
> +		spin_lock_irq(&ib_nodev_conns_lock);
> +		list_del_init(&ic->ib_node);
> +		spin_unlock_irq(&ib_nodev_conns_lock);

[Severity: High]
Is it safe to iterate over tmp_list locklessly here?

If a concurrent worker calls rds_ib_add_conn(), it will unlink the node from
tmp_list. If the unlinked node happens to be the prefetched _ic pointer in
this loop, its next and prev pointers become LIST_POISON1 and LIST_POISON2,
which can cause a kernel panic on the next loop iteration.

Additionally, if the concurrent rds_ib_add_conn() has already moved the node
onto rds_ibdev->conn_list, could the list_del_init(&ic->ib_node) here
improperly modify rds_ibdev->conn_list without holding rds_ibdev->spinlock?

> +
> +		rds_conn_destroy(conn);
> +		rds_conn_put(conn);
> +	}
>  }

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

Re: [PATCH net-next v4 03/12] 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
> [ ... ]
> single connection.
> 
> 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/20260917073958.174056-1-achender@kernel.org?part=3

Re: [PATCH net-next v4 02/12] 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/20260917073958.174056-1-achender@kernel.org?part=2

Re: [PATCH net-next v4 05/12] 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/20260917073958.174056-1-achender@kernel.org?part=5

Re: [PATCH net-next v4 04/12] 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 - apart from the workers' own self-requeues, which the sync
> cancel in the destroy path already rejects - 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/20260917073958.174056-1-achender@kernel.org?part=4

Re: [PATCH net-next v4 06/12] 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/20260917073958.174056-1-achender@kernel.org?part=6

Re: [PATCH net-next v4 08/12] 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/20260917073958.174056-1-achender@kernel.org?part=8

Re: [PATCH net-next v4 01/12] 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/20260917073958.174056-1-achender@kernel.org?part=1

Re: [PATCH net-next v4 09/12] net/rds: refuse to queue 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.  rds_send_probe() adds to
> cp_send_queue under cp_lock as well, for pings and pongs, and gets the
> same test: a probe queued after the purge would pin the connection
> just the same.  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/20260917073958.174056-1-achender@kernel.org?part=9

Re: [PATCH net-next v4 10/12] 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.  Before this series that was already a use-after-free
> whenever a callback destroyed the connection, since rds_conn_destroy()
> freed it synchronously and the handler's mutex_unlock() ran on freed
> memory; the one such callback, rds_ib_cm_connect_complete() on a
> protocol version below 3.1, has meanwhile been switched to
> rds_conn_drop() by commit f97d8c7bab78 ("rds: ib: use rds_conn_drop()
> on protocol version mismatch"), which also removed the deadlock that
> destroy took on c_cm_lock.
> 
> Now that a connection is freed by its last reference, the remaining
> exposure is a callback that drops the last reference other than the
> handler's - which has none - and the reference handed out by
> [ ... ]
> 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/20260917073958.174056-1-achender@kernel.org?part=10

Re: [PATCH net-next v4 11/12] 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.  (The free path
> still sleeps in destroy_workqueue(), so the last reference has to be
> dropped from process context as before; only that one lock goes.)
> 
> 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/20260917073958.174056-1-achender@kernel.org?part=11

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

[sashiko-bot]

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

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

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

Re: [PATCH net-next v4 02/12] 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 (1) · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] Commit message misstates reachability of the leak it claims to
  fix (and carries a Fixes: tag that will pull it into stable as a leak…

Pre-existing issues:
- [High] Create/destroy npaths disagreement leaks per-path ordered
  workqueues for loopback-substituted RDS/TCP connections.

--

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

This moves the per-path transport-data free loop of __rds_conn_create()
into a new helper, rds_conn_free_transport_data(), and calls it from the
passive-conn exit as well as from the lost-creation-race exit.  The
changelog describes the passive exit as leaking transport data for paths
1..npaths-1 of a multipath connection.

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

[ ... ]

> @@ -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: Medium]
Can this exit ever run with npaths > 1?

The changelog says:

    RDS/TCP loopback is exactly a multipath passive connection, so this
    is reachable.

npaths comes from the caller's transport at the top of
__rds_conn_create():

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

and t_mp_capable is set only by rds_tcp_transport in net/rds/tcp.c.

parent is only set when the initial rds_conn_lookup() finds a conn
matching:

	if (conn &&
	    conn->c_loopback &&
	    conn->c_trans != &rds_loop_transport &&
	    ipv6_addr_equal(laddr, faddr) &&
	    !is_outgoing) {

and rds_conn_lookup() also requires conn->c_trans == trans, so the found
conn's transport is the caller's transport.

c_loopback is set in only one place, and for TCP (t_prefer_loopback == 1)
that same block either rewrites the transport or fails the request:

		conn->c_loopback = 1;
		if (trans->t_prefer_loopback) {
			if (likely(is_outgoing)) {
				trans = &rds_loop_transport;
			} else {
				...
				conn = ERR_PTR(-EOPNOTSUPP);
				goto out;
			}
		}

Since conn->c_trans = trans is assigned afterwards, a hashed conn with
c_loopback == 1 and c_trans == &rds_tcp_transport does not seem to exist,
which is also why the in-tree comment on that lookup says "This is a
looped back IB connection".  rds_tcp_accept_one() additionally rejects a
local peer address with -EOPNOTSUPP before reaching rds_conn_create().

If that is right, the only transport reaching this branch is IB, which
has t_mp_capable == 0, so npaths == 1 and the previous single
trans->conn_free(conn->c_path[0].cp_transport_data) already freed
everything.  Would it be more accurate to describe this as an equivalent
refactor / hardening change rather than a leak fix, and to drop the
Fixes: 1c5113cf796b tag so it is not picked up for stable as a leak fix?

[Severity: High]
This isn't a bug introduced by this patch, but while looking at how
npaths is used on these rollback paths: does the create/destroy npaths
disagreement leak the per-path ordered workqueues for loopback-substituted
RDS/TCP connections?

__rds_conn_create() computes npaths from the caller's transport (8 for
TCP), then substitutes the loop transport for an outgoing connection to a
local address:

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

npaths is not recomputed, so the init loop still allocates a workqueue for
all 8 paths:

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

rds_loop_conn_alloc() only sets conn->c_transport_data, which
rds_single_path.h maps to c_path[0].cp_transport_data, so paths 1..7 keep
cp_transport_data == NULL.

rds_conn_destroy() then recomputes npaths from conn->c_trans, which is now
the loop transport:

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

so only path 0 is torn down, and rds_conn_path_destroy() would skip the
rest anyway:

	if (!cp->cp_transport_data)
		return;

before it reaches destroy_workqueue(cp->cp_wq).  kfree(conn->c_path) then
drops the last pointers to the 7 remaining workqueues.

The reachable sequence looks like: an AF_RDS socket using the TCP
transport sends to a local address, rds_conn_create_outgoing() ->
__rds_conn_create() builds the loop-substituted conn with 8 workqueues,
and netns exit (rds_loop_exit_net -> rds_loop_kill_conns ->
rds_conn_destroy) or module unload leaves 7 unbound ordered workqueues per
such conn.  Distinct 127.0.0.0/8 destinations create more conns, so this
repeats.

The same shape appears at the end of the series in rds_conn_destroy_fini(),
which derives npaths from conn->c_trans, while rds_conn_path_free() still
early-returns on !cp_transport_data before destroy_workqueue().

Would it make sense to store the allocated path count on the connection
(or recompute npaths after the loop-transport substitution) so create and
destroy always agree, and to destroy cp_wq independently of
cp_transport_data?

[ ... ]

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

Re: [PATCH net-next v4 04/12] 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: 1 · Medium: 0 · Low: 1 (1)

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Unconditional self-deadlock in the very per-connection destroy
  path this patch is written for.
- [Low] The two comments added by this patch state an invariant the tree
  does not maintain.

Pre-existing issues:
- [Low] Stale contract comment above rds_conn_destroy()
  (net/rds/connection.c:564-570) still claims 'We can only ensure this…

--

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

This adds a per-connection conn->c_destroy_in_prog flag, set by
rds_conn_destroy() before the unhash plus synchronize_rcu() sequence, and
tests it from rds_destroy_pending() so that the work-requeueing sites stop
arming path works on a connection that is being torn down.  The now unused
RDS_DESTROY_PENDING cp_flags bit and its dead test in
rds_ib_is_unloading() are removed.

A few questions on the new comments and on the per-connection destroy path
this is aimed at.

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

[Severity: Low]

This isn't a bug introduced by this patch, but the contract comment
immediately above rds_conn_destroy() 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.
   */

The flag added below, and its comment in net/rds/rds.h, describe protection
for the non-rmmod case, and in-tree callers such as
rds_ib_cm_connect_complete() (net/rds/ib_cm.c) destroy a single connection
outside rmmod.  Later in this series "net/rds: split connection destroy into
quiesce and kref-governed free" replaces that comment, so is it worth
refreshing it here so the two do not disagree in between?

>  		 "%pI4\n", conn, &conn->c_laddr,
>  		 &conn->c_faddr);
>  
> +	/* Make rds_destroy_pending() true for this conn.  Together with
> +	 * the synchronize_rcu() below this stops the work-requeueing
> +	 * sites (which all test rds_destroy_pending() under
> +	 * rcu_read_lock()) from queueing new work on the path
> +	 * workqueues once we start cancelling and destroying them.
> +	 */

[Severity: Low]

Is the parenthetical "which all test rds_destroy_pending() under
rcu_read_lock()" accurate for the tree as it stands?  The same claim is made
unconditionally in the new struct rds_connection comment:

  /* ... A site that arms
   * a path work must test the predicate and queue the work inside
   * one rcu_read_lock() section: ...
   */

Three arming sites do neither.  rds_send_worker() and rds_recv_worker() in
net/rds/threads.c re-arm their own works with no predicate and no RCU
section:

	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;

And rds_conn_path_drop() bypasses the predicate on purpose for destroy:

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

that being the call rds_conn_path_destroy() makes with destroy == true,
after c_destroy_in_prog has already been set.  The commit message does note
the worker self-requeues and the sync cancel that covers them, but the
in-tree comments do not mention either exception.  Could the comments spell
out that the workers and the destroy == true drop are covered by the sync
cancel and by the destroy path driving its own final shutdown pass instead?

> +	WRITE_ONCE(conn->c_destroy_in_prog, true);

[Severity: High]

Can the single-connection destroy this flag is written for actually
complete?  The RDMA CM handler holds conn->c_cm_lock (which is
conn->c_path[0].cp_cm_lock, see net/rds/rds_single_path.h) across the
ESTABLISHED dispatch:

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

and the version-mismatch destroy runs from inside that dispatch:

net/rds/ib_cm.c:rds_ib_cm_connect_complete() {
	if (conn->c_version < RDS_PROTOCOL_VERSION) {
		if (conn->c_version != RDS_PROTOCOL_COMPAT_VERSION) {
			pr_notice(...);
			rds_conn_destroy(conn);
			return;
	...
}

rds_conn_destroy() then reaches rds_conn_path_destroy(), which queues
cp_down_w with the guard bypassed and waits for it while still holding
cp_cm_lock:

net/rds/connection.c:rds_conn_path_destroy() {
	rds_conn_path_drop(cp, true);	/* cp_state = RDS_CONN_ERROR, queue */
	flush_work(&cp->cp_down_w);
	...
}

The shutdown worker fails the DOWN to DOWN transition because the state is
RDS_CONN_ERROR, and then takes the mutex the flusher owns:

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

IB is not t_mp_capable, so npaths is 1 and the path being destroyed is
exactly the one whose cp_cm_lock is held.  ERROR to DISCONNECTING is only
performed under cp_cm_lock, so no other pass can unblock the transition
either.  Does this leave the CM event thread parked in flush_work() holding
cp_cm_lock and the path worker parked in mutex_lock() behind it, with the
peer-supplied CM private data (major 3, minor 0) as the trigger?

If so, the motivating scenario in the commit message, "queue_delayed_work()
on a cp_wq that destroy_workqueue() has already freed", would not be
reachable from this caller, because the destroy hangs before
destroy_workqueue() runs.

There is a related use-after-free on the same path at the handler tail:

net/rds/rdma_transport.c:rds_rdma_cm_event_handler_cmn() {
out:
	if (conn)
		mutex_unlock(&conn->c_cm_lock);
	...
}

which runs on memory already returned by kfree(conn->c_path) and
kmem_cache_free(rds_conn_slab, conn) in rds_conn_destroy().  That part is
addressed later in this series by "net/rds: pin the connection across
RDMA-CM event handling", but the lock re-entry above appears to survive to
the end of the series, where rds_conn_path_quiesce() still does
rds_conn_path_drop(cp, true) plus flush_work(&cp->cp_down_w) and
net/rds/ib_cm.c still calls rds_conn_destroy() from the CM callback.  Would
deferring that destroy out of the CM callback be the better fix for this
site?

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

[ ... ]

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

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

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The freeing half of teardown (destroy_workqueue() of the per-path
  workqueues in rds_conn_path_free(), net/rds/connection.c:565-576, plus…
- [Low] The destruction-protocol comments state that every site arming a
  path work tests rds_destroy_pending() inside one rcu_read_lock()…
- [Low] The new duplicate-call branch in rds_conn_destroy()
  (net/rds/connection.c:646-651) returns as soon as c_destroy_in_prog is…

Pre-existing issues:
- [High] Unsupported-protocol-version handling deadlocks the RDS/IB
  connection teardown: rds_rdma_cm_event_handler_cmn()…

--

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

This patch adds a kref to struct rds_connection, keeps the synchronous
quiesce work in rds_conn_destroy() and moves the actual frees (transport
conn_free, path workqueues, c_path array, slab object) into
rds_conn_destroy_fini(), which runs from the last rds_conn_put().
rds_conn_get()/rds_conn_put() plus an inline rds_conn_get_unless_zero()
are exported for the reference holders added in the following patches.

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 quiesce (and its
flush_work()) stays synchronous inside rds_conn_destroy(), can this
teardown still deadlock when a peer negotiates an unsupported protocol
version?

The RDMA-CM handler calls the transport callback with the connection
management mutex held:

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

and for a version mismatch that callback destroys the connection:

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

rds_conn_destroy() -> rds_conn_path_quiesce() then queues cp_down_w and
waits for it:

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

while the worker it waits for needs the same mutex:

net/rds/connection.c:rds_conn_shutdown() {
	...
	mutex_lock(&cp->cp_cm_lock);
}

For IB (single path) net/rds/rds_single_path.h aliases c_cm_lock to
c_path[0].cp_cm_lock, so the CM handler and the shutdown worker are
waiting on each other, and the version field comes from the peer's CM
private data.

Upstream commit f97d8c7bab78 ("rds: ib: use rds_conn_drop() on protocol
version mismatch", Cc: stable) addresses exactly this by not calling
rds_conn_destroy() there, but that change is not in this baseline -
rds_ib_cm_connect_complete() still calls rds_conn_destroy() both here and
at the tip of this series.  The later series patch "net/rds: pin the
connection across RDMA-CM event handling" states that the call was already
removed; is that accurate for this tree?

At the tip of the series this also turns transport unload into an
unbounded wait, since rds_conn_wait_conns_freed() in rds_ib_exit() polls
for a t_conn_count that a never-freed connection cannot drop.

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

[Severity: Low]
Would it be worth spelling out the calling-context contract that
rds_conn_put() now carries?

kref_put() runs the release in the caller's context, and
rds_conn_destroy_fini() -> rds_conn_path_free() sleeps:

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

destroy_workqueue() calls drain_workqueue(), mutex_lock(&wq->mutex) and
kthread_stop(), and in this commit rds_conn_destroy_fini() also takes
rds_conn_lock, so a last put issued under rds_conn_lock would recurse on
it.  That rds_conn_lock use is removed later in the series by "net/rds:
drop rds_conn_count in favor of t_conn_count", but the sleeping part
remains.

At this commit the only put site is the tail of rds_conn_destroy(), which
is process context and holds no lock, so nothing is broken today.  Should
rds_conn_put() carry a might_sleep() and a comment on the prototype, so
the future holders added by the follow-up patches have the requirement
stated in the header rather than only in commit messages?

>  /*
>   * Stop and free a connection.
>   *

[ ... ]

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

[Severity: Low]
This isn't a bug, but is the "all test rds_destroy_pending() under
rcu_read_lock()" wording too broad?

The workers requeue themselves without the predicate and without an RCU
section:

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

net/rds/threads.c:rds_recv_worker() {
	...
	case -EAGAIN:
		rds_stats_inc(s_recv_immediate_retry);
		queue_delayed_work(cp->cp_wq, &cp->cp_recv_w, 0);
}

The same claim also appears on c_destroy_in_prog 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").  The self-requeues are safe because
rds_conn_path_quiesce() cancels those works with
cancel_delayed_work_sync(), which is what the earlier series patch
"net/rds: guard every work-requeueing site with rds_destroy_pending()"
explains, and the over-broad clause came from "net/rds: make
rds_destroy_pending() cover single-connection destroy" rather than this
patch.  Could both comments be qualified with something like "apart from
the workers' own self-requeues"?

> +	 *
> +	 * 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: Low]
What does a second caller learn from this early return?  c_destroy_in_prog
is set here, before synchronize_rcu() and before any path is quiesced, so
on return the first caller may still be inside rds_conn_path_quiesce().

Meanwhile the new comment in net/rds/rds.h promises unconditionally:

	/* rds_conn_destroy() quiesces the connection synchronously;
	 * freeing it - the connection memory, the path workqueues and
	 * the transport's per-connection state - is deferred until the
	 * last reference is dropped via rds_conn_put().
	 */

The only real second destroyer today is rds_ib_cm_connect_complete()
racing the rds_ib_exit() sweep; the TCP and loopback sweeps unlink each
node under the transport lock first, and rds_ib_remove_one() only drops
the path.  The next patch in the series adds rds_conn_wait_conns_freed()
to rds_ib_exit(), which closes the window where the sweep returns while
another thread is still quiescing, so this is only about the wording.
Could the rds.h comment note that a duplicate rds_conn_destroy() call
returns without waiting for the in-flight quiesce?

> diff --git a/net/rds/rds.h b/net/rds/rds.h
> index 50b08c28ab865..defda3ddefa32 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;
> +	/* rds_conn_destroy() quiesces the connection synchronously;
> +	 * freeing it - the connection memory, the path workqueues and
> +	 * the transport's per-connection state - is deferred until the
> +	 * last reference is dropped via rds_conn_put().
> +	 */
> +	struct kref		c_refcount;

[ ... ]

> @@ -826,6 +832,13 @@ struct rds_connection *rds_conn_create_outgoing(struct net *net,
>  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);
> +}

Thanks for looking at these.

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

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] rds_ib_exit()'s new wait can never complete for a connection
  whose rds_conn_destroy() is issued from the RDMA-CM event handler, so…
- [Medium] rds_conn_wait_conns_freed() (net/rds/connection.c) is an
  unbounded TASK_UNINTERRUPTIBLE poll loop executed from module exit…
- [Medium] At this commit, list_splice_init() empties ib_nodev_conns
  while the swept connections' ib_node entries sit on the rmmod thread's…
- [Low] Undocumented, load-bearing semantic change in net/rds/ib_rdma.c:
  rds_ib_destroy_nodev_conns() is switched from list_splice() to…
- [Low] The kernel-doc-style comment on rds_conn_wait_conns_freed()
  states a precondition its own IB caller deliberately violates: 'Called…

Pre-existing issues:
- [Medium] Queued incoming datagrams hold no connection reference at this
  commit, so t_conn_count can reach zero and permit transport unload…

--

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

This adds a per-transport live-connection counter (t_conn_count),
incremented when a connection is published in __rds_conn_create() and
decremented at the end of rds_conn_destroy_fini().  The transport exit
paths (rds_ib_exit(), rds_tcp_exit(), rds_loop_exit()) then poll that
count until it reaches zero before letting the module unload proceed.

A few questions below.

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

[ ... ]

> @@ -596,7 +601,52 @@ static void rds_conn_destroy_fini(struct kref *kref)
>  	spin_lock_irqsave(&rds_conn_lock, flags);
>  	rds_conn_count--;
>  	spin_unlock_irqrestore(&rds_conn_lock, flags);
> +
> +	/* only after everything the transport module owns has been
> +	 * freed above may its unload proceed
> +	 */
> +	if (!atomic_dec_return(&trans->t_conn_count))
> +		wake_up_all(&rds_conn_freed_waitq);

[Severity: Medium]

This isn't a bug introduced by this patch, but at this commit a queued
incoming datagram holds no connection reference: rds_inc_init() and
rds_inc_path_init() in net/rds/recv.c store inc->i_conn without calling
rds_conn_get().

So t_conn_count can reach zero and let the transport unload while an
unread inc still carries inc->i_conn and dispatches through that
transport's callbacks:

net/rds/recv.c:rds_inc_put() {
	...
	inc->i_conn->c_trans->inc_free(inc);
}

The commit message is explicit that this only closes "once the following
patches make incs hold a connection reference", and the later patch
"net/rds: hold a connection reference from struct rds_incoming" does add
rds_conn_get() in rds_inc_init()/rds_inc_path_init().  Noting it here only
so the intermediate state in the series is on record - would it be worth
saying in the changelog that the guarantee is incomplete until that patch
lands?

> +}
> +
> +/* 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 - once incs hold one, 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]

Is the stated precondition the one the code actually has?  The comment
says the function is "Called on transport module unload, after the
transport has destroyed all of its connections", but rds_ib_exit() calls
it while connections are still attached to devices and migrating to
ib_nodev_conns asynchronously - which is why it passes
rds_ib_destroy_nodev_conns as the resweep callback and why the loop body
calls resweep() after every timeout.

Could the comment describe the real contract instead, i.e. that the caller
does an initial sweep and the resweep destroys late arrivals during the
wait?

Also, the sentence "so the wait is unbounded: the" / "frees that run after
unload call into this module's text" looks like a mangled line break.

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

[Severity: Medium]

Can an unprivileged application wedge rmmod permanently here?  The loop
has no timeout, no signal check and no bail-out, and wait_event_timeout()
sleeps in TASK_UNINTERRUPTIBLE.

By the end of the series an inc holds a connection reference, so a process
that binds an AF_RDS socket, receives one datagram and never calls
recvmsg() keeps t_conn_count non-zero for as long as it likes.

The transport module reference a bound socket holds does not cover this
case: rds_bind() can take rs->rs_transport preset via SO_RDS_TRANSPORT,
and rds_find_bound() does not filter on transport, so the socket pinning
an IB connection's inc need hold no rds_rdma reference at all - which the
commit message itself notes.

delete_module() has already moved the module to MODULE_STATE_GOING before
calling mod->exit(), so at that point the module can neither finish
unloading nor be re-inserted, and the rmmod task is unkillable.  The same
applies to rds_tcp_exit(), where the wait runs after
unregister_pernet_device() and rds_tcp_destroy_conns(), and to
rds_loop_exit() in the core module.

Was having long-term reference holders take a module reference considered
instead, so delete_module() fails with -EBUSY rather than blocking inside
->exit()?

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

[Severity: High]

Can this wait ever finish for a connection whose destroy was issued from
the RDMA-CM event handler?

net/rds/ib_cm.c:rds_ib_cm_connect_complete() destroys the connection on a
protocol version mismatch:

	rds_conn_destroy(conn);
	return;

and it runs with conn->c_cm_lock (c_path[0].cp_cm_lock) held by
rds_rdma_cm_event_handler_cmn() in net/rds/rdma_transport.c:

	if (conn) {
		mutex_lock(&conn->c_cm_lock);
	...
	case RDMA_CM_EVENT_ESTABLISHED:
		if (conn)
			trans->cm_connect_complete(conn, event);

The destroy path is then:

rds_conn_destroy() -> 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 this thread */

rds_conn_destroy_fini() is never reached, so
atomic_dec_return(&trans->t_conn_count) never runs and this wait loops
forever, printing the 10 s warning indefinitely, with no indication of
which connection is stuck.  The protocol version that triggers it is
chosen by the remote peer.

This state persists to the end of the series: net/rds/ib_cm.c still calls
rds_conn_destroy(conn) at that site, even though the later commit
"net/rds: pin the connection across RDMA-CM event handling" says it was
switched to rds_conn_drop().  Should that conversion come with this patch,
since this is the change that turns a stuck destroy into an unbounded
uninterruptible wait?

Separately, the changelog states the wake at the end of
rds_conn_destroy_fini() "can run from a thread executing transport module
text, but never as that thread's last use of it".  Only a few core-module
calls separate the wait's return from the module text being freed - is
there anything in the code that enforces that ordering, or is it an
argument about the current call sites only?

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

[Severity: Low]

Could this one-line change get a mention in the changelog?  It looks
load-bearing for the new resweep: rds_ib_exit() now passes
rds_ib_destroy_nodev_conns as the resweep callback, so it runs every
100 ms, and plain list_splice() leaves ib_nodev_conns still pointing at
entries that were moved onto the caller's stack list, since __list_splice()
does not re-init the source head.  The commit message describes the resweep
at length but not this fix, which also reads like a separate preparatory
change.

[Severity: Medium]

At this commit, does emptying ib_nodev_conns while the swept entries live
on the rmmod thread's stack list conflict with rds_ib_add_conn()?  That
function still has:

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

A connect worker already in flight when rds_ib_exit() runs would either hit
the assertion or unlink an entry from the sweeper's stack-local list.

The next patch in the series, "net/rds: unlink transport nodes before a
possibly deferred connection free", removes both BUG_ONs and rewrites the
sweep to take a reference per entry and list_del_init() each node under the
lock just before its destroy, so the window is confined to this commit -
would it be simpler to fold the two together?

[ ... ]

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

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] rds_ib_destroy_nodev_conns() leaves each gathered ib_node
  *linked* on its stack-local tmp_list and only unlinks it…
- [Low] The comment above the lock selection in rds_ib_conn_free() still
  asserts the two-state invariant 'Conn is either on a dev's list or on…

--

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

The transport teardown helpers now unlink each per-connection transport node
under the transport lock right before calling rds_conn_destroy(), instead of
leaving that to the transport's conn_free().  The gather step also takes a
reference on each connection it moves onto the stack list and drops it after
the destroy, and the IB list movers no longer assert that a node is linked.

> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 118e033229aa8..26a32c1ec8f7f 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: Low]
This isn't a bug, but should the comment just above the lock selection in
rds_ib_conn_free() be updated too?  It still reads:

	/*
	 * 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.
	 ...
	lock_ptr = ic->rds_ibdev ? &ic->rds_ibdev->spinlock : &ib_nodev_conns_lock;

After this patch "on neither list" is a normal state, which is exactly what
the new list_empty() guard below handles.  rds_ib_add_conn() can now set
ic->rds_ibdev and take a device reference without linking ic->ib_node onto
rds_ibdev->conn_list, and rds_ib_remove_conn() can clear ic->rds_ibdev
without putting the node back on ib_nodev_conns.

The comment also still denies the connect/shutdown race that the new
comments in rds_ib_add_conn() and rds_ib_remove_conn() say can happen.

> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index a9b27f06cbfcf..bdd66c9b93c70 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c
> @@ -123,15 +123,18 @@ void rds_ib_add_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *con
>  {
>  	struct rds_ib_connection *ic = conn->c_transport_data;
>  
> -	/* conn was previously on the nodev_conns_list */
> +	/* conn was previously on the nodev_conns_list, unless a teardown
> +	 * sweep already unlinked it ahead of destroying it: then it is
> +	 * on its way out and stays off every list.
> +	 */
>  	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);
> +	if (!list_empty(&ic->ib_node)) {
> +		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_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;
> @@ -142,15 +145,22 @@ void rds_ib_remove_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *
>  {
>  	struct rds_ib_connection *ic = conn->c_transport_data;
>  
> -	/* place conn on nodev_conns_list */
> +	bool detached;
> +
> +	/* place conn on nodev_conns_list - unless a teardown sweep
> +	 * already unlinked it ahead of destroying it, in which case it
> +	 * stays off every list
> +	 */
>  	spin_lock(&ib_nodev_conns_lock);
>  
>  	spin_lock_irq(&rds_ibdev->spinlock);
> -	BUG_ON(list_empty(&ic->ib_node));
> -	list_del(&ic->ib_node);
> +	detached = list_empty(&ic->ib_node);
> +	if (!detached)
> +		list_del(&ic->ib_node);
>  	spin_unlock_irq(&rds_ibdev->spinlock);
>  
> -	list_add_tail(&ic->ib_node, &ib_nodev_conns);
> +	if (!detached)
> +		list_add_tail(&ic->ib_node, &ib_nodev_conns);
>  
>  	spin_unlock(&ib_nodev_conns_lock);
>  
> @@ -163,13 +173,38 @@ void rds_ib_destroy_nodev_conns(void)
>  	struct rds_ib_connection *ic, *_ic;
>  	LIST_HEAD(tmp_list);
>  
> -	/* avoid calling conn_destroy with irqs off */
> +	struct rds_connection *conn;
> +
> +	/* Gather the connections and take a reference on each, so that
> +	 * none is freed under the walk below (a connection destroyed
> +	 * earlier, for a protocol version mismatch, can be on this list
> +	 * with only a socket's reference still pending).  One whose free
> +	 * is already running gets no reference: its free unlinks the
> +	 * node itself, under this lock, once we drop it.  Avoid calling
> +	 * conn_destroy with irqs off.
> +	 */
>  	spin_lock_irq(&ib_nodev_conns_lock);
> -	list_splice_init(&ib_nodev_conns, &tmp_list);
> +	list_for_each_entry_safe(ic, _ic, &ib_nodev_conns, ib_node) {
> +		if (rds_conn_get_unless_zero(ic->conn))
> +			list_move_tail(&ic->ib_node, &tmp_list);
> +	}
>  	spin_unlock_irq(&ib_nodev_conns_lock);
>  
> -	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
> -		rds_conn_destroy(ic->conn);
> +	/* rds_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) {
> +		conn = ic->conn;
> +
> +		spin_lock_irq(&ib_nodev_conns_lock);
> +		list_del_init(&ic->ib_node);
> +		spin_unlock_irq(&ib_nodev_conns_lock);
> +
> +		rds_conn_destroy(conn);
> +		rds_conn_put(conn);
> +	}
>  }

[Severity: High]
Can a concurrent rds_ib_add_conn() steal an entry out of the tmp_list that
rds_ib_destroy_nodev_conns() keeps on its own stack?

The gather in rds_ib_destroy_nodev_conns() leaves each node linked, just on
a different list head:

	list_for_each_entry_safe(ic, _ic, &ib_nodev_conns, ib_node) {
		if (rds_conn_get_unless_zero(ic->conn))
			list_move_tail(&ic->ib_node, &tmp_list);
	}

The new "has a sweep already claimed me" test in rds_ib_add_conn() is
list_empty(&ic->ib_node), which is false for a node parked on tmp_list:

	spin_lock_irq(&ib_nodev_conns_lock);
	if (!list_empty(&ic->ib_node)) {
		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);
	}

So rds_ib_setup_qp()->rds_ib_add_conn() from the RDMA-CM event path would
treat a gathered node as still being on ib_nodev_conns and move it onto
rds_ibdev->conn_list.  rds_ib_remove_conn() does the mirror image, unlinking
a gathered node while holding only rds_ibdev->spinlock and re-adding it to
ib_nodev_conns.

The second loop in rds_ib_destroy_nodev_conns() walks tmp_list without
ib_nodev_conns_lock, and each iteration is long, since rds_conn_destroy()
does synchronize_rcu() plus work cancel/flush.  If the entry the iterator
is about to follow has been moved, doesn't the walk continue onto
rds_ibdev->conn_list or ib_nodev_conns?

That walk would then call rds_conn_destroy() and rds_conn_put() on
connections for which the gather never took a reference, and the loop's
termination test pos == &tmp_list can no longer fire on the foreign chain,
so the conn_list head embedded in struct rds_ib_device is eventually treated
as a struct rds_ib_connection and ic->conn read from it.

The list_del_init(&ic->ib_node) in that loop is also done under
ib_nodev_conns_lock, while a stolen node now lives on rds_ibdev->conn_list,
which is protected by rds_ibdev->spinlock and walked concurrently by
rds_ib_dev_shutdown().

And if a stolen entry is never reached by the walk, the reference the gather
took on it is never dropped, so does the resweep loop in rds_ib_exit():

	rds_ib_destroy_nodev_conns();
	rds_conn_wait_conns_freed(&rds_ib_transport,
				  rds_ib_destroy_nodev_conns);

keep spinning with "still waiting for N connection(s) to be freed before
unload" because t_conn_count never reaches zero?

The commit message says "a connect or shutdown worker can still be running
for a connection the sweep has just unlinked", and the gathered window is
earlier and longer than the unlinked one the guard was written for.  TCP
records this state explicitly with tc->t_tcp_node_detached; would IB need an
equivalent recorded under the transport lock at gather time, rather than
relying on list_empty()?

[ ... ]

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

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

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

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

--

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

	conn = rds_conn_create_outgoing(sock_net(sock->sk),
					&rs->rs_bound_addr, &daddr,
					rs->rs_transport, rs->rs_tos,
					sock->sk->sk_allocation,
					scope_id);

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

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

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

[ ... ]

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

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

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

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

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

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

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

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

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

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

[ ... ]

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

[ ... ]

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

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

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

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

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

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

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

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

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

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

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

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

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

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 32c411d10e3ef..2d7839438abdc 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c
> @@ -1340,11 +1341,29 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
>  	rm->m_daddr = daddr;
>  
>  	/* rds_conn_create has a spinlock that runs with IRQ off.
> -	 * Caching the conn in the socket helps a lot. */
> -	if (rs->rs_conn && ipv6_addr_equal(&rs->rs_conn->c_faddr, &daddr) &&
> -	    rs->rs_tos == rs->rs_conn->c_tos) {
> -		conn = rs->rs_conn;
> +	 * Caching the conn in the socket helps a lot.
> +	 *
> +	 * The cached rs_conn holds a connection reference; take one of
> +	 * our own for the duration of this call (dropped on both exit
> +	 * paths), so that neither a concurrent sender replacing the
> +	 * cache nor rds_conn_destroy() can free the connection under
> +	 * us.  A cached connection whose destruction has begun is not
> +	 * reused: dropping it here lets the next sendmsg look up or
> +	 * create a live one instead of returning -EAGAIN forever.
> +	 */
> +	spin_lock_irqsave(&rs->rs_lock, flags);
> +	conn = rs->rs_conn;
> +	if (conn && ipv6_addr_equal(&conn->c_faddr, &daddr) &&
> +	    rs->rs_tos == conn->c_tos && !rds_destroy_pending(conn)) {
> +		rds_conn_get(conn);

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

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

	if (scope_id != rs->rs_bound_scope_id) {
		if (!scope_id) {
			scope_id = rs->rs_bound_scope_id;
		} else if (rs->rs_bound_scope_id) {
			release_sock(sk);
			ret = -EINVAL;
			...
		}
	}

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

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

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

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

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

Thanks for looking at these.

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

Re: [PATCH net-next v4 09/12] net/rds: refuse to queue on a connection being destroyed

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] rds_conn_path_quiesce() still asserts
  BUG_ON(!list_empty(&rm->m_sock_item)) for every message it takes off…
- [High] The quiesce purge unlinks m_conn_item and drops the connection-
  queue message reference without clearing RDS_MSG_ON_CONN and without…
- [High] The new rds_destroy_pending() refusal in rds_send_queue_rm() is
  placed inside the `if (rs->rs_snd_bytes < rds_sk_sndbuf(rs))` branch,…
- [Low] The comment "The code ordering is a little weird, but we're
  trying to minimize the time we hold c_lock" in rds_send_queue_rm() now…
- [Low] Series bisectability: this patch repairs a defect introduced
  earlier in the same series (the preceding commit "net/rds: hold…

--

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

This makes rds_conn_path_quiesce() splice cp_send_queue away under cp_lock
instead of walking it unlocked, and adds an rds_destroy_pending() test under
that same lock to rds_send_queue_rm() and rds_send_probe() so nothing is
queued after the purge.  rds_send_queue_rm() reports the refusal by setting
*queued negative and rds_sendmsg() turns that into -EAGAIN.

A few questions about the new purge and the placement of the refusal test.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 965d68e51a1c1..5699f45e4c377 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);

[Severity: High]
The commit message says "A sender that gets there first has its message
purged".  Can that message actually be purged, given the retained
BUG_ON(!list_empty(&rm->m_sock_item))?

rds_send_queue_rm() links a sendmsg message onto both queues in the same
locked region:

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

So a message on cp_send_queue always has a non-empty m_sock_item, and the
sender that "gets there first" hits BUG() rather than a clean purge.

The window exists because the flag the new test reads is published under a
different lock:

net/rds/connection.c:rds_conn_destroy() {
	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);
	...
}

CPU0 (sendmsg)                        CPU1 (destroy)
rds_send_queue_rm()
  spin_lock(&cp->cp_lock)
  rds_destroy_pending() == false
  list_add_tail(m_sock_item, ...)
  list_add_tail(m_conn_item, ...)
  spin_unlock(&cp->cp_lock)
                                      WRITE_ONCE(c_destroy_in_prog, true)
                                      rds_conn_path_quiesce()
                                        splice cp_send_queue
                                        BUG_ON(!list_empty(m_sock_item))

Is there also a non-racy path into the same assertion?  rds_conn_shutdown()
runs before the purge and rds_send_path_reset() puts retransmit messages
back on cp_send_queue:

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

Those messages are still on their socket's send queue, so destroying a
connection while a socket has unacked messages would reach the BUG_ON with
no timing involved at all.

[Severity: High]
The purge unlinks m_conn_item and drops the conn-side reference, but leaves
RDS_MSG_ON_CONN set and does not touch the socket side.  Can that let a
concurrent remover work on the private purge list?

RDS_MSG_ON_CONN is the token other conn-side removers use:

net/rds/send.c:rds_send_drop_to() {
	...
	spin_lock_irqsave(&cp->cp_lock, flags);
	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);
	spin_unlock_irqrestore(&cp->cp_lock, flags);
	...
	rds_message_put(rm);
}

rds_send_drop_to() is reachable from rds_release() and from
setsockopt(RDS_CANCEL_SENT_TO).  If it runs while the quiesce is walking
'purge' with cp_lock dropped, it unlinks an entry from that stack-local
list; if it runs after the purge, test_and_clear_bit() still succeeds and
rds_message_put() drops the conn-queue reference a second time.  Since
rds_send_queue_rm() takes one reference per membership, does that underflow
the rds_message refcount and free the message while rs_send_queue and the
sender still point at it?  rds_send_probe() messages, which have an empty
m_sock_item and survive the assertion above, look like the easiest case to
hit.

The established protocol clears the flag under the lock before releasing it:

net/rds/send.c:rds_send_path_drop_acked() {
	...
	list_move(&rm->m_conn_item, &list);
	clear_bit(RDS_MSG_ON_CONN, &rm->m_flags);
	...
	if (!list_empty(&list))
		smp_mb__after_atomic();
	spin_unlock_irqrestore(&cp->cp_lock, flags);
}

Should the splice clear RDS_MSG_ON_CONN for each message under cp_lock, and
retire the socket side (m_sock_item, rs_snd_bytes) through
rds_send_remove_from_sock()?  As written the purge never calls
rds_send_sndbuf_remove(), so rs_snd_bytes stays charged for the purged
messages.

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 2d7839438abdc..f7bc4c5446d61 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;

[Severity: High]
The refusal sits inside the "if (rs->rs_snd_bytes < rds_sk_sndbuf(rs))"
branch.  When the send buffer is already full, does *queued ever go
negative?

For a full sndbuf rds_send_queue_rm() skips the whole branch and returns the
unchanged *queued == 0, so rds_sendmsg() keeps sleeping:

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

With the default SO_SNDTIMEO, timeo is MAX_SCHEDULE_TIMEOUT, so the loop
re-sleeps and the new "if (queued < 0)" exit is never reached.

Can the send room ever appear afterwards?  Credit is returned by
rds_send_remove_from_sock() on ack processing, which cannot happen on a
quiesced connection, or by rds_send_drop_to() on close/cancel, which the
blocked task itself is holding the socket against.  The purge in
rds_conn_path_quiesce() touches neither rs_snd_bytes nor sk_sleep(), so
there is no wake-up either.

The commit message states:

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

Is that true for the full-sndbuf case, which is the only case that reaches
the wait loop?  And since rds_sendmsg() holds rds_conn_get(conn) for the
whole call, does a sender parked this way keep t_conn_count non-zero and
leave rds_conn_wait_conns_freed() looping on transport unload?  Would moving
the rds_destroy_pending() test above the sndbuf-room check, and returning
the credit plus a wake-up from the purge, address both?

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

[Severity: Low]
This isn't a bug, but the comment a few lines above now describes something
the function no longer does:

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

cp_lock is now taken at the top of the branch, so it is held across the
socket-queue insertion, sock_hold(), the header population, the
rds_message_addref() and the sequence bump.  Could the comment be updated or
dropped?

> @@ -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;
> @@ -1485,6 +1498,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()

[ ... ]

> @@ -1567,6 +1585,14 @@ rds_send_probe(struct rds_conn_path *cp, __be16 sport,
>  		goto out;
>  
>  	spin_lock_irqsave(&cp->cp_lock, flags);
> +	/* Same rule as rds_send_queue_rm(): once the destroy has purged
> +	 * cp_send_queue under this lock, nothing may be added behind it.
> +	 */
> +	if (rds_destroy_pending(cp->cp_conn)) {
> +		spin_unlock_irqrestore(&cp->cp_lock, flags);
> +		ret = -EAGAIN;
> +		goto out;
> +	}
>  	list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
>  	set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
>  	rds_message_addref(rm);

[Severity: Low]
One question about the series ordering rather than this hunk.  The
serialization added here is what makes the quiesce safe against senders that
are allowed to be in flight by the earlier commits "net/rds: split
connection destroy into quiesce and kref-governed free" and "net/rds: hold
connection references in lookup, sockets and c_passive".

At those intermediate commits the unlocked cp_send_queue walk in
rds_conn_path_quiesce() already races the locked add in
rds_send_queue_rm(), so a bisect or a partial backport lands on a tree with
the list corruption and pinned-connection behaviour described here.

Would folding this change into the prerequisite commit, or adding a Fixes:
tag naming it, keep the series bisectable?

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

Re: [PATCH net-next v4 10/12] net/rds: pin the connection across RDMA-CM event handling

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The commit message's central premise is false for this tree: it
  claims that rds_ib_cm_connect_complete()'s protocol-version-mismatch…
- [Low] Stale forward-reference comment: net/rds/ib_cm.c:936-940 (out:
  block of rds_ib_cm_handle_connect()) still says the conn "stays…

Pre-existing issues:
- [High] rds_rdma_cm_event_handler_cmn() declares `struct rds_transport
  *trans;` with no initializer and assigns it only for InfiniBand…

--

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

The RDMA-CM event handler takes a reference on the connection it picked
up from cm_id->context for the duration of the callback dispatch, and
ignores the event when the connection is already being freed.
rds_ib_cm_handle_connect() additionally re-checks rds_destroy_pending()
under c_cm_lock and rejects the incoming request in that case.

Two questions on the code and one on a quoted paragraph of the commit
message below.

> net/rds: pin the connection across RDMA-CM event handling
>
> 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.  Before this series that was already a use-after-free
> whenever a callback destroyed the connection, since rds_conn_destroy()
> freed it synchronously and the handler's mutex_unlock() ran on freed
> memory; the one such callback, rds_ib_cm_connect_complete() on a
> protocol version below 3.1, has meanwhile been switched to
> rds_conn_drop() by commit f97d8c7bab78 ("rds: ib: use rds_conn_drop()
> on protocol version mismatch"), which also removed the deadlock that
> destroy took on c_cm_lock.

[Severity: High]
Which tree does commit f97d8c7bab78 ("rds: ib: use rds_conn_drop() on
protocol version mismatch") come from?  I cannot find it in the baseline
or anywhere in this series, and rds_ib_cm_connect_complete() still calls
rds_conn_destroy() on the version-mismatch path, both at this commit and
at the end of the series:

net/rds/ib_cm.c: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;
		}
	}
	...
}

If that call site is still there, is the deadlock the paragraph says was
removed still reachable?  The handler dispatches this callback with
c_cm_lock held:

rds_rdma_cm_event_handler_cmn()
    mutex_lock(&conn->c_cm_lock);
    case RDMA_CM_EVENT_ESTABLISHED:
        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);  /* forces RDS_CONN_ERROR, queues cp_down_w */
                    flush_work(&cp->cp_down_w);
                        rds_shutdown_worker() -> rds_conn_shutdown()
                            mutex_lock(&cp->cp_cm_lock);   /* held by the flushing thread */

Since c_cm_lock is c_path[0].cp_cm_lock, does the flush_work() in
rds_conn_path_quiesce() wait for a shutdown worker that blocks on the
mutex the same thread is holding?  rds_conn_shutdown() cannot take the
RDS_CONN_DOWN -> RDS_CONN_DOWN shortcut here because
rds_conn_path_drop(cp, true) has just set the state to RDS_CONN_ERROR.

The version bytes come from the peer's CM private data, so a remote
endpoint advertising a version below 3.1 (and different from the compat
version) selects this branch.  Would that hang the RDMA-CM event thread
with c_cm_lock held, leave the connection unfreed, and make rds_ib
unload block in rds_conn_wait_conns_freed()?

If the referenced commit is a prerequisite that is not part of this
posting, could the ordering be stated, or could the destroy-from-callback
site be converted in this series?

> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 98f34b494237a..0199560488738 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");

[Severity: Low]
This isn't a bug, but the out: block a bit further down in the same
function still points at this change as future work:

net/rds/ib_cm.c:rds_ib_cm_handle_connect() {
	...
out:
	if (conn) {
		mutex_unlock(&conn->c_cm_lock);
		/* Drop the reference rds_conn_create() handed us.  The
		 * conn stays reachable through cm_id->context without a
		 * reference of its own for now; the CM event handler is
		 * given one of its own by a following patch.
		 */
		rds_conn_put(conn);
	}
	...
}

This commit is that following patch, and it does not give cm_id->context
a reference of its own - rds_rdma_cm_event_handler_cmn() takes a
per-event reference and drops it at out:.  Could that comment be updated
to describe the per-event pinning?  It is unchanged at the end of the
series.

> 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 - it reads the same way in the
baseline - but while we are in rds_rdma_cm_event_handler_cmn(): can trans
be used uninitialized here?  It is declared as

	struct rds_transport *trans;

with no initializer, assigned only for RDMA_NODE_IB_CA, and the
CONNECT_REQUEST arm dereferences it unconditionally:

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

The listener is created without a node-type restriction -
rds_rdma_listen_init_common() calls

	cm_id = rdma_create_id(&init_net, handler, NULL,
			       RDMA_PS_TCP, IB_QPT_RC);

and RDS never calls rdma_restrict_node_type(), so id_priv->
restricted_node_type stays RDMA_NODE_UNSPECIFIED and cma_listen_on_dev()
installs the listener on iWARP devices too.  An incoming iWARP request
then goes:

iw_conn_req_handler()          /* event.event = RDMA_CM_EVENT_CONNECT_REQUEST */
    cma_listen_handler()
        rds_rdma_cm_event_handler()
            rds_rdma_cm_event_handler_cmn()

with cm_id->device->node_type == RDMA_NODE_RNIC and cm_id->context NULL,
so the assignment above is skipped, conn stays NULL (the new
rds_conn_get_unless_zero() branch and the c_cm_lock block are both
skipped), and the indirect call goes through an uninitialized stack
value.  The other two arms happen to be guarded by conn.

Would an explicit else that rejects non-IB node types, or initialising
trans to NULL plus a check, be worth adding while this function is being
touched?

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

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

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

[netdev-bot+sashiko]

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The commit message's reachability claim — "reaching the use-
  after-free requires freeing a connection out from under a live socket,…
- [Low] The patch closes a reported, KASAN-confirmed use-after-free
  (`rds6_inc_info_copy()` reading a freed `rds_connection` via…

--

net/rds: hold a connection reference from struct rds_incoming

This makes every assignment of rds_incoming->i_conn take a connection
reference, dropped from rds_inc_put() and from the new rds_message_free()
helper, so a connection stays allocated while incs that arrived over it
are still queued on socket receive queues.

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

The two comments below are about the changelog only, so the diff hunks
are snipped.

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

[Severity: Medium]
Is this list of prerequisites complete?  There looks to be a fourth way
to free a live connection that needs none of the three, in
rds_ib_cm_connect_complete():

net/rds/ib_cm.c:rds_ib_cm_connect_complete() {
	dp = event->param.conn.private_data;
	...
		major = dp->ricp_v4.dp_protocol_major;
		minor = dp->ricp_v4.dp_protocol_minor;
	...
	if (major) {
		rds_ib_set_protocol(conn, RDS_PROTOCOL(major, minor));
	...
	if (conn->c_version < RDS_PROTOCOL_VERSION) {
		if (conn->c_version != RDS_PROTOCOL_COMPAT_VERSION) {
			pr_notice("RDS/IB: Connection <%pI6c,%pI6c> version %u.%u no longer supported\n", ...);
			rds_conn_destroy(conn);
			return;
		}
	}
}

The version comes from the peer's CM private data, and this callback is
reached from live event handling:

rds_rdma_cm_event_handler_cmn() -> RDMA_CM_EVENT_ESTABLISHED
  -> trans->cm_connect_complete()
    -> rds_ib_cm_connect_complete()
      -> rds_conn_destroy()

Since the same struct rds_connection is reused across reconnects for a
given laddr/faddr/tos tuple, can a reconnect on which the peer advertises
an unsupported version destroy a connection that unread incs from the
previous session still point at, without CAP_SYS_MODULE, netns teardown or
device removal?

If so, could the paragraph be reworded to cover that path, and the stable
reasoning re-derived from it?  The dependency on the reference counting
introduced earlier in the series is a separate argument.

One related note: the sibling patch "net/rds: pin the connection across
RDMA-CM event handling" states this call "has meanwhile been switched to
rds_conn_drop() by commit f97d8c7bab78", but at this revision
rds_ib_cm_connect_complete() still calls rds_conn_destroy(conn).  Which of
the two is right?

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

[Severity: Low]
Should this carry a Fixes: tag for the commit that introduced the bare
i_conn assignment in rds_inc_init()?

The changelog documents two user-visible crashes, the KASAN report in
rds6_inc_info_copy() and the panic in rds_ib_inc_free(), and the second
one is reachable on the pre-series code, where nothing keeps the
connection alive for a queued inc:

net/rds/ib_recv.c:rds_ib_inc_free() {
	struct rds_ib_connection *ic = inc->i_conn->c_transport_data;
	...
	rds_ib_recv_cache_put(&ibinc->ii_cache_entry, &ic->i_cache_incs);
}

reached by rds_release() -> rds_clear_recv_queue() -> rds_inc_put().

The other patches in this series that fix pre-existing defects do carry
one (e266df6b -> Fixes: 745cbccac3fe, 23531807 -> Fixes: 1c5113cf796b,
d4f3ede0 -> Fixes: ebeeb1ad9b8a, ef753cfe -> Fixes: cdc306a5c9cd3), so
this one looks inconsistent with the rest.  Even if no backport is
possible because the fix depends on the earlier reference counting, would
naming the introducing commit, or stating which kernels are exposed, help
downstream trees decide whether they are affected?

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