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

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

[Allison Henderson]

Hi all,

This is v7 of the connection-lifetime set (v1 at [1], v2 at [2],
v3 at [3], v4 at [6], v5 at [7], v6 at [8]), 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.

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.

  Patch 1 fixes rds_ib_conn_free() re-enabling interrupts under
  a caller's irqsave lock.

  Patch 2 makes the passive-connection exits of __rds_conn_create()
  undo conn_alloc() the same way the lost-race exit does, through one
  helper (a refactor: the passive twin only exists for IB, so nothing
  leaked there).  Both 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.
  One destroy caller does escape the old predicate: unloading the core
  module runs the loopback pernet exit for every live namespace, where
  check_net() is still true and the loop transport's unloading flag is
  not yet set.  The later patches also need the precise answer: the IB
  unload re-sweep hands a connection to rds_conn_destroy() more than
  once, and the passive-twin creation must refuse a parent whose
  destroy has begun.
  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.  IB marks a gathered node as claimed by the
  sweep, since its connect and shutdown paths move the node too.

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

  Patch 9 has the quiesce purge cp_send_queue under cp_lock, as every
  adder to that queue holds it, and clear RDS_MSG_ON_CONN as it does
  so, the way rds_send_drop_to() expects.  No sender can be in flight
  at any of today's destroy triggers (netns teardown, module unload),
  so this is hygiene rather than a race fix.

  Patch 10 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 11 pins the connection across the RDMA-CM event handler and,
  on both the passive and the active side, declines to set up a QP for
  a connection whose destroy has already begun.

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

Changes since v6 [8]:
 - Rebased onto net-next with ba3d1f480c7a ("net/rds: size a
   connection's path set by the transport it ends up with"), which
   closes the pre-existing per-path workqueue leak the v5 review found
   under patch 8, and 5ccac2330deb ("net/rds: replace tasklets with
   workqueue").
 - Patch order: the rds_incoming reference patch (v6 patch 12) moves
   ahead of the purge and the reference-holder patches, and the purge
   precedes the reference holders, so no bisect point has a queued
   message or a cached connection without the reference that protects
   it, nor a pinned connection over an unlocked purge.
 - Changelogs and comments no longer describe a destroy under a live
   socket as reachable: no trigger in this tree (netns teardown, module
   unload) runs while a socket is sending, and IB device removal drops
   rather than destroys.  Patches 7, 8, 10, 11 and 12 are reworded
   accordingly; patch 7 no longer cites the version-mismatch destroy.
 - Patch 4 names the destroy == true rds_conn_path_drop() among the
   arming sites the predicate does not cover.
 - Patch 5 states the first-caller-wins semantics of the destroy guard,
   in the changelog and the function comment, and the c_destroy_in_prog
   comment records that it is set under rds_conn_lock as a test-and-set.
 - Patch 10: the rds_tcp_accept_one() comment credits
   rds_tcp_kill_sock() for the clear-then-flush ordering; the
   c_destroy_in_prog comment covers the direct reads at the c_passive
   sites.
 - Patch 11: the pin's rationale says what it guards - the holders
   outside the handler - and that it is defensive today.
 - Patch 4 is a fix after all: the loopback pernet exit at core-module
   unload destroys connections with the old predicate false.  Changelog
   rewritten around that path, Fixes: c809195f5523; the c_destroy_in_prog
   comment names the two exempt arming sites.
 - Patch 5: stale version-mismatch example removed; the first-caller
   guard documented as inert until the re-sweep needs it.
 - Patch 6: the nodev-list BUG_ON in rds_ib_add_conn() goes here, not
   two patches later, so no bisect point empties that list under it;
   changelog no longer describes holders that arrive later.
 - Patch 7: rds_ib_conn_free() comment describes the claimed state.
 - Patch 8: a cached connection found with its destroy begun is dropped
   from the cache on the spot, instead of staying pinned until close;
   rs_lock comment names rs_conn and rs_tos.
 - Patch 9: the purge clears RDS_MSG_ON_CONN under cp_lock, so
   rds_send_drop_to() can neither double-put nor unlink from the purge
   list, and the BUG_ON on a socket-linked message goes; send.c is no
   longer touched (v6 had accidentally dropped the READ_ONCE() on
   rs_tos there).
 - Patch 10: rds_ib_cm_initiate_connect() gets the same
   rds_destroy_pending() check as the passive side, so a connect that
   completes after the destroy's quiesce cannot leave a QP and a device
   reference nothing tears down.

Changes since v5 [7]:
 - Rebased; the version-mismatch destroy is now an rds_conn_drop() in
   net-next (f97d8c7bab78), so patch 4's Fixes: tag is gone and its
   changelog describes what still needs the per-connection flag, and
   the CM-callback deadlock reports against earlier versions no longer
   apply.
 - Patch 2: described as the refactor it is (the passive twin is
   IB-only, single path); Fixes: tag dropped.
 - Patch 9: reduced to the cp_lock purge; the send-side refusals and
   the negative *queued signalling are dropped, since no sender can be
   in flight at a destroy trigger.
 - Patch 6: wait comment describes the initial-sweep-plus-resweep
   contract; changelog notes the guarantee is complete only once incs
   hold references.
 - Patch 8: c_destroy_in_prog read through rds_destroy_pending() in
   __rds_conn_create(); the c_passive puts documented as possibly the
   last; READ_ONCE() on the unlocked rs_tos sample; rs_conn comment
   notes the rds_release() exception.
 - Patch 10: stale forward-reference comment fixed.

Changes since v4 [6]:
 - Patch 7: the IB sweep marks each gathered node with a new
   i_ib_node_detached flag instead of relying on list emptiness.  A
   node parked on the sweep's stack list is not empty, so a concurrent
   rds_ib_add_conn() would have unlinked it from under the sweep's
   lockless walk; rds_ib_add_conn(), rds_ib_remove_conn() and
   rds_ib_conn_free() now leave a claimed node alone.

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/
[6] https://lore.kernel.org/netdev/20260917073958.174056-1-achender@kernel.org/
[7] https://lore.kernel.org/netdev/20260919061149.250658-1-achender@kernel.org/
[8] https://lore.kernel.org/netdev/20260922085410.391323-1-achender@kernel.org/

Allison


Allison Henderson (8):
  net/rds: ib: don't enable interrupts in rds_ib_conn_free()
  net/rds: undo conn_alloc() the same way on every __rds_conn_create()
    exit
  net/rds: guard every work-requeueing site with rds_destroy_pending()
  net/rds: make rds_destroy_pending() report a connection's own destroy
  net/rds: unlink transport nodes before a possibly deferred connection
    free
  net/rds: take cp_lock to purge cp_send_queue in the quiesce
  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     | 341 +++++++++++++++++++++++++++++++++------
 net/rds/ib.c             |  22 ++-
 net/rds/ib.h             |   4 +
 net/rds/ib_cm.c          |  49 +++++-
 net/rds/ib_rdma.c        |  67 ++++++--
 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            |  54 ++++++-
 net/rds/recv.c           |  26 ++-
 net/rds/send.c           |  80 +++++++--
 net/rds/tcp.c            |  53 +++++-
 net/rds/tcp_listen.c     |  21 ++-
 16 files changed, 735 insertions(+), 123 deletions(-)

-- 
2.25.1

[PATCH net-next v7 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 3ebe13d00953..74d384f0c323 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -1272,6 +1272,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);
 
@@ -1279,12 +1280,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 v7 02/12] net/rds: undo conn_alloc() the same way on every __rds_conn_create() exit

[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 frees only path 0.

That is not a leak today: a passive twin is only created for an IB
loopback connection (an incoming TCP connect to a local address is
refused with -EOPNOTSUPP before it gets here), and the IB transport is
not multipath, so npaths is 1 on that exit.  But the two exits express
the same "undo conn_alloc()" step in two different ways, and the
following patches add another exit of the same kind.  Move the loop
into a helper and use it everywhere, so that the step cannot silently
diverge if a multipath transport ever grows a passive twin.

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 c752a8623cfc..95ff50f31d1e 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
@@ -322,7 +338,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;
@@ -338,18 +354,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 v7 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 v7 04/12] net/rds: make rds_destroy_pending() report a connection's own 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, and the destroy == true
rds_conn_path_drop() that the destroy itself, and IB device removal,
issue and then flush - 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.

One caller escapes both terms.  When the core rds module unloads,
rds_conn_exit() runs rds_loop_net_exit() first, and unregistering the
pernet operations invokes rds_loop_exit_net() -> rds_loop_kill_conns()
-> rds_conn_destroy() for the loopback connections of every network
namespace that is still alive: check_net() is true for all of them,
and the loop transport's unloading flag is only set afterwards, by
rds_loop_exit().  For the duration of those destroys the predicate is
false, so 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, and the other requeueing sites can likewise re-arm works that
live in the about-to-be-freed connection.

The predicate is also imprecise even where it is true: it answers "is
this connection's world going away", not "is this connection being
destroyed", and the following patches need the second answer.  Once
the free is deferred to the last reference, a connection can be
handed to rds_conn_destroy() more than once (the IB unload path
re-sweeps its list until every connection is gone) and must
recognise its own destroy in progress, and the passive-twin creation
must refuse a parent whose destroy has begun.

The cp_flags bit that once served this purpose, RDS_DESTROY_PENDING
from commit c90ecbfaf50d2 ("rds: Use atomic flag to track connections
being destroyed"), was only ever set on the IB protocol-version path
and lost its last set_bit in commit cdc306a5c9cd3 ("rds: make v3.1 as
compat version"); it never covered the loopback path above, which
commit c809195f5523 ("rds: clean up loopback rds_connections on netns
deletion") added.

Record the destroy on the connection itself, where every caller is
covered: set
conn->c_destroy_in_prog before the unhash + synchronize_rcu() sequence
in rds_conn_destroy() and test it first 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: c809195f5523 ("rds: clean up loopback rds_connections on netns deletion")
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        | 16 ++++++++++++++--
 3 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index 95ff50f31d1e..cbc49426ba08 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -585,6 +585,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..d595fb78c61f 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,18 @@ 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.  Two kinds
+	 * of site are exempt: the workers' own self-requeues, which the
+	 * sync cancel in the destroy path rejects, and the destroy == true
+	 * rds_conn_path_drop(), which the destroy itself (and IB device
+	 * removal) issues and then flushes.
+	 */
+	bool			c_destroy_in_prog;
 	struct rds_connection	*c_passive;
 	struct rds_transport	*c_trans;
 
@@ -994,7 +1005,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 v7 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 the following patches deliberately let a
connection outlive the call.

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.

rds_conn_destroy() also gains a first-caller-wins guard on the new
c_destroy_in_prog flag, so that a connection handed to it a second
time - which the IB unload re-sweep added later in the series does -
is not quiesced twice: the later caller returns at once, without
quiescing anything and without waiting for the first caller to
finish, which is what the re-sweep wants.  With the initial reference
the only one, no second call can happen yet, so the guard is inert
here.

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 | 102 +++++++++++++++++++++++++++++++++++--------
 net/rds/rds.h        |  19 +++++++-
 2 files changed, 101 insertions(+), 20 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index cbc49426ba08..638e9f3140e2 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);
@@ -477,9 +478,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.
 			 */
@@ -526,10 +528,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;
 
@@ -558,6 +562,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);
@@ -567,16 +581,55 @@ 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.  Only the first call for a connection does any
+ * of that: a later one finds c_destroy_in_prog already set and
+ * returns at once, without waiting for the first.  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);
@@ -590,11 +643,26 @@ 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 -
+	 * the IB unload path re-sweeps its connection list until every
+	 * connection is gone.  Only the first caller proceeds; the
+	 * unhash also happens under rds_conn_lock, so a looked-up conn
+	 * can never be quiesced twice.  (With the initial reference the
+	 * only one, as it is at this point in the series, nothing can
+	 * reach a second call yet; this is the contract the following
+	 * patches rely on.)
 	 */
+	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();
@@ -602,7 +670,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));
 	}
 
@@ -613,12 +681,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 d595fb78c61f..01ac6f1507b1 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 */
@@ -147,8 +153,10 @@ 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
+	/* Set once, by rds_conn_destroy() under rds_conn_lock - a
+	 * test-and-set, so a second destroy of the same connection
+	 * returns at once - 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
@@ -830,6 +838,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 v7 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 - once the following patches hand out references to lookups,
sockets and incs - would execute transport module code after that
module's text is gone.  At this point in the series the initial
reference is the only one and the wait returns at once; the guarantee
becomes load-bearing with those patches.

rds_ib_add_conn() loses its assertion that the nodev list is
non-empty: with the sweep below emptying that list while it destroys
the entries, a connect still in flight would trip it.

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: the CM event handler's reference (added later in the series) 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.

At this point in the series the only reference holder is the
destroyer itself, so the count reaches zero as soon as the sweep has
run; the guarantee only becomes load-bearing once the following
patches hand references to sockets and incs.

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

diff --git a/net/rds/connection.c b/net/rds/connection.c
index 638e9f3140e2..83e59fcaccea 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;
 
@@ -347,6 +349,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 */
@@ -365,6 +368,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);
@@ -590,6 +594,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;
 
@@ -602,7 +607,54 @@ 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 an initial sweep has
+ * destroyed the transport's connections; @resweep, when given, is
+ * called on every poll to destroy connections that were still
+ * detaching from a device when the sweep ran (IB).  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..0c91f1b85c9b 100644
--- a/net/rds/ib_rdma.c
+++ b/net/rds/ib_rdma.c
@@ -125,7 +125,6 @@ void rds_ib_add_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *con
 
 	/* conn was previously on the nodev_conns_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);
 
@@ -165,7 +164,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 01ac6f1507b1..84c5f7650817 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -563,6 +563,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);
@@ -845,6 +851,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 v7 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; loopback uses list_del_init() and has its
conn_free() skip a node that is already empty.

IB needs an explicit flag, i_ib_node_detached, because its node has
other movers: a connect worker moves it from the nodev list to a
device's list in rds_ib_add_conn(), and a shutdown moves it back in
rds_ib_remove_conn().  Either can run while the sweep holds the node
on its stack list, and "the node is linked" cannot tell that list
from the nodev list - an add_conn() that went by list emptiness would
unlink the node from under the sweep's lockless walk.  So the sweep
sets the flag when it gathers the node, under ib_nodev_conns_lock,
and from then on add_conn(), remove_conn() and conn_free() leave the
node alone; the node belongs to the sweep, and its walk needs no
lock.  Those movers used to assert that the node is linked (and
rds_ib_add_conn() that the nodev list is non-empty); a connect or
shutdown worker can still be running for a connection the sweep has
claimed, and such a connection is about to be destroyed anyway, so
the assertions go.

The walk itself must not lose the entries either.  Once the following
patches hand out references, a connection on one of these lists can
be kept alive only by a reference that is dropped at any point during
the walk, freeing the transport node the iterator is about to read;
nothing in this tree destroys a connection behind the sweep's back
today, so this is preparation for those patches rather than a fix.
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.h      |  4 +++
 net/rds/ib_cm.c   | 13 +++++++---
 net/rds/ib_rdma.c | 66 +++++++++++++++++++++++++++++++++++++----------
 net/rds/loop.c    | 59 +++++++++++++++++++++++++++++++++---------
 net/rds/tcp.c     | 52 ++++++++++++++++++++++++++++++++-----
 5 files changed, 158 insertions(+), 36 deletions(-)

diff --git a/net/rds/ib.h b/net/rds/ib.h
index 1901226368c9..1efd4a9dfcdc 100644
--- a/net/rds/ib.h
+++ b/net/rds/ib.h
@@ -143,6 +143,10 @@ struct rds_ib_device;
 struct rds_ib_connection {
 
 	struct list_head	ib_node;
+	/* set under ib_nodev_conns_lock once a transport teardown has
+	 * claimed ib_node; from then on only the teardown touches it
+	 */
+	bool			i_ib_node_detached;
 	struct rds_ib_device	*rds_ibdev;
 	struct rds_connection	*conn;
 
diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 74d384f0c323..3123fd101f4b 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -1277,9 +1277,12 @@ void rds_ib_conn_free(void *arg)
 	rdsdebug("ic %p\n", ic);
 
 	/*
-	 * 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.
+	 * Conn is on a dev's list or on the nodev list - or, once a
+	 * transport teardown has claimed it (i_ib_node_detached), on
+	 * neither, in which case the lock chosen here only guards the
+	 * test below.  A connect or shutdown still running for a
+	 * claimed conn leaves the node alone, see rds_ib_add_conn() and
+	 * rds_ib_remove_conn().
 	 *
 	 * Callers may hold rds_conn_lock with interrupts disabled
 	 * (__rds_conn_create() undoing a lost creation race), so do not
@@ -1288,7 +1291,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);
+	/* a transport teardown that gathered us first owns the node */
+	if (!ic->i_ib_node_detached)
+		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 0c91f1b85c9b..71926f8ada41 100644
--- a/net/rds/ib_rdma.c
+++ b/net/rds/ib_rdma.c
@@ -123,14 +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 has claimed it ahead of destroying it: then it is on its
+	 * way out, and its node belongs to the sweep.
+	 */
 	spin_lock_irq(&ib_nodev_conns_lock);
-	BUG_ON(list_empty(&ic->ib_node));
-	list_del(&ic->ib_node);
+	if (!ic->i_ib_node_detached) {
+		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;
@@ -141,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 has
+	 * claimed it ahead of destroying it, in which case its node
+	 * belongs to the sweep
+	 */
 	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 = ic->i_ib_node_detached;
+	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);
 
@@ -162,13 +173,40 @@ 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 once the deferred frees
+	 * introduced later in the series can drop a connection's last
+	 * reference behind this sweep.  One whose free
+	 * is already running gets no reference: its free unlinks the
+	 * node itself, under this lock, once we drop it.  Marking the
+	 * node detached claims it for this sweep: rds_ib_add_conn(),
+	 * rds_ib_remove_conn() and rds_ib_conn_free() leave a claimed
+	 * node alone, so the walk over tmp_list below needs no lock.
+	 * 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)) {
+			ic->i_ib_node_detached = true;
+			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 would unlink
+	 * ib_node.  tmp_list lives on this stack frame, so take each node
+	 * off it before its destroy; the free then leaves it alone.
+	 */
+	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node) {
+		conn = ic->conn;
+		list_del_init(&ic->ib_node);
+
+		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 v7 08/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 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, 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 32c411d10e3e..7235974343dd 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -290,6 +290,8 @@ int rds_send_xmit(struct rds_conn_path *cp)
 			}
 			rm->data.op_active = 1;
 			rm->m_inc.i_conn_path = cp;
+			/* put in rds_message_put() */
+			rds_conn_get(cp->cp_conn);
 			rm->m_inc.i_conn = cp->cp_conn;
 
 			cp->cp_xmit_rm = rm;
@@ -947,6 +949,7 @@ static int rds_send_queue_rm(struct rds_sock *rs, struct rds_connection *conn,
 		/* The code ordering is a little weird, but we're
 		   trying to minimize the time we hold c_lock */
 		rds_message_populate_header(&rm->m_inc.i_hdr, sport, dport, 0);
+		rds_conn_get(conn);	/* put in rds_message_put() */
 		rm->m_inc.i_conn = conn;
 		rm->m_inc.i_conn_path = cp;
 		rds_message_addref(rm);
@@ -1527,6 +1530,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

[PATCH net-next v7 09/12] net/rds: take cp_lock to purge cp_send_queue in the quiesce

[Allison Henderson]

rds_conn_path_quiesce() empties cp_send_queue by walking it with no
lock held, while every path that adds to that queue -
rds_send_queue_rm(), rds_send_probe(), the retransmit requeue in
rds_send_path_reset() - does so under cp_lock.  The unlocked walk was
justified by the destroy running with nothing else alive: at every
destroy trigger there is - netns teardown and module unload - no
socket can still be sending on the connection, since a bound socket
pins its transport module and a namespace closes its sockets before
its RDS connections are torn down.

That argument still holds, but it is an argument about the callers,
not a property of the code.  Splice the queue away under cp_lock and
drop the message references outside it, so that the one walker of the
list follows the same lock discipline as its adders.  This does not
by itself make a sender that is still running safe - nothing here
tells such a sender that the queue is closed - and it does not need
to, since no such sender exists at any destroy trigger.

While at it, make the purge coherent with rds_send_drop_to(), the
other path that removes messages from a connection queue.  drop_to
decides whether it owns the queue's reference by test_and_clear on
RDS_MSG_ON_CONN; the purge left that bit set, so a message it had
already put could be put a second time by a close() or
RDS_CANCEL_SENT_TO racing it.  Clear the bit under cp_lock as part of
the splice.  With that, a message that is still on its socket's send
queue is no longer a fatal condition for the purge - the socket side
keeps its own reference and retires it on close - so the
BUG_ON(!list_empty(&rm->m_sock_item)) goes as well.

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

diff --git a/net/rds/connection.c b/net/rds/connection.c
index 83e59fcaccea..1d7932cac035 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -540,6 +540,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;
@@ -551,12 +553,24 @@ 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.  Every path that adds to
+	 * cp_send_queue does so under cp_lock; take it here too.  No
+	 * sender can still be running at any destroy trigger, so this is
+	 * lock discipline rather than a race fix: nothing here tells a
+	 * sender that the queue is closed.
+	 */
+	spin_lock_irqsave(&cp->cp_lock, flags);
+	list_splice_init(&cp->cp_send_queue, &purge);
+	/* Give up the queue's claim on each message while still under
+	 * the lock, so that rds_send_drop_to(), which decides ownership
+	 * of the connection-queue reference by this bit, neither drops
+	 * it a second time nor unlinks the message from our list.
+	 */
+	list_for_each_entry(rm, &purge, m_conn_item)
+		clear_bit(RDS_MSG_ON_CONN, &rm->m_flags);
+	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);
 	}
 	if (cp->cp_xmit_rm)
-- 
2.25.1

[PATCH net-next v7 10/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.
  No destroy trigger in this tree - netns teardown, module unload -
  can run while a socket is still sending, since a socket holds its
  netns and pins its transport module; what the reference buys today
  is the data race fix below and a cache that no longer sticks to a
  quiesced connection, and it is the discipline the later patches
  rely on.  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; the cache's own reference on it is dropped
  right there, so a socket that never sends again does not pin a
  quiesced connection until it is closed.  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 go through rds_tcp_kill_sock(), which clears the listen
socket the accept tests first and then flushes the accept work in
rds_tcp_listen_stop(), 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 data race it closes has no consequence beyond the report
while no destroy can run under a live socket, which is the case in
this tree.

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 | 127 +++++++++++++++++++++++++++++++++++++++++--
 net/rds/ib_cm.c      |   9 ++-
 net/rds/loop.c       |   2 +-
 net/rds/rds.h        |  16 ++++--
 net/rds/send.c       |  65 ++++++++++++++++++++--
 net/rds/tcp_listen.c |  12 +++-
 7 files changed, 232 insertions(+), 23 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 1d7932cac035..960fc5732c6e 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 && rds_destroy_pending(conn))
+			conn = NULL;
+		if (conn)
+			rds_conn_get(conn);
 	}
 	rcu_read_unlock();
 	if (conn)
@@ -340,13 +375,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 (rds_destroy_pending(parent)) {
+			/* 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 (rds_destroy_pending(passive)) {
+				/* 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);
@@ -365,6 +431,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++;
@@ -375,6 +445,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)
@@ -697,6 +769,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);
 
@@ -730,7 +805,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 */
@@ -747,6 +853,17 @@ 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.  Either may
+	 * be the last one - the twin's own destroy may already have run
+	 * - so these must stay here, in sleepable context with no lock
+	 * held, where the free that follows the last put is allowed.
+	 */
+	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 3123fd101f4b..ee018dd230e9 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 84c5f7650817..04e5852697b2 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -156,7 +156,10 @@ struct rds_connection {
 	/* Set once, by rds_conn_destroy() under rds_conn_lock - a
 	 * test-and-set, so a second destroy of the same connection
 	 * returns at once - before it cancels the path works.  Read
-	 * through rds_destroy_pending().  A site that arms
+	 * through rds_destroy_pending(), which also reports netns
+	 * teardown and module unload; the c_passive handling reads the
+	 * flag directly, since only this connection's own destroy
+	 * matters there.  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
@@ -167,7 +170,7 @@ struct rds_connection {
 	 * removal) issues and then flushes.
 	 */
 	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;
@@ -675,7 +678,10 @@ 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 - except by
+	 * rds_release(), which runs once no one else can reach the socket.
 	 */
 	struct rds_connection	*rs_conn;
 
@@ -684,7 +690,9 @@ struct rds_sock {
 	/* seen congestion (ENOBUFS) when sending? */
 	int			rs_seen_congestion;
 
-	/* rs_lock protects all these adjacent members before the newline */
+	/* rs_lock protects all these adjacent members before the newline,
+	 * as well as rs_conn and rs_tos above
+	 */
 	spinlock_t		rs_lock;
 	struct list_head	rs_send_queue;
 	u32			rs_snd_bytes;
diff --git a/net/rds/send.c b/net/rds/send.c
index 7235974343dd..bedcd8b836f6 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -1162,13 +1162,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) &&
@@ -1343,21 +1344,68 @@ 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 && rds_destroy_pending(conn)) {
+		/* drop the cache's reference right here, or the socket
+		 * would pin the quiesced connection until it is closed
+		 */
+		rs->rs_conn = NULL;
+		spin_unlock_irqrestore(&rs->rs_lock, flags);
+		rds_conn_put(conn);
+		conn = NULL;
 	} else {
+		if (conn && ipv6_addr_equal(&conn->c_faddr, &daddr) &&
+		    rs->rs_tos == conn->c_tos)
+			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,
+						rs->rs_transport,
+						READ_ONCE(rs->rs_tos),
 						sock->sk->sk_allocation,
 						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) {
@@ -1477,6 +1525,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:
@@ -1484,6 +1534,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..bb4f01c07169 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,13 @@ 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 go through rds_tcp_kill_sock() - which
+	 * clears the listen socket that the top of this function tests
+	 * and then flushes this work in rds_tcp_listen_stop() - 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 +355,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 v7 11/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, none of the
callbacks the handler dispatches drops a reference on the connection
it was handed: the version-mismatch path only drops the connection,
and rds_ib_cm_handle_connect() puts the reference rds_conn_create()
gave it, on a connection the listener's cm_id never pointed at.  What
can reach zero while an event is in flight are the holders outside
the handler - the destroy's initial reference, a socket's cache, a
parent's c_passive, an inc.  Today the shutdown pass destroys the
cm_id, and rdma_destroy_id() waits for a running handler, before the
initial reference is dropped, so every event is ordered ahead of the
free; the pin is defensive, keeping the handler correct without
leaning on that ordering.  Take a reference for the duration of the
handler, and ignore the event if the connection is already at zero
references rather than handle it.

rds_ib_cm_initiate_connect() has the same hole on the active side: a
connection whose destroy began while its address and route were
resolving reaches RDMA_CM_EVENT_ROUTE_RESOLVED and sets up a QP -
taking a device reference in rds_ib_add_conn() - after the destroy's
shutdown pass has run, or with that pass waiting on c_cm_lock behind
the handler.  Nothing would ever release the QP, the cm_id or the
device reference, and rds_ib_exit() would wait forever for the
device.  Return before the QP is set up when the destroy is pending;
the cm_id is still ic->i_cm_id, so the shutdown destroys it.

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          | 22 ++++++++++++++++++++--
 net/rds/rdma_transport.c | 19 ++++++++++++++++++-
 2 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index ee018dd230e9..948fbf4b6a85 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");
@@ -928,8 +935,8 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id,
 		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.
+		 * reference of its own; rds_rdma_cm_event_handler_cmn()
+		 * takes one for the duration of each event it handles.
 		 */
 		rds_conn_put(conn);
 	}
@@ -948,6 +955,17 @@ int rds_ib_cm_initiate_connect(struct rdma_cm_id *cm_id, bool isv6)
 	union rds_ib_conn_priv dp;
 	int ret;
 
+	/* A destroy that began while the address and route were being
+	 * resolved has already quiesced this conn, or is waiting on
+	 * c_cm_lock to do so.  Setting up a QP now would leave it - and
+	 * the device reference rds_ib_add_conn() takes - with no
+	 * shutdown pass left to tear them down.  The id we were handed
+	 * is still ic->i_cm_id, so return success and let that shutdown
+	 * destroy it, rather than have the rdma_cm destroy it on error.
+	 */
+	if (rds_destroy_pending(conn))
+		return 0;
+
 	/* If the peer doesn't do protocol negotiation, we must
 	 * default to RDSv3.0 */
 	rds_ib_set_protocol(conn, RDS_PROTOCOL_4_1);
diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
index b15cf316b23a..3dda7cf76ebb 100644
--- a/net/rds/rdma_transport.c
+++ b/net/rds/rdma_transport.c
@@ -63,6 +63,21 @@ 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, since the mutex
+	 * released at out: lives in the connection's path array.  None
+	 * of the callbacks below drops a reference on this connection,
+	 * and the shutdown destroys the cm_id - waiting for a running
+	 * handler - before the last reference can go, so this is
+	 * defensive.  A connection already at zero references 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 +186,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 v7 12/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 960fc5732c6e..c13b98df3476 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];
@@ -338,12 +337,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;
 	}
@@ -414,7 +414,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 {
@@ -437,7 +436,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);
 		}
 	}
@@ -681,7 +679,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++)
@@ -690,10 +687,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

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

[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 commit message says this patch fixes a live race, and it
  carries a Fixes: tag, but none of the five newly guarded sites can…

--

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

This patch wraps five work-arming sites in the rcu_read_lock() plus
rds_destroy_pending() pattern that the other arming sites already use.
The sites are the IB send completion and credit paths, rds_ib_recv_refill(),
rds_tcp_accept_one() and rds_sendmsg(). The goal is to stop new work
being queued on a path workqueue once rds_conn_destroy() has started
cancelling it.

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

[ ... ]

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

[Severity: Low]
At this commit, can any of the five newly guarded sites actually fire
while rds_conn_destroy() is running?

Here rds_destroy_pending() is still only:

net/rds/rds.h:rds_destroy_pending() {
	return !check_net(rds_conn_net(conn)) ||
	       (conn->c_trans->t_unloading && conn->c_trans->t_unloading(conn));
}

The teardown paths that reach rds_conn_destroy() seem to keep these
sites out of the window between the cancel and destroy_workqueue() in
rds_conn_path_destroy().

For IB, rds_conn_destroy() is only called from
rds_ib_exit()->rds_ib_destroy_nodev_conns(), and that function only takes
connections from ib_nodev_conns:

- rds_ib_setup_qp() takes the connection off that list with
  rds_ib_add_conn() before it calls rdma_create_qp().
- rds_ib_conn_path_shutdown() puts it back with rds_ib_remove_conn()
  only after disable_work_sync() on i_send_work/i_recv_work and after
  rdma_destroy_qp().

So wouldn't every IB connection that reaches rds_conn_destroy() have no
QP or CQ? That would leave rds_ib_send_cqe_handler(),
rds_ib_send_add_credits() and rds_ib_recv_refill() with nothing to
deliver.

For the TCP accept path, rds_tcp_kill_sock() clears
rtn->rds_tcp_listen_sock and flushes rds_tcp_accept_w in
rds_tcp_listen_stop() before it destroys any connection. After that,
rds_tcp_accept_one() returns early:

	if (!listen_sock) /* module unload or netns delete in progress */
		return -ENETUNREACH;

rds_tcp_exit() also calls unregister_pernet_device() before
rds_tcp_destroy_conns().

For rds_sendmsg(), the sending socket holds a reference on its netns.
rs_transport is also pinned with try_module_get() in
rds_trans_get_preferred(). So the new guard there looks like it always
evaluates to false.

The guards seem to take effect only with the follow-up patch, "net/rds:
make rds_destroy_pending() report a connection's own destroy", which adds
the c_destroy_in_prog term. That patch also says the last stragglers
"were converted by the previous patch".

There is one narrow interleaving where the IB guards could matter at
this commit. A CM handler could fetch rds_ibdev through
rds_ib_get_client_data() before rds_ib_remove_one() clears it. It could
then call rds_ib_add_conn() after the sweep's list_splice() and bring up
a QP while rds_conn_destroy() is running. That path already corrupts the
sweep's on-stack list, and the commit message does not describe it.

Would it be more accurate to describe this as preparation for the next
patch? The Fixes: tag could then be adjusted or dropped, or this patch
could be folded into the one that extends the predicate.

The Fixes: tag also doesn't seem to match the rds_sendmsg() hunk. The
cp_conn_w queue site there came from 9d27a0fb122f, not from
ebeeb1ad9b8a. The IB queue lines were rewritten to use cp_wq by
d327e2e74aedbe.

A stable backport chosen from this tag would do nothing on its own. The
rds_sendmsg() hunk would also lack context in older trees.

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

Re: [PATCH net-next v7 04/12] net/rds: make rds_destroy_pending() report a connection's own destroy

[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 only concrete justification for the
  `Fixes: c809195f5523` tag cannot happen.
- [Low] The new comment on struct rds_connection::c_destroy_in_prog
  (net/rds/rds.h) says the destroy == true rds_conn_path_drop() is…

--

net/rds: make rds_destroy_pending() report a connection's own destroy

This patch adds a c_destroy_in_prog flag to struct rds_connection.
rds_conn_destroy() sets it before the unhash and synchronize_rcu()
sequence, and rds_destroy_pending() now tests it first. The patch also
removes the unused RDS_DESTROY_PENDING cp_flags bit and its dead test in
rds_ib_is_unloading().

> One caller escapes both terms.  When the core rds module unloads,
> rds_conn_exit() runs rds_loop_net_exit() first, and unregistering the
> pernet operations invokes rds_loop_exit_net() -> rds_loop_kill_conns()
> -> rds_conn_destroy() for the loopback connections of every network
> namespace that is still alive: check_net() is true for all of them,
> and the loop transport's unloading flag is only set afterwards, by
> rds_loop_exit().  For the duration of those destroys the predicate is
> false, so 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, and the other requeueing sites can likewise re-arm works that
> live in the about-to-be-freed connection.

[ ... ]

> Fixes: c809195f5523 ("rds: clean up loopback rds_connections on netns deletion")

[Severity: Medium]
Can this race with rds_cong_queue_updates() actually happen during
rds_exit()?

rds_cong_queue_updates() has only two callers, rds_recv_rcvbuf_delta() and
rds_cong_remove_socket(). Both of them run on behalf of a live rds_sock:

  rds_recv_incoming() (after rds_find_bound()), rds_still_queued(),
  rds_clear_recv_queue() -> rds_recv_rcvbuf_delta()

  rds_release(), rds_cong_monitor() -> rds_cong_remove_socket()

Every rds_sock pins the rds module. rds_proto has:

net/rds/af_rds.c:rds_proto {
	.owner	  = THIS_MODULE,
}

and the socket allocation does:

net/core/sock.c:sk_prot_alloc() {
    ...
	if (!try_module_get(prot->owner))
    ...
}

That reference is only dropped in sk_prot_free(). Also, rds_tcp and
rds_rdma import rds symbols, so they have to be unloaded before rds.

So when rds_exit() -> rds_conn_exit() runs, no RDS sockets should be
left, and only loopback connections remain.

The paragraph also says "the other requeueing sites can likewise re-arm
works", but doesn't name any. There may be a narrower path, for example a
cp_conn_w still in flight from a sendmsg just before the last close:

  rds_connect_worker()
    rds_loop_conn_path_connect()
      rds_connect_path_complete()

This re-queues cp_send_w and cp_recv_w after rds_conn_path_destroy() has
already cancelled them.

Could the commit message cite a trigger that can actually happen, such as
this one, as the reason for the Fixes tag? Or could it describe the patch
as hardening and a prerequisite for the following patches? Stable
backporters will rely on the reason given here.

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

[ ... ]

> @@ -148,6 +147,18 @@ 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.  Two kinds
> +	 * of site are exempt: the workers' own self-requeues, which the
> +	 * sync cancel in the destroy path rejects, and the destroy == true
> +	 * rds_conn_path_drop(), which the destroy itself (and IB device
> +	 * removal) issues and then flushes.
> +	 */
> +	bool			c_destroy_in_prog;

[Severity: Low]
Is the IB device removal part of this comment accurate?

rds_ib_remove_one() calls rds_ib_dev_shutdown(), which does this for each
connection on rds_ibdev->conn_list:

net/rds/ib.c:rds_ib_dev_shutdown() {
    ...
	rds_conn_path_drop(&ic->conn->c_path[0], true);
    ...
}

With destroy == true, rds_conn_path_drop() skips the predicate and always
queues:

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

Neither rds_ib_dev_shutdown() nor rds_ib_remove_one() flushes cp_down_w
afterwards. The only flush_work(&cp->cp_down_w) is in
rds_conn_path_destroy().

Two other places have the same gap:

- The new comment above the WRITE_ONCE() in rds_conn_destroy() says the
  requeueing sites "all test rds_destroy_pending() under rcu_read_lock()".
- The commit message says the destroy itself "and IB device removal,
  issue and then flush".

The code looks safe today, but that comes from ordering. IB connections
are only destroyed by rds_ib_destroy_nodev_conns() from rds_ib_exit().
That runs after rds_ib_unregister_client() has finished every remove_one
callback. Any cp_down_w queued earlier is then flushed by
rds_conn_path_destroy().

Could the comments and commit message describe that ordering instead?
Later patches in the series build on this reasoning, and the rds.h comment
is unchanged at the end of the series.

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

Re: [PATCH net-next v7 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 comment added to rds_conn_destroy()
  (net/rds/connection.c:647-656) and this commit's message justify the…

--

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

This commit adds a kref to struct rds_connection. It splits
rds_conn_destroy() into a synchronous quiesce step and a deferred free in
rds_conn_destroy_fini(), which runs on the final rds_conn_put(). It also
adds a first-caller-wins guard on c_destroy_in_prog, so a second
rds_conn_destroy() of the same connection returns at once.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index cbc49426ba085..638e9f3140e28 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -590,11 +643,26 @@ 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 -
> +	 * the IB unload path re-sweeps its connection list until every
> +	 * connection is gone.  Only the first caller proceeds; the
> +	 * unhash also happens under rds_conn_lock, so a looked-up conn
> +	 * can never be quiesced twice.  (With the initial reference the
> +	 * only one, as it is at this point in the series, nothing can
> +	 * reach a second call yet; this is the contract the following
> +	 * patches rely on.)
>  	 */
> +	spin_lock_irq(&rds_conn_lock);
> +	if (conn->c_destroy_in_prog) {
> +		spin_unlock_irq(&rds_conn_lock);
> +		return;
> +	}

[Severity: Low]
Is this comment accurate? At this commit, rds_ib_exit() calls
rds_ib_destroy_nodev_conns() once, and that function makes a single pass
over its list:

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

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

So nothing re-sweeps the list and passes the same conn to
rds_conn_destroy() a second time.

Later in the series, rds_conn_wait_conns_freed() does call
rds_ib_destroy_nodev_conns() again on each poll. However, "net/rds: unlink
transport nodes before a possibly deferred connection free" makes the sweep
claim each node under ib_nodev_conns_lock before the destroy:

	ic->i_ib_node_detached = true;
	list_move_tail(&ic->ib_node, &tmp_list);

Nothing ever clears i_ib_node_detached. rds_ib_add_conn(),
rds_ib_remove_conn() and rds_ib_conn_free() all skip a detached node, so a
gathered conn can't show up on ib_nodev_conns again for a later sweep to
find. The TCP and loop sweeps also unlink and detach each node before
destroying it.

That seems to contradict "the transport state stays discoverable (e.g. on
the transports' connection lists) until the final rds_conn_put()". The
commit message makes the same claim:

  so that a connection handed to it a second time - which the IB unload
  re-sweep added later in the series does - is not quiesced twice

If no caller can reach a second rds_conn_destroy() on the same conn,
should the c_destroy_in_prog guard be dropped? If it stays, could this
comment, the rds_conn_destroy() header comment, the c_destroy_in_prog
comment in rds.h ("so a second destroy of the same connection returns at
once") and the commit message be reworded? As written, they describe a
double destroy that no path produces.

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

[ ... ]

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

Re: [PATCH net-next v7 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 3 potential
issue(s) to consider.

Critical: 0 · High: 2 · 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] At this commit rds_ib_add_conn() (net/rds/ib_rdma.c) loses its
  BUG_ON(list_empty(&ib_nodev_conns)).
- [High] rds_ib_exit() can hang forever in rds_conn_wait_conns_freed()
  when an IB connection attaches to a device's conn_list after…
- [Low] In rds_ib_exit() (net/rds/ib.c) this patch fixes two rmmod bugs
  that already exist, but the commit message calls the change…

--

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

This adds a per-transport t_conn_count. It is incremented when a
connection is published and decremented at the end of
rds_conn_destroy_fini(). The IB, TCP and loop exit paths now wait,
with no bound, for that count to reach zero. On IB, the wait resweeps
ib_nodev_conns on every poll, and rds_wq is flushed once afterwards.

> At this point in the series the initial
> reference is the only one and the wait returns at once; the guarantee
> becomes load-bearing with those patches.

[ ... ]

> At this point in the series the only reference holder is the
> destroyer itself, so the count reaches zero as soon as the sweep has
> run; the guarantee only becomes load-bearing once the following
> patches hand references to sockets and incs.

[ ... ]

> rds_ib_exit() has one more wrinkle.

[Severity: Low]
Is this accurate for IB?

rds_ib_remove_one()->rds_ib_dev_shutdown() only queues cp_down_w on
each connection's own ordered cp_wq:

    list_for_each_entry(ic, &rds_ibdev->conn_list, ib_node)
        rds_conn_path_drop(&ic->conn->c_path[0], true);

A connection moves to ib_nodev_conns only when that work runs
rds_ib_conn_path_shutdown()->rds_ib_remove_conn(). The
flush_workqueue(rds_wq) in rds_ib_unregister_client() does not wait for
cp_wq. So whenever a shutdown work is still pending, the new wait polls
and resweeps instead of returning at once.

That means the resweep fixes an existing rmmod bug. Before this patch,
rds_ib_exit() swept ib_nodev_conns only once. A connection that moved
there after that sweep was never destroyed, and it outlived the module
while still pointing at rds_ib_transport.

The same goes for the new flush_workqueue(rds_wq) in rds_ib_exit().
rds_ib_remove_conn()->rds_ib_dev_put() can drop the last device
reference after the flush in rds_ib_unregister_client(). That queues
rds_ib_dev_free() on rds_wq, and before this patch nothing flushed
rds_wq again before the module went away.

Could the commit message describe these two changes as fixes for
existing bugs, with a Fixes: tag, rather than as a wrinkle of the new
counter?

The message also doesn't mention the change from list_splice() to
list_splice_init() in rds_ib_destroy_nodev_conns(). With list_splice(),
the global ib_nodev_conns head was left pointing into the previous
call's stack-local tmp_list. That change is what makes repeated
resweeps safe. It is also why BUG_ON(list_empty(&ib_nodev_conns)) in
rds_ib_add_conn() can now trip.

This isn't a bug, but the two "At this point in the series" paragraphs
quoted above are nearly identical.

[ ... ]

> 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 loop forever if an IB connection attaches to a device's
conn_list after rds_ib_dev_shutdown() has already walked it?

rds_ib_remove_one() does:

	rds_ib_dev_shutdown(rds_ibdev);

	/* stop connection attempts from getting a reference to this device. */
	ib_set_client_data(device, &rds_ib_client, NULL);
	...
	synchronize_rcu();

An active connect's ROUTE_RESOLVED handler goes through
rds_ib_cm_initiate_connect()->rds_ib_setup_qp():

	rds_ibdev = rds_ib_get_client_data(dev);
	if (!rds_ibdev)
		return -EOPNOTSUPP;
	...
	rds_ib_add_conn(rds_ibdev, conn);

Suppose the handler gets rds_ibdev before ib_set_client_data(NULL), but
calls rds_ib_add_conn() after the rds_ib_dev_shutdown() walk. Then the
connection moves from ib_nodev_conns onto the removed device's
conn_list. The synchronize_rcu() doesn't order anything against the
attach, because the attach happens after the RCU section in
rds_ib_get_client_data().

After that, nothing seems to drop the connection:

  - rds_conn_path_drop(cp, false) returns early, because
    rds_destroy_pending() checks t_unloading, which is now true.

  - The resweep callback, rds_ib_destroy_nodev_conns(), only scans
    ib_nodev_conns.

So rds_ib_transport.t_conn_count never reaches zero. The only exit from
rds_conn_wait_conns_freed() is:

	while (!wait_event_timeout(rds_conn_freed_waitq,
				   !atomic_read(&trans->t_conn_count),
				   msecs_to_jiffies(RDS_CONN_FREE_POLL_MS))) {

rmmod would then block uninterruptibly, and the 100 ms polling keeps
the hung task detector quiet. Before this patch, the same race left a
live connection behind after unload. With this patch it becomes an
rmmod hang that never ends.

The commit message says a connection that has not migrated "would never
be destroyed, and would hold the count up for good", and the resweep is
meant to handle that. Doesn't that stay reachable for a connection that
never goes back to ib_nodev_conns?

Later in the series, "net/rds: pin the connection across RDMA-CM event
handling" adds an rds_destroy_pending() check at the start of
rds_ib_cm_initiate_connect(). That narrows the window but doesn't seem
to close it. A handler that passed the check before
rds_ib_set_unloading() and is then preempted around
rds_ib_get_client_data()/rds_ib_add_conn() still escapes. That commit's
own message notes this outcome ("rds_ib_exit() would wait forever for
the device").

[ ... ]

> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index db7e92e7bd29f..0c91f1b85c9b7 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c
> @@ -125,7 +125,6 @@ void rds_ib_add_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *con
>  
>  	/* conn was previously on the nodev_conns_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);

[Severity: High]
With this assertion gone, what stops an in-flight connect from moving a
node that is sitting on the sweeper's stack-local tmp_list?

rds_ib_destroy_nodev_conns() moves everything onto tmp_list, drops
ib_nodev_conns_lock, and then walks tmp_list without the lock:

	spin_lock_irq(&ib_nodev_conns_lock);
	list_splice_init(&ib_nodev_conns, &tmp_list);
	spin_unlock_irq(&ib_nodev_conns_lock);

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

Meanwhile, a ROUTE_RESOLVED handler may have got rds_ibdev from
rds_ib_get_client_data() before rds_ib_remove_one() cleared it. That
handler can still reach rds_ib_setup_qp()->rds_ib_add_conn(), which
does:

	list_del(&ic->ib_node);

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

Suppose that node is the one the sweeper has already cached in _ic.
The walk then follows ib_node.next to &rds_ibdev->conn_list, which is
not &tmp_list, so the loop doesn't stop. It then calls
rds_conn_destroy() on an ic->conn computed from memory inside struct
rds_ib_device.

The resweep callback in rds_ib_exit() repeats this unlocked walk on
every 100 ms poll, so the window opens again on each poll. A similar
list_del() from tmp_list was already possible before this patch, when
list_splice() was used.

The next patch in the series, "net/rds: unlink transport nodes before a
possibly deferred connection free", appears to fix this. It adds
i_ib_node_detached, which the sweep sets under ib_nodev_conns_lock, and
rds_ib_add_conn(), rds_ib_remove_conn() and rds_ib_conn_free() then
leave a claimed node alone. Could that be folded in here, so this
commit doesn't leave the hole open in between?

[ ... ]

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

Re: [PATCH net-next v7 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 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 rds_conn_destroy() comment in net/rds/connection.c gives a
  reason that this patch makes false, and the comment is still there,…
- [Low] The commit message does not describe the IB changes accurately.

--

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

This patch changes the RDS TCP, IB and loopback teardown helpers so each
gathered transport node comes off the on-stack list before
rds_conn_destroy() runs. A deferred conn_free() then cannot write into a
dead stack frame. The gather also takes a connection reference, and a new
i_ib_node_detached flag makes rds_ib_add_conn(), rds_ib_remove_conn() and
rds_ib_conn_free() leave a claimed IB node alone.

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

[Severity: Low]
This matches loop (loop_conns_lock) and TCP (rds_tcp_conn_lock). It does
not match IB: rds_ib_destroy_nodev_conns() takes the node off tmp_list
with no lock held:

	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node) {
		conn = ic->conn;
		list_del_init(&ic->ib_node);

Only the later IB paragraph says that the walk is lockless.

> Those movers used to assert that the node is linked (and
> rds_ib_add_conn() that the nodev list is non-empty); a connect or
> shutdown worker can still be running for a connection the sweep has
> claimed, and such a connection is about to be destroyed anyway, so
> the assertions go.

The nodev-list assertion in rds_ib_add_conn() was already removed by the
previous patch, "net/rds: wait for connections to be freed on transport
unload". That patch's message says:

  rds_ib_add_conn() loses its assertion that the nodev list is
  non-empty

This patch only drops the two BUG_ON(list_empty(&ic->ib_node)) checks in
rds_ib_add_conn() and rds_ib_remove_conn().

Could the commit message be updated to match what the IB changes do?

> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index 0c91f1b85c9b7..71926f8ada41a 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c

[ ... ]

> @@ -162,13 +173,40 @@ void rds_ib_destroy_nodev_conns(void)
>  	struct rds_ib_connection *ic, *_ic;
>  	LIST_HEAD(tmp_list);
>  

[ ... ]

>  	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)) {
> +			ic->i_ib_node_detached = true;
> +			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 would unlink
> +	 * ib_node.  tmp_list lives on this stack frame, so take each node
> +	 * off it before its destroy; the free then leaves it alone.
> +	 */
> +	list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node) {
> +		conn = ic->conn;
> +		list_del_init(&ic->ib_node);
> +
> +		rds_conn_destroy(conn);
> +		rds_conn_put(conn);
> +	}
>  }

[Severity: Low]
After this change, is the rds_conn_destroy() comment in
net/rds/connection.c still accurate? It says:

net/rds/connection.c:rds_conn_destroy() {
	 * 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 -
	 * the IB unload path re-sweeps its connection list until every
	 * connection is gone.  Only the first caller proceeds; the
	 * unhash also happens under rds_conn_lock, so a looked-up conn
	 * can never be quiesced twice.  (With the initial reference the
	 * only one, as it is at this point in the series, nothing can
	 * reach a second call yet; this is the contract the following
	 * patches rely on.)
}

In the sweep above, each gathered node gets i_ib_node_detached set under
ib_nodev_conns_lock and is list_del_init()'d before rds_conn_destroy().
rds_ib_add_conn() and rds_ib_remove_conn() never put a claimed node back
on ib_nodev_conns.

So the resweep that rds_ib_exit() passes to rds_conn_wait_conns_freed()
cannot find a connection it has already destroyed. That is the
double-destroy case the comment gives as the reason for the
c_destroy_in_prog test-and-set.

The other transports behave the same way.
rds_loop_destroy_gathered_conns() list_del_init()s the node and nothing
links it again. In rds_tcp_exit(), unregister_pernet_device() calls
rds_tcp_kill_sock() before the only rds_tcp_destroy_conns(), and nothing
sweeps after that.

Each sweep also holds its own extra reference when it calls
rds_conn_destroy(). So "the initial reference the only one" no longer
matches these callers, and "at this point in the series" means nothing
once the series is merged.

The comment is still unchanged at the end of the series. The guard itself
is harmless. Could the comment be updated to give the real reason for
keeping c_destroy_in_prog, or say that the IB re-sweep no longer reaches
it?

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

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

[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 commit fixes a use-after-free that users can hit and that was
  reported with a KASAN trace and a panic trace (Reported-by/Closes tags…

--

net/rds: hold a connection reference from struct rds_incoming

This commit makes every holder of rds_incoming->i_conn take a reference
on the connection. It covers rds_inc_init(), rds_inc_path_init(),
rds_recv_incoming(), rds_send_queue_rm(), rds_send_probe() and the
congestion map path of rds_send_xmit(). The references are dropped from
rds_inc_put() and from a new rds_message_free() helper, which fixes a
use-after-free of the connection seen from rds6_inc_info_copy() and from
rds_ib_inc_free() after the connection has been destroyed.

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

> 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, and the fix
> depends on the connection reference counting introduced earlier in this
> series.

[ ... ]

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

[Severity: Low]
This isn't a bug, but should the commit carry a Fixes: tag?

It fixes a use-after-free that has been reported, with a KASAN trace in
rds6_inc_info_copy() and a panic in rds_ib_recv_cache_put(), and it has
Reported-by and Closes trailers. The unreferenced i_conn pointer appears
to date back to the original RDS code.

Leaving the tag out looks deliberate. The quoted paragraph says the patch
depends on the connection reference counting added earlier in this series,
and a Fixes: tag could lead stable tooling to pick it up without those
prerequisites.

Would it help to add a Fixes: tag pointing at the commit that introduced
i_conn, maybe together with a note or a Cc: stable line that names the
prerequisite patches? That way the origin of the bug is recorded without
inviting a partial backport.

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

Re: [PATCH net-next v7 09/12] net/rds: take cp_lock to purge cp_send_queue in the quiesce

[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 commit message does not match what the code changes actually
  do.

--

net/rds: take cp_lock to purge cp_send_queue in the quiesce

This commit changes rds_conn_path_quiesce() so that cp_send_queue is
spliced onto a local list under cp_lock before the message references
are dropped. It also clears RDS_MSG_ON_CONN on each spliced message
while holding the lock, and removes the
BUG_ON(!list_empty(&rm->m_sock_item)) check from the purge loop.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 83e59fcaccea6..1d7932cac0358 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -551,12 +553,24 @@ 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.  Every path that adds to
> +	 * cp_send_queue does so under cp_lock; take it here too.  No
> +	 * sender can still be running at any destroy trigger, so this is
> +	 * lock discipline rather than a race fix: nothing here tells a
> +	 * sender that the queue is closed.
> +	 */
> +	spin_lock_irqsave(&cp->cp_lock, flags);
> +	list_splice_init(&cp->cp_send_queue, &purge);
> +	/* Give up the queue's claim on each message while still under
> +	 * the lock, so that rds_send_drop_to(), which decides ownership
> +	 * of the connection-queue reference by this bit, neither drops
> +	 * it a second time nor unlinks the message from our list.
> +	 */
> +	list_for_each_entry(rm, &purge, m_conn_item)
> +		clear_bit(RDS_MSG_ON_CONN, &rm->m_flags);
> +	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: Low]
The commit message says:

  drop_to decides whether it owns the queue's reference by test_and_clear
  on RDS_MSG_ON_CONN; the purge left that bit set, so a message it had
  already put could be put a second time by a close() or
  RDS_CANCEL_SENT_TO racing it.

Could that double put happen before this patch? Because of the BUG_ON,
the old loop only called rds_message_put() on a message whose
m_sock_item was empty.

rds_send_queue_rm() links m_sock_item before m_conn_item, and nothing
links m_sock_item again after it has been emptied:

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);
		rds_message_addref(rm);
    ...
		list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
		set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
    ...
}

rds_send_drop_to() can only reach a message through m_sock_item, either
on rs_send_queue or on its own local list. It empties m_sock_item only
in its second loop, and that loop runs after the first loop's
test_and_clear_bit():

net/rds/send.c:rds_send_drop_to() {
    ...
	list_for_each_entry(rm, &list, m_sock_item) {
		...
		if (!test_and_clear_bit(RDS_MSG_ON_CONN, &rm->m_flags)) {
		...
	}
    ...
	while (!list_empty(&list)) {
		rm = list_entry(list.next, struct rds_message, m_sock_item);
		list_del_init(&rm->m_sock_item);
    ...
}

So a message that a racing close() or RDS_CANCEL_SENT_TO could still
reach would have had a non-empty m_sock_item. The old BUG_ON would have
fired before the put happened.

It looks like the drop_to-after-purge double put only becomes possible
once the BUG_ON is removed. The clear_bit(RDS_MSG_ON_CONN) is what makes
that removal safe; it does not fix an existing double put.

The new code does keep the references balanced if a socket still has
the message queued. After the purge clears the bit and drops the
connection-queue reference, a later rds_send_drop_to() skips its first
loop and drops only the socket-queue reference.

The one pre-patch hazard runs the other way. The unlocked
list_for_each_entry_safe() could follow a stale next pointer to a
message that rds_send_drop_to() had already unlinked and put. Taking
cp_lock is what closes that, not the ON_CONN clear.

Could the commit message be reworded to describe what each change does?

Also, both the commit message and the new comment say no sender or
closer can be running at any destroy trigger. If so, the main visible
change at the current callers is that the BUG_ON is gone. It was the
only runtime check that no socket still had messages on cp_send_queue
at destroy. Is losing that check intended?

>  	if (cp->cp_xmit_rm)

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

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

[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] Two new comments in net/rds do not match the c_passive code in
  __rds_conn_create().
- [Low] The commit message lists "a cache that no longer sticks to a
  quiesced connection" as something the reference "buys today".
- [Low] The updated rs_lock comment in struct rds_sock
  (net/rds/rds.h:693-695) says rs_lock protects "rs_conn and rs_tos…
- [Low] The rds_conn_destroy() comment (connection.c:794-797) still says
  "With the initial reference the only one, as it is at this point in…
- [Low] rds_sendmsg() reads rs->rs_tos without rs_lock, as
  READ_ONCE(rs->rs_tos), for the rds_conn_create_outgoing() call…

Pre-existing issues:
- [Medium] This problem wasn't introduced by this patch, but the patch
  rewrites the affected code.

--

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

This patch makes rds_conn_lookup() and __rds_conn_create() return
referenced connections. It also makes the rs->rs_conn sendmsg cache and
parent->c_passive own references, and serializes rs_conn and the ToS
ioctls under rs_lock. c_passive becomes an RCU-annotated pointer, and the
rs_lock serialization addresses a syzbot KCSAN report on rs->rs_conn.

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

> No destroy trigger in this tree - netns teardown, module unload -
> can run while a socket is still sending, since a socket holds its
> netns and pins its transport module; what the reference buys today
> is the data race fix below and a cache that no longer sticks to a
> quiesced connection, and it is the discipline the later patches
> rely on.

[Severity: Low]

Does the patch really give "a cache that no longer sticks to a quiesced
connection" today? If no destroy trigger can run while the socket exists,
this new branch in rds_sendmsg() does not look reachable for user sockets
in this tree:

    if (conn && rds_destroy_pending(conn)) {
        ...
        rs->rs_conn = NULL;

rds_conn_destroy() is only called from three places:

    rds_ib_destroy_nodev_conns() (module exit)
    rds_tcp_kill_sock(), via rds_tcp_exit_net()
    the loop gather/destroy path

t_unloading is only set at module exit.

The cached conn's netns is sock_net(sk), which sk_alloc() pins. Its
c_trans is either the socket's pinned rs_transport or rds_loop_transport.

Could the commit message say that today the only benefit is the rs_lock
serialization that closes the KCSAN report?

> 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

[ ... ]

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

[Severity: Low]

rds_sendmsg() now reads rs_tos without rs_lock:

    conn = rds_conn_create_outgoing(sock_net(sock->sk),
                                    &rs->rs_bound_addr, &daddr,
                                    rs->rs_transport,
                                    READ_ONCE(rs->rs_tos),
                                    ...

Taking rs_lock here does not exclude that lockless reader. Should this
store be WRITE_ONCE(rs->rs_tos, tos) to pair with the READ_ONCE()?

The race needs a concurrent sendmsg (with rs_conn still NULL) and a
SIOCRDSSETTOS on the same socket. KCSAN with
CONFIG_KCSAN_ASSUME_PLAIN_WRITES_ATOMIC=n would report it. The c_tos
re-check under rs_lock stops a wrong ToS from being cached, so only the
annotation is missing.

> -		spin_unlock_bh(&rds_sock_lock);
> +		spin_unlock_irqrestore(&rs->rs_lock, flags);
>  		break;

[ ... ]

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 1d7932cac0358..960fc5732c6e6 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -697,6 +769,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);
>  

[Severity: Low]

This isn't a bug, but the existing comment further down in
rds_conn_destroy() still says:

    * can never be quiesced twice.  (With the initial reference the
    * only one, as it is at this point in the series, nothing can
    * reach a second call yet; this is the contract the following
    * patches rely on.)

After this patch, rds_conn_lookup() and __rds_conn_create() hand out
references, and rs->rs_conn and parent->c_passive each own one.
rds_incoming already held one.

Should the "initial reference the only one" wording be updated, since a
conn can now outlive its destroy while other holders keep it?

The "nothing can reach a second call yet" part still seems correct.
rds_ib_destroy_nodev_conns() marks swept nodes i_ib_node_detached and
unlinks them before it calls rds_conn_destroy().

[ ... ]

> diff --git a/net/rds/rds.h b/net/rds/rds.h
> index 84c5f76508177..04e5852697b2c 100644
> --- a/net/rds/rds.h
> +++ b/net/rds/rds.h
> @@ -156,7 +156,10 @@ struct rds_connection {
>  	/* Set once, by rds_conn_destroy() under rds_conn_lock - a
>  	 * test-and-set, so a second destroy of the same connection
>  	 * returns at once - before it cancels the path works.  Read
> -	 * through rds_destroy_pending().  A site that arms
> +	 * through rds_destroy_pending(), which also reports netns
> +	 * teardown and module unload; the c_passive handling reads the
> +	 * flag directly, since only this connection's own destroy
> +	 * matters there.  A site that arms

[Severity: Low]

Does the c_passive handling read the flag directly? All three c_passive
checks in __rds_conn_create() go through rds_destroy_pending():

    conn = rcu_dereference(parent->c_passive);
    if (conn && rds_destroy_pending(conn))
        conn = NULL;
    ...
    if (rds_destroy_pending(parent)) {
    ...
    if (rds_destroy_pending(passive)) {

rds_destroy_pending() also returns true for !check_net() and for
t_unloading, which this comment says do not matter there.

The new comment in rds_conn_destroy() makes the same claim: "refuses to
install a new one once c_destroy_in_prog is set, which it checks under
this lock".

There is a second mismatch. The comment on the locked branch in
__rds_conn_create() describes a state that does not seem possible:

    if (rds_destroy_pending(passive)) {
        /* Its destroy will clear the parent's
         * pointer under this lock shortly; until
         * then there is no usable passive conn.
         */

rds_conn_destroy() sets c_destroy_in_prog and clears parent->c_passive in
the same rds_conn_lock critical section. So, under that lock, a passive
still stored in parent->c_passive never has c_destroy_in_prog set.

Is rds_destroy_pending(passive) only ever true here because of netns
teardown or unload? If so, the rds_destroy_pending(parent) check just
above would already have caught those cases.

Could the comments or the checks be changed so the two agree?

[ ... ]

> @@ -684,7 +690,9 @@ struct rds_sock {
>  	/* seen congestion (ENOBUFS) when sending? */
>  	int			rs_seen_congestion;
>  
> -	/* rs_lock protects all these adjacent members before the newline */
> +	/* rs_lock protects all these adjacent members before the newline,
> +	 * as well as rs_conn and rs_tos above
> +	 */

[Severity: Low]

This isn't a bug, but rs_tos is not above rs_lock. It is the last member
of struct rds_sock, after rs_zcookie_queue.

Also, rds_sendmsg() reads rs_tos without rs_lock, via
READ_ONCE(rs->rs_tos), when it calls rds_conn_create_outgoing(). Could
the comment mention that exception?

[ ... ]

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 7235974343dda..bedcd8b836f66 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c

[ ... ]

> +	spin_lock_irqsave(&rs->rs_lock, flags);
> +	conn = rs->rs_conn;
> +	if (conn && rds_destroy_pending(conn)) {
> +		/* drop the cache's reference right here, or the socket
> +		 * would pin the quiesced connection until it is closed
> +		 */
> +		rs->rs_conn = NULL;
> +		spin_unlock_irqrestore(&rs->rs_lock, flags);
> +		rds_conn_put(conn);
> +		conn = NULL;
>  	} else {
> +		if (conn && ipv6_addr_equal(&conn->c_faddr, &daddr) &&
> +		    rs->rs_tos == conn->c_tos)
> +			rds_conn_get(conn);

[Severity: Medium]

This isn't a bug introduced by this patch, but the test is being
rewritten here anyway. Should the cache hit also compare conn->c_dev_if
against scope_id?

Take a socket bound to a non-link-local IPv6 address, so
rs_bound_scope_id == 0. The earlier scope check in rds_sendmsg() then
allows explicit sends with any nonzero sin6_scope_id:

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

A send to fe80::1%A creates and caches a conn with c_dev_if = A.

A later send to fe80::1%B matches here on c_faddr and c_tos alone and
reuses that conn. This skips rds_conn_lookup(), which does key on
c_dev_if.

The transport then uses conn->c_dev_if as the destination scope, for
example in rds_tcp_conn_path_connect():

    sin6.sin6_scope_id = conn->c_dev_if;

Could the message meant for the peer on link B then be delivered to a
different host at fe80::1 on link A?

Adding conn->c_dev_if == scope_id to this test should avoid that. The
baseline predicate had the same gap.

> +		else
> +			conn = NULL;
> +		spin_unlock_irqrestore(&rs->rs_lock, flags);
> +	}

[ ... ]

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

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

[netdev-bot+sashiko]

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

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

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 new rds_destroy_pending() check at the top of
  rds_ib_cm_initiate_connect() (ib_cm.c:966) does not close the…

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but it sits in the
  function the patch modifies and is still reachable at the end of the…

--

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

This commit makes rds_rdma_cm_event_handler_cmn() hold a reference on the
connection it takes from cm_id->context for as long as it handles each
RDMA-CM event. Events for connections that are already at zero references
are ignored. It also adds rds_destroy_pending() checks to
rds_ib_cm_initiate_connect() and rds_ib_cm_handle_connect(), so that no new
QP or cm_id is installed on a connection whose destroy has already started.

> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index ee018dd230e9d..948fbf4b6a852 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c

[ ... ]

> @@ -948,6 +955,17 @@ int rds_ib_cm_initiate_connect(struct rdma_cm_id *cm_id, bool isv6)
>  	union rds_ib_conn_priv dp;
>  	int ret;
>  
> +	/* A destroy that began while the address and route were being
> +	 * resolved has already quiesced this conn, or is waiting on
> +	 * c_cm_lock to do so.  Setting up a QP now would leave it - and
> +	 * the device reference rds_ib_add_conn() takes - with no
> +	 * shutdown pass left to tear them down.  The id we were handed
> +	 * is still ic->i_cm_id, so return success and let that shutdown
> +	 * destroy it, rather than have the rdma_cm destroy it on error.
> +	 */
> +	if (rds_destroy_pending(conn))
> +		return 0;

[Severity: High]

Does this check also cover transport unload? rds_destroy_pending() reports
unload through t_unloading.

Against the connection's own destroy the check works, because the shutdown
pass also takes c_cm_lock. The unload teardown in rds_ib_exit() never takes
c_cm_lock, though. Nothing checks for unload again when the conn is
attached to the device. This interleaving looks possible:

CPU1 (ROUTE_RESOLVED handler)
rds_ib_cm_initiate_connect()
  rds_destroy_pending(conn)   <- false, unloading not yet set
  rds_ib_setup_qp()
    rds_ib_get_client_data()  <- takes dev ref, drops rcu_read_lock()
    <preempted>

CPU2 (rmmod)
rds_ib_exit()
  rds_ib_set_unloading()
  synchronize_rcu()           <- does not wait for CPU1
  rds_ib_unregister_client()
    rds_ib_remove_one()
      rds_ib_dev_shutdown()   <- walks only rds_ibdev->conn_list, but the
                                 conn is still on ib_nodev_conns
      ib_set_client_data(device, &rds_ib_client, NULL)
      list_del_rcu(&rds_ibdev->list)

CPU1 resumes
    rds_ib_add_conn()         <- i_ib_node_detached is still false, so the
                                 conn moves to the removed device's
                                 conn_list
  rdma_connect_locked()       <- succeeds, the HCA is still present

rds_ib_destroy_nodev_conns() then scans only ib_nodev_conns, and so does
every resweep from rds_conn_wait_conns_freed(). This conn is never
destroyed.

Later DISCONNECTED or REJECTED events call rds_conn_drop(). That queues no
shutdown, because rds_destroy_pending() is true. No DEVICE_REMOVAL event
arrives either, since the IB device isn't going away.

Would t_conn_count then never reach zero? If so, rds_ib_exit() would loop
in rds_conn_wait_conns_freed() forever. The conn, QP, CQs and device
reference would all leak. That is the same hang the commit message says
this check prevents.

The passive side doesn't seem to have this problem during unload.
rds_rdma_listen_stop() runs first. rdma_destroy_id() on the listener then
waits on the handler_mutex, which is held while CONNECT_REQUEST is
delivered.

To close the active-side window, would rds_ib_add_conn() or
rds_ib_setup_qp() need an admission check that is synchronized with the
unload sweep? The early predicate check alone doesn't seem to be enough.

> +
>  	/* If the peer doesn't do protocol negotiation, we must
>  	 * default to RDSv3.0 */
>  	rds_ib_set_protocol(conn, RDS_PROTOCOL_4_1);
> diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
> index b15cf316b23a2..3dda7cf76ebb6 100644
> --- a/net/rds/rdma_transport.c
> +++ b/net/rds/rdma_transport.c
> @@ -63,6 +63,21 @@ 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, since the mutex
> +	 * released at out: lives in the connection's path array.  None
> +	 * of the callbacks below drops a reference on this connection,
> +	 * and the shutdown destroys the cm_id - waiting for a running
> +	 * handler - before the last reference can go, so this is
> +	 * defensive.  A connection already at zero references 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) {

[Severity: High]

This isn't a bug introduced by this patch, but it is in the function
being modified, and it is still reachable after this series. Further down,
the RDMA_CM_EVENT_ADDR_RESOLVED case passes the result of
rdma_resolve_route() back to the rdma_cm without filtering it:

net/rds/rdma_transport.c:rds_rdma_cm_event_handler_cmn() {
    ...
	case RDMA_CM_EVENT_ADDR_RESOLVED:
		if (conn) {
			...
			/* XXX do we need to clean up if this fails? */
			ret = rdma_resolve_route(cm_id,
						 RDS_RDMA_RESOLVE_TIMEOUT_MS);
		}
		break;
    ...
}

rdma_resolve_route() can fail synchronously. Some examples:

  - -ENOMEM from cma_resolve_ib_route()
  - an SA query setup failure
  - -ENODEV or -EINVAL from cma_resolve_iboe_route() on RoCE

When the handler returns non-zero, addr_handler() frees the id:

drivers/infiniband/core/cma.c:addr_handler() {
    ...
	if (cma_cm_event_handler(id_priv, &event)) {
		destroy_id_handler_unlock(id_priv);
		return;
	}
    ...
}

The same cm_id is still stored in ic->i_cm_id, and nothing clears it. The
connection stays in RDS_CONN_CONNECTING. Incoming connects from the peer
only hit the "Wait and see" branch in rds_ib_cm_handle_connect(), so the
pair cannot reconnect.

Later the connection is shut down, for example by rds_conn_destroy() on
module unload. rds_ib_conn_path_shutdown() then finds ic->i_cm_id
non-NULL and does the following on it:

  - calls rdma_disconnect(ic->i_cm_id)
  - reads ic->i_cm_id->qp
  - calls rdma_destroy_id(ic->i_cm_id)

Is that a use-after-free, followed by a second destroy of the cm_id?

The ROUTE_RESOLVED path already avoids this. rds_ib_cm_initiate_connect()
forces ret = 0 when ic->i_cm_id == cm_id, and the new early return in
this patch returns 0 for the same reason.

Should the ADDR_RESOLVED case get the same guard?

[ ... ]

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

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

[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 commit message says the wrong patch introduced t_conn_count.

--

net/rds: drop rds_conn_count in favor of t_conn_count

This commit removes the global rds_conn_count. The per-path workqueue
names in __rds_conn_create() now use the per-transport t_conn_count as
their seed. It also drops the rds_conn_lock round-trip that
rds_conn_destroy_fini() took only to decrement the old counter.

> The previous patch gave each transport its own connection count in
> t_conn_count, incremented and decremented at exactly the points where
> the global rds_conn_count is.

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

The patch right before this one is "net/rds: pin the connection across
RDMA-CM event handling". It does not touch net/rds/connection.c or
net/rds/rds.h, and it has nothing to do with t_conn_count.

t_conn_count was added earlier in the series by "net/rds: wait for
connections to be freed on transport unload". That commit's notes say
"t_conn_count did not exist upstream and is introduced here". It also
cites the same UEK commit, "net/rds: Each RDS transport should keep its
own connection count".

Could this sentence name that commit by its subject instead? For
example:

  Commit "net/rds: wait for connections to be freed on transport
  unload" gave each transport its own connection count in
  t_conn_count, ...

That way the reference still holds if the series is reordered or
split when it is applied.

The rest of the message matches the code. In the parent tree,
rds_conn_count's only reader is the alloc_ordered_workqueue() name seed
in __rds_conn_create(). Its increments and decrement sit right next to
the t_conn_count ones.

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

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

[Jakub Kicinski]

On Sat, 26 Sep 2026 23:14:36 -0700 Allison Henderson wrote:
> This is v7 of the connection-lifetime set (v1 at [1], v2 at [2],
> v3 at [3], v4 at [6], v5 at [7], v6 at [8]), 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.

More changes coming / planned or is this good enough?

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

[Allison Henderson]

On Fri, 2026-10-02 at 12:37 -0700, Jakub Kicinski wrote:
> On Sat, 26 Sep 2026 23:14:36 -0700 Allison Henderson wrote:
> > This is v7 of the connection-lifetime set (v1 at [1], v2 at [2],
> > v3 at [3], v4 at [6], v5 at [7], v6 at [8]), 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.
> 
> More changes coming / planned or is this good enough?

I'll send another v8 that addresses the review complaints. I've just noticed that the sashiko reviews come in two sweeps
now, so I wanted to catch both before spinning another version.

Thanks!
Allison