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

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

[Allison Henderson]

Hi all,

This set is the next stage of the RDS stabilization work, following
"net/rds: own the fastpath locks across connection teardown", now
net-next.  This series is targeted to net-next as well. Though the 
series fixes real use-after-frees (one syzbot report and one report
from Chengfeng Ye below), it does so by reworking connection lifetime,
which is a substantial change.

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 not only on rmmod but also
on IB device removal and on protocol-version mismatch, 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.  Each of those is a use-after-free waiting for its trigger, and
no single Fixes: commit covers the rot, so the series carries
Reported-by tags where there are concrete reports instead.

  Patch 1 gives the connection itself a destroy-in-progress marker so
  that rds_destroy_pending() covers single-connection destroy, not
  just the netns-teardown and module-unload cases.  Without it, the
  work-requeueing sites can re-arm works on a connection whose
  workqueues are about to be destroyed.
  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 2 splits rds_conn_destroy() into a quiesce phase and a
  kref-governed free, so a connection with references still
  outstanding stays allocated (quiesced, unhashed and unusable) until
  the last reference is dropped.
  Based on UEK commits:
     2c8569e4c880 ("net/rds: Add krefs to struct rds_connection").
     https://github.com/oracle/linux-uek/commit/2c8569e4c880

  Patch 3 hands out real references everywhere a connection pointer
  previously escaped bare: rds_conn_lookup(), __rds_conn_create()'s
  return, the rs_conn sendmsg cache, and the parent's c_passive
  pointer.
  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 4 makes each transport's exit path wait for its own
  connections to actually be freed before the module text goes away,
  since the free - including the transport's conn_free - is now
  asynchronous.
  Based on UEK commits:
     ece4b4e39afa ("net/rds: wait_event_timeout until zero connections during rmmod")
     https://github.com/oracle/linux-uek/commit/ece4b4e39afa

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

  Patch 5 is the cleanup fallout of patch 4: the global rds_conn_count
  has no remaining consumer besides a workqueue-name seed, so switch
  the seed to the per-transport count and remove it.
  Based on UEK commits:
     905ec90e6166 ("net/rds: Each RDS transport should keep its own connection count").
     https://github.com/oracle/linux-uek/commit/905ec90e6166

  Patch 6 makes struct rds_incoming hold a reference on i_conn, which
  is the fix for the KASAN use-after-free Chengfeng Ye reported [1],
  where rds_info_getsockopt() walks a socket receive queue whose incs
  point at connections rmmod already freed.
  Based on UEK commits:
      99b9a3715419 ("net/rds: fix crash by expanding kref coverage to rds_incoming.i_conn").
      https://github.com/oracle/linux-uek/commit/99b9a3715419

Patches 2, 3, 4 and 6 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 both
loopback-TCP and RXE-RDMA transports, plus targeted churn tests that
delete network namespaces and unload the modules under live rds-stress
traffic - the paths this series changes.

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

Allison


Allison Henderson (2):
  net/rds: make rds_destroy_pending() cover single-connection destroy
  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: hold connection references in lookup, sockets and c_passive
  net/rds: wait for connections to be freed on transport unload
  net/rds: hold a connection reference from struct rds_incoming

 net/rds/af_rds.c     |   8 ++
 net/rds/connection.c | 189 ++++++++++++++++++++++++++++++++++++++-----
 net/rds/ib.c         |  14 +++-
 net/rds/ib_cm.c      |   8 +-
 net/rds/loop.c       |   2 +
 net/rds/message.c    |  16 +++-
 net/rds/rds.h        |  25 +++++-
 net/rds/recv.c       |  21 ++++-
 net/rds/send.c       |  45 +++++++++--
 net/rds/tcp.c        |   1 +
 net/rds/tcp_listen.c |   5 +-
 11 files changed, 297 insertions(+), 37 deletions(-)

-- 
2.25.1

[PATCH net-next 1/6] net/rds: make rds_destroy_pending() cover single-connection destroy

[Allison Henderson]

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

rds_conn_destroy() is not limited to the global paths: rds_ib
destroys connections whose underlying IB device was removed
(rds_ib_destroy_nodev_conns()) and connections whose peer negotiated
an unsupported protocol version (rds_ib_cm_connect_complete()).
While one of those runs, a concurrent rds_cong_queue_updates() can
still find the connection on the congestion map's m_conn_list (the
conn is only removed from it after the paths are torn down) and call
queue_delayed_work() on a cp_wq that destroy_workqueue() has already
freed.  Additionally, the other requeueing sites can likewise re-arm
works that live in the about-to-be-freed connection unless
rds_destroy_pending() has something to guard it with.

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

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

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

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

diff --git a/net/rds/connection.c b/net/rds/connection.c
index b6c4beb50eaf..50e1b6bfceea 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -574,6 +574,14 @@ void rds_conn_destroy(struct rds_connection *conn)
 		 "%pI4\n", conn, &conn->c_laddr,
 		 &conn->c_faddr);
 
+	/* Make rds_destroy_pending() true for this conn.  Together with
+	 * the synchronize_rcu() below this stops the work-requeueing
+	 * sites (which all test rds_destroy_pending() under
+	 * rcu_read_lock()) from queueing new work on the path
+	 * workqueues once we start cancelling and destroying them.
+	 */
+	WRITE_ONCE(conn->c_destroy_in_prog, true);
+
 	/* Ensure conn will not be scheduled for reconnect */
 	spin_lock_irq(&rds_conn_lock);
 	hlist_del_init_rcu(&conn->c_hash_node);
diff --git a/net/rds/ib.c b/net/rds/ib.c
index 786f39169bc1..9fe3b9951bd3 100644
--- a/net/rds/ib.c
+++ b/net/rds/ib.c
@@ -525,10 +525,7 @@ static void rds_ib_set_unloading(void)
 
 static bool rds_ib_is_unloading(struct rds_connection *conn)
 {
-	struct rds_conn_path *cp = &conn->c_path[0];
-
-	return (test_bit(RDS_DESTROY_PENDING, &cp->cp_flags) ||
-		atomic_read(&rds_ib_unloading) != 0);
+	return atomic_read(&rds_ib_unloading) != 0;
 }
 
 void rds_ib_exit(void)
diff --git a/net/rds/rds.h b/net/rds/rds.h
index 2db49573dacd..cede2b03baa5 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,10 @@ struct rds_connection {
 				c_pad_to_32:29;
 	int			c_npaths;
 	bool			c_with_sport_idx;
+	/* Set (under RCU) when rds_conn_destroy() starts on this conn;
+	 * read through rds_destroy_pending().
+	 */
+	bool			c_destroy_in_prog;
 	struct rds_connection	*c_passive;
 	struct rds_transport	*c_trans;
 
@@ -994,7 +997,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 2/6] 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 can also be destroyed
when the underlying IB device is removed (in
rds_ib_destroy_nodev_conns()), or when a peer negotiates an
unsupported protocol version.

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

Prepare to close those holes by making the connection refcounted:

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

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

Based on Oracle UEK commit 2c8569e4c880 ("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 | 89 ++++++++++++++++++++++++++++++++++++--------
 net/rds/rds.h        |  8 ++++
 2 files changed, 82 insertions(+), 15 deletions(-)

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

[PATCH net-next 3/6] 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(), skipping entries whose free has already
  begun and that an RCU traversal may still encounter), and
  __rds_conn_create() returns the connection with a reference held
  for the caller on every path: lookup hit, fresh creation, lost
  creation race, and the passive-loopback lookup, which now also
  holds the parent while it dereferences parent->c_passive.

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

- parent->c_passive owns a reference, dropped when the parent is
  destroyed.

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

The cm_id->context back-pointers deliberately remain reference-free:
connection destroy tears down the cm_id before the connection can be
freed, so a CM callback can never see a stale context.

Based on Oracle UEK commits 2c8569e4c880 ("net/rds: Add krefs to
struct rds_connection") and 0e9e3a72b7f7 ("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     |  8 ++++++
 net/rds/connection.c | 62 ++++++++++++++++++++++++++++++++++++++++++--
 net/rds/ib_cm.c      |  8 +++++-
 net/rds/send.c       | 42 ++++++++++++++++++++++++++----
 net/rds/tcp_listen.c |  5 +++-
 5 files changed, 116 insertions(+), 9 deletions(-)

diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
index d5defe9172e3..0eaa5b976572 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);
diff --git a/net/rds/connection.c b/net/rds/connection.c
index f45fd1fb1843..df26959b0fdc 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -79,7 +79,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,
@@ -96,6 +99,13 @@ 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) {
+			/* An entry whose refcount already dropped to
+			 * zero has been unhashed and is about to be
+			 * freed; an RCU traversal may still come
+			 * across it.  Treat it as absent.
+			 */
+			if (!kref_get_unless_zero(&conn->c_refcount))
+				continue;
 			ret = conn;
 			break;
 		}
@@ -197,7 +207,14 @@ 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;
+		/* 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.
+		 */
 		conn = parent->c_passive;
+		if (conn)
+			rds_conn_get(conn);
 	}
 	rcu_read_unlock();
 	if (conn)
@@ -316,12 +333,32 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 	spin_lock_irqsave(&rds_conn_lock, flags);
 	if (parent) {
 		/* Creating passive conn */
-		if (parent->c_passive) {
+		if (READ_ONCE(parent->c_destroy_in_prog)) {
+			/* The parent's destroy has begun (it sets the
+			 * flag and snatches c_passive under this
+			 * lock); do not install a new passive conn
+			 * that nothing would ever destroy.
+			 */
+			trans->conn_free(conn->c_path[0].cp_transport_data);
+			free_cp = conn->c_path;
+			kmem_cache_free(rds_conn_slab, conn);
+			conn = ERR_PTR(-ENETDOWN);
+		} else if (parent->c_passive) {
+			rds_conn_get(parent->c_passive);
 			trans->conn_free(conn->c_path[0].cp_transport_data);
 			free_cp = conn->c_path;
 			kmem_cache_free(rds_conn_slab, conn);
 			conn = parent->c_passive;
 		} else {
+			/* 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 */
 			parent->c_passive = conn;
 			rds_cong_add_conn(conn);
 			rds_conn_count++;
@@ -351,6 +388,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++;
@@ -360,6 +401,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)
@@ -616,6 +659,7 @@ EXPORT_SYMBOL_GPL(rds_conn_put);
 void rds_conn_destroy(struct rds_connection *conn)
 {
 	int i;
+	struct rds_connection *passive;
 	struct rds_conn_path *cp;
 	int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
 
@@ -646,6 +690,16 @@ 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 = conn->c_passive;
+	conn->c_passive = NULL;
 	spin_unlock_irq(&rds_conn_lock);
 	synchronize_rcu();
 
@@ -663,6 +717,10 @@ void rds_conn_destroy(struct rds_connection *conn)
 	 */
 	rds_cong_remove_conn(conn);
 
+	/* drop the reference our c_passive pointer held, if any */
+	if (passive)
+		rds_conn_put(passive);
+
 	/* 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 4feb0edc360c..24e538c253a7 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -924,8 +924,14 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id,
 		rds_ib_conn_error(conn, "rdma_accept failed\n");
 
 out:
-	if (conn)
+	if (conn) {
 		mutex_unlock(&conn->c_cm_lock);
+		/* The conn stays reachable through cm_id->context
+		 * without a reference of its own: connection destroy
+		 * shuts the cm_id down before the conn is freed.
+		 */
+		rds_conn_put(conn);
+	}
 	if (err)
 		rdma_reject(cm_id, &err, sizeof(int),
 			    IB_CM_REJ_CONSUMER_DEFINED);
diff --git a/net/rds/send.c b/net/rds/send.c
index 1afa981e5c06..036a68372e2f 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -1159,13 +1159,14 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
 	DECLARE_SOCKADDR(struct sockaddr_in *, usin, msg->msg_name);
 	__be16 dport;
 	struct rds_message *rm = NULL;
-	struct rds_connection *conn;
+	struct rds_connection *conn = NULL;
 	int ret = 0;
 	int queued = 0, allocated_mr = 0;
 	int nonblock = msg->msg_flags & MSG_DONTWAIT;
 	long timeo = sock_sndtimeo(sk, nonblock);
 	struct rds_conn_path *cpath;
 	struct in6_addr daddr;
+	unsigned long flags;
 	__u32 scope_id = 0;
 	size_t rdma_payload_len = 0;
 	bool zcopy = ((msg->msg_flags & MSG_ZEROCOPY) &&
@@ -1340,11 +1341,29 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
 	rm->m_daddr = daddr;
 
 	/* rds_conn_create has a spinlock that runs with IRQ off.
-	 * Caching the conn in the socket helps a lot. */
-	if (rs->rs_conn && ipv6_addr_equal(&rs->rs_conn->c_faddr, &daddr) &&
-	    rs->rs_tos == rs->rs_conn->c_tos) {
-		conn = rs->rs_conn;
+	 * Caching the conn in the socket helps a lot.
+	 *
+	 * The cached rs_conn holds a connection reference; take one of
+	 * our own for the duration of this call (dropped on both exit
+	 * paths), so that neither a concurrent sender replacing the
+	 * cache nor rds_conn_destroy() can free the connection under
+	 * us.  A cached connection whose destruction has begun is not
+	 * reused: dropping it here lets the next sendmsg look up or
+	 * create a live one instead of returning -EAGAIN forever.
+	 */
+	spin_lock_irqsave(&rs->rs_lock, flags);
+	conn = rs->rs_conn;
+	if (conn && ipv6_addr_equal(&conn->c_faddr, &daddr) &&
+	    rs->rs_tos == conn->c_tos && !rds_destroy_pending(conn)) {
+		rds_conn_get(conn);
 	} else {
+		conn = NULL;
+	}
+	spin_unlock_irqrestore(&rs->rs_lock, flags);
+
+	if (!conn) {
+		struct rds_connection *old;
+
 		conn = rds_conn_create_outgoing(sock_net(sock->sk),
 						&rs->rs_bound_addr, &daddr,
 						rs->rs_transport, rs->rs_tos,
@@ -1352,9 +1371,17 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
 						scope_id);
 		if (IS_ERR(conn)) {
 			ret = PTR_ERR(conn);
+			conn = NULL;
 			goto out;
 		}
+		/* hand the cache its own reference */
+		rds_conn_get(conn);
+		spin_lock_irqsave(&rs->rs_lock, flags);
+		old = rs->rs_conn;
 		rs->rs_conn = conn;
+		spin_unlock_irqrestore(&rs->rs_lock, flags);
+		if (old)
+			rds_conn_put(old);
 	}
 
 	if (conn->c_trans->t_mp_capable) {
@@ -1469,6 +1496,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:
@@ -1476,6 +1505,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 13fa60c1985b..0d2ced892a8a 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.
@@ -343,6 +344,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 4/6] net/rds: wait for connections to be freed on transport unload

[Allison Henderson]

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

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

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

Sockets cannot keep the count elevated here: a bound socket holds a
module reference on its transport (rds_trans_get_preferred()), so a
transport cannot reach its exit path while any socket that could
cache one of its connections in rs_conn still exists.  The remaining
holders are short-lived, hence the bounded wait; if it expires
anyway, warn - the pending frees will touch freed module text.

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 Oracle UEK commits ece4b4e39afa ("net/rds: wait_event_timeout
until zero connections during rmmod") and 905ec90e6166 ("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()); 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 | 29 +++++++++++++++++++++++++++++
 net/rds/ib.c         |  9 +++++++++
 net/rds/loop.c       |  2 ++
 net/rds/rds.h        |  9 +++++++++
 net/rds/tcp.c        |  1 +
 5 files changed, 50 insertions(+)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index df26959b0fdc..90d660a45662 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;
 
@@ -362,6 +364,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 */
@@ -395,6 +398,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);
@@ -621,6 +625,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;
 
@@ -633,7 +638,31 @@ static void rds_conn_destroy_fini(struct kref *kref)
 	spin_lock_irqsave(&rds_conn_lock, flags);
 	rds_conn_count--;
 	spin_unlock_irqrestore(&rds_conn_lock, flags);
+
+	/* only after everything the transport module owns has been
+	 * freed above may its unload proceed
+	 */
+	if (!atomic_dec_return(&trans->t_conn_count))
+		wake_up_all(&rds_conn_freed_waitq);
+}
+
+/* Wait for all of @trans's connections to be freed; the free runs
+ * asynchronously once rds_conn_destroy() has quiesced a connection.
+ * Called on transport module unload, after the transport destroyed
+ * all of its connections: anything still holding a connection
+ * reference at that point is a short-lived lookup-style holder, so
+ * a bounded wait suffices - but warn if it expires, since the frees
+ * that follow the unload will then touch freed module text.
+ */
+void rds_conn_wait_conns_freed(struct rds_transport *trans)
+{
+	if (!wait_event_timeout(rds_conn_freed_waitq,
+				!atomic_read(&trans->t_conn_count),
+				msecs_to_jiffies(RDS_CONN_FREE_TIMEOUT_MS)))
+		WARN(1, "RDS/%s: %d connection(s) not freed\n",
+		     trans->t_name, atomic_read(&trans->t_conn_count));
 }
+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..755690583325 100644
--- a/net/rds/ib.c
+++ b/net/rds/ib.c
@@ -538,6 +538,15 @@ void rds_ib_exit(void)
 #endif
 	rds_ib_unregister_client();
 	rds_ib_destroy_nodev_conns();
+	rds_conn_wait_conns_freed(&rds_ib_transport);
+
+	/* Tearing down the last connection may have dropped the final
+	 * reference on a device, deferring rds_ib_dev_free() to rds_wq.
+	 * Drain it before the module goes away; it queues nothing
+	 * further on rds_wq.
+	 */
+	flush_workqueue(rds_wq);
+
 	rds_ib_sysctl_exit();
 	rds_ib_recv_exit();
 	rds_trans_unregister(&rds_ib_transport);
diff --git a/net/rds/loop.c b/net/rds/loop.c
index e6b0750bbeda..7daf8ed25d69 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);
 }
 
 static void rds_loop_kill_conns(struct net *net)
diff --git a/net/rds/rds.h b/net/rds/rds.h
index e27e5e2e3329..35760dd6b077 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -553,6 +553,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);
@@ -830,6 +836,9 @@ 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);
+/* how long transport unload waits for its connections to be freed */
+#define RDS_CONN_FREE_TIMEOUT_MS	10000
+void rds_conn_wait_conns_freed(struct rds_transport *trans);
 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..2685ee21a22d 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);
 	rds_trans_unregister(&rds_tcp_transport);
 	rds_tcp_recv_exit();
 	kmem_cache_destroy(rds_tcp_conn_slab);
-- 
2.25.1

[PATCH net-next 5/6] net/rds: drop rds_conn_count in favor of t_conn_count

[Allison Henderson]

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

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

Based on Oracle UEK commit 905ec90e6166 ("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 90d660a45662..2c8ca54a91ca 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];
@@ -302,8 +301,9 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 				     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,
+						atomic_read(&trans->t_conn_count),
+						i);
 		if (!conn->c_path[i].cp_wq)
 			conn->c_path[i].cp_wq = rds_wq;
 	}
@@ -363,7 +363,6 @@ static struct rds_connection *__rds_conn_create(struct net *net,
 			rds_conn_get(conn);	/* caller */
 			parent->c_passive = conn;
 			rds_cong_add_conn(conn);
-			rds_conn_count++;
 			atomic_inc(&conn->c_trans->t_conn_count);
 		}
 	} else {
@@ -397,7 +396,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);
 		}
 	}
@@ -626,7 +624,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++)
@@ -635,10 +632,6 @@ static void rds_conn_destroy_fini(struct kref *kref)
 	kfree(conn->c_path);
 	kmem_cache_free(rds_conn_slab, conn);
 
-	spin_lock_irqsave(&rds_conn_lock, flags);
-	rds_conn_count--;
-	spin_unlock_irqrestore(&rds_conn_lock, flags);
-
 	/* only after everything the transport module owns has been
 	 * freed above may its unload proceed
 	 */
-- 
2.25.1

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

[Allison Henderson]

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

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

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

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

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

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

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

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

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

This keeps a connection allocated for as long as messages that arrived
over it are queued on sockets, which is longer than before but costs
only the connection's memory: rds_conn_destroy() still quiesces the
connection synchronously, so a lingering inc holds nothing running.

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

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

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

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

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

Re: [PATCH net-next 1/6] net/rds: make rds_destroy_pending() cover single-connection destroy

[netdev-bot+sashiko]

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

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

- [High] The fix is incomplete against its own stated invariant, so the
  bug class it claims to close stays reachable.
- [Medium] The commit message mischaracterizes which destroy paths this
  patch actually newly guards.
- [Low] The comment on the new struct field misdescribes the write side.

--

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

This adds a per-connection c_destroy_in_prog flag, set in
rds_conn_destroy() before the unhash + synchronize_rcu() sequence and
tested in rds_destroy_pending(), and removes the now-dead
RDS_DESTROY_PENDING cp_flags bit along with its test in
rds_ib_is_unloading().

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

[Severity: High]
Is the claim in this comment that the requeueing sites "all test
rds_destroy_pending() under rcu_read_lock()" accurate for the current
tree?

A grep of the queue_work()/queue_delayed_work() callers under net/rds
turns up several arming sites that never consult the predicate at all,
so the new flag cannot suppress them:

net/rds/ib_send.c:rds_ib_send_cqe_handler()
	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);

net/rds/ib_send.c:rds_ib_send_add_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);

net/rds/ib_recv.c:rds_ib_recv_refill()
		queue_delayed_work(conn->c_path->cp_wq, &conn->c_recv_w, 1);

net/rds/tcp_listen.c:rds_tcp_accept_one()
		queue_delayed_work(cp->cp_wq, &cp->cp_recv_w, 0);

net/rds/send.c:rds_sendmsg()
		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);

Also, the one rds_destroy_pending(conn) test that rds_sendmsg() does
perform is not inside an RCU read-side section (send.c holds
rcu_read_lock() only around the three cp_send_w arming sites), so
the synchronize_rcu() in rds_conn_destroy() does not order against that
reader either.

Given that, can the window that rds_conn_path_quiesce() already warns
about still end in a crash?  It cancels the works while the path is
still RDS_CONN_UP:

	cancel_delayed_work_sync(&cp->cp_send_w);
	cancel_delayed_work_sync(&cp->cp_recv_w);

	rds_conn_path_drop(cp, true);
	flush_work(&cp->cp_down_w);
	...
	WARN_ON(delayed_work_pending(&cp->cp_send_w));
	WARN_ON(delayed_work_pending(&cp->cp_recv_w));
	WARN_ON(delayed_work_pending(&cp->cp_conn_w));

and teardown then proceeds regardless of the warning to

		destroy_workqueue(cp->cp_wq);
		cp->cp_wq = NULL;

followed by kfree(conn->c_path) in rds_conn_destroy().  An IB send or
receive completion landing in that window re-arms through one of the
unguarded sites above.  With delay 0 and cp_wq already NULL,
__queue_delayed_work() falls through to __queue_work(cpu, NULL, work);
with the delay 1 arming in rds_ib_recv_refill() the timer is invisible
to destroy_workqueue() and delayed_work_timer_fn() later calls
__queue_work(dwork->cpu, dwork->wq, ...) on the destroyed workqueue,
from a timer_list living in the freed c_path array.

Would it make sense to either extend the rcu_read_lock() +
rds_destroy_pending() pattern to those sites (or move the workqueue
teardown behind the transport quiesce), or otherwise to soften the
blanket coverage claim in this comment and in the changelog?

>  
>  	/* 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 786f39169bc14..9fe3b9951bd32 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;
>  }

[Severity: Medium]
Does the changelog describe the right set of paths that this patch newly
guards?  It says:

    rds_conn_destroy() is not limited to the global paths: rds_ib
    destroys connections whose underlying IB device was removed
    (rds_ib_destroy_nodev_conns())

and

    Bring the protection back at the connection level, where it also
    covers the device-removal path that was never guarded

At this revision net/rds/ib.c has no rds_conn_destroy() call at all; the
device removal path (rds_ib_remove_one() / rds_ib_dev_shutdown() /
rds_ib_remove_conn()) only drops connections and moves them onto
ib_nodev_conns.

rds_ib_destroy_nodev_conns() has a single caller, rds_ib_exit(), which
runs before it:

net/rds/ib.c:rds_ib_exit() {
	rds_ib_set_unloading();
	synchronize_rcu();
	...
	rds_ib_unregister_client();
	rds_ib_destroy_nodev_conns();
	...
}

Since rds_ib_is_unloading() is the IB ->t_unloading callback and returns
atomic_read(&rds_ib_unloading) != 0 for the whole of rds_ib_exit(),
wasn't rds_destroy_pending() already true for every conn destroyed on
that path, so the new flag adds nothing there?

That would leave the version-mismatch rds_conn_destroy() in
rds_ib_cm_connect_complete() as the only genuinely unguarded per-conn
destroy, which matches the Fixes: cdc306a5c9cd3 tag.  Could the
changelog be narrowed to that, so stable backport triage does not read
it as "hot-removing an IB device can queue work on a destroyed cp_wq"?

>  
>  void rds_ib_exit(void)
> diff --git a/net/rds/rds.h b/net/rds/rds.h
> index 2db49573dacd5..cede2b03baa50 100644
> --- a/net/rds/rds.h
> +++ b/net/rds/rds.h

[ ... ]

> @@ -148,6 +147,10 @@ struct rds_connection {
>  				c_pad_to_32:29;
>  	int			c_npaths;
>  	bool			c_with_sport_idx;
> +	/* Set (under RCU) when rds_conn_destroy() starts on this conn;
> +	 * read through rds_destroy_pending().
> +	 */
> +	bool			c_destroy_in_prog;

[Severity: Low]
This isn't a bug, but does "Set (under RCU)" match the setter?  The only
write is the plain WRITE_ONCE(conn->c_destroy_in_prog, true) in
rds_conn_destroy(), which is not inside an rcu_read_lock() section; the
RCU involvement is the synchronize_rcu() that follows.

Would it also be worth spelling out the reader-side requirement here,
namely that rcu_read_lock() must be held across both the
rds_destroy_pending() test and the queue_work()/queue_delayed_work()
that follows?  As written, the field comment permits the pattern already
present in rds_sendmsg():

net/rds/send.c:rds_sendmsg() {
	...
	if (rds_destroy_pending(conn)) {
		ret = -EAGAIN;
		goto out;
	}
	...
}

where the predicate is read outside any RCU read section and the
synchronize_rcu() in rds_conn_destroy() fences nothing.

>  	struct rds_connection	*c_passive;
>  	struct rds_transport	*c_trans;
>  
> @@ -994,7 +997,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));
>  }

Thanks for looking at these.

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

Re: [PATCH net-next 2/6] net/rds: split connection destroy into quiesce and kref-governed free

[netdev-bot+sashiko]

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

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

- [Low] Commit message misattributes the non-rmmod destroy paths.
- [Low] Concern (assessed as a false positive, retained for policy
  reasons): the new c_refcount comment in net/rds/rds.h:140-144 was read…
- [Low] Stale symbol reference after the rename performed by this patch:
  the correctness argument inside rds_conn_shutdown() still says…

Pre-existing issues:
- [High] Deadlock when an IB connection is destroyed from the CM event
  handler after unsupported protocol negotiation:…

--

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

This patch adds a kref to struct rds_connection and splits the teardown so
that rds_conn_destroy() still quiesces synchronously while the frees (the
transport conn_free, the path workqueues, the c_path array and the slab
object) move to rds_conn_destroy_fini(), which runs from the last
rds_conn_put().  rds_conn_get()/rds_conn_put() are exported for the
reference holders added in later patches.

No Link: tags are present in the commit header.

A couple of questions below, one about the changelog and two about the
teardown path.

[Severity: Low]

The changelog says:

    Today, connections can also be destroyed when the underlying IB
    device is removed (in rds_ib_destroy_nodev_conns()), or when a peer
    negotiates an unsupported protocol version.

Is rds_ib_destroy_nodev_conns() the device removal path?  It has a single
caller, rds_ib_exit(), which is the rds_rdma module unload path, i.e. the
same rmmod case the old comment allowed.

The IB device removal path, rds_ib_dev_shutdown(), only drops paths:

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

Meanwhile the two destroy paths that really are not rmmod are the netns
teardown ones, and they are not mentioned:

	rds_tcp_exit_net() -> rds_tcp_kill_sock() -> rds_conn_destroy()
	rds_loop_exit_net() -> rds_loop_kill_conns() -> rds_conn_destroy()

Would citing those two, together with the peer protocol version case in
rds_ib_cm_connect_complete(), describe the motivation more accurately?

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

[ ... ]

> @@ -515,10 +516,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)

[Severity: Low]

With rds_conn_path_destroy() gone, the comment inside rds_conn_shutdown()
that justifies the cancel_delayed_work_sync(&cp->cp_conn_w) call still
names it:

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

That comment is now the only match for rds_conn_path_destroy in the tree,
and it stays that way through the end of the series.  Since the flush of
cp_down_w is done by rds_conn_path_quiesce() while the free happens later
in rds_conn_path_free(), should the comment be reworded to name
rds_conn_path_quiesce() and note that the free is deferred?

> +static void rds_conn_path_quiesce(struct rds_conn_path *cp)
>  {
>  	struct rds_message *rm, *rtmp;
>  

[Severity: High]

This isn't a bug introduced by this patch, the same sequence exists in the
baseline rds_conn_path_destroy(), but can the quiesce deadlock when it is
reached from the RDMA CM event handler?

rds_conn_path_quiesce() keeps the drop plus synchronous flush:

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

rds_rdma_cm_event_handler_cmn() holds the mutex across the callback:

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

and rds_single_path.h aliases that mutex to the path one:

	#define	c_cm_lock		c_path[0].cp_cm_lock

If the peer negotiates a version below RDS_PROTOCOL_VERSION,
rds_ib_cm_connect_complete() destroys the connection with that mutex still
held:

		rds_conn_destroy(conn);
		return;

rds_conn_path_drop(cp, true) sets cp_state to RDS_CONN_ERROR and queues
cp_down_w, so the flushed worker runs rds_shutdown_worker() ->
rds_conn_shutdown(), where the initial transition fails and it blocks on
the same mutex:

	if (!rds_conn_path_transition(cp, RDS_CONN_DOWN, RDS_CONN_DOWN)) {
		...
		mutex_lock(&cp->cp_cm_lock);

Does flush_work() then wait for a worker that is waiting for the mutex the
blocked CM handler owns?  The chain has no trylock or timeout, and it is
unchanged at the head of the series, so would this be a good place to
break the cycle while the destroy path is being reworked?

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

[ ... ]

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

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

[netdev-bot+sashiko]

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

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

- [High] Transport per-connection nodes can stay linked into a stack-
  local temporary list after rds_conn_destroy() returns, and the…
- [High] The commit asserts that cm_id->context may stay reference-free
  because "connection destroy tears down the cm_id before the connection…
- [Medium] rds_sendmsg() now declares and enforces rs->rs_lock as the
  serialization for rs->rs_conn (plain load under rs_lock at…
- [Medium] The new reference model makes parent->c_passive own a
  reference that is only dropped when the *parent* is destroyed, but…
- [Medium] parent->c_passive becomes a concurrently written, RCU-consumed
  pointer with no RCU annotations.
- [Low] The patch closes a syzbot-reported KCSAN data race on rs->rs_conn
  (Reported-by/Closes tags present) and removes stale-pointer reuse of a…
- [Low] The new comment in rds_conn_lookup() asserts that an RCU
  traversal of rds_conn_hash may legitimately encounter a connection…

--

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

This hands out real references wherever a struct rds_connection pointer
used to escape bare: rds_conn_lookup() and __rds_conn_create() return a
held reference, the rs->rs_conn sendmsg cache owns one and is serialized
under rs_lock, and parent->c_passive owns one dropped when the parent is
destroyed.  The cm_id->context back-pointers are left reference-free by
design.

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

>     Reported-by: syzbot+879c1877016972360186@syzkaller.appspotmail.com
>     Closes: https://syzkaller.appspot.com/bug?extid=879c1877016972360186

[Severity: Low]
The changelog closes a reported KCSAN data race but carries no Fixes: tag
for the commit that introduced the unsynchronized rs_conn cache (git blame
points at 5c11559046c4 "RDS: send.c").

Should a Fixes: tag be added, or if the patch is intentionally not a stable
candidate because it depends on the earlier patches in this series, could
the changelog say so explicitly?

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index f45fd1fb1843d..df26959b0fdce 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

> @@ -96,6 +99,13 @@ 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) {
> +			/* An entry whose refcount already dropped to
> +			 * zero has been unhashed and is about to be
> +			 * freed; an RCU traversal may still come
> +			 * across it.  Treat it as absent.
> +			 */
> +			if (!kref_get_unless_zero(&conn->c_refcount))
> +				continue;
>  			ret = conn;
>  			break;
>  		}

[Severity: Low]
Can an RCU traversal of rds_conn_hash actually observe an entry whose
refcount has already reached zero?  rds_conn_destroy() unhashes the node
under rds_conn_lock, calls synchronize_rcu(), and only then drops the
initial reference, so a reader that can still reach the node appears to be
inside a grace period where the refcount is at least one.

If the state the comment describes were reachable, would the guard even
help?  rds_conn_destroy_fini() frees immediately at the final put:

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

and rds_conn_init() creates the cache without SLAB_TYPESAFE_BY_RCU:

	rds_conn_slab = KMEM_CACHE(rds_connection, 0);

so the c_faddr/c_laddr/c_trans comparisons above the guard would already be
reads of freed or recycled memory.

kref_get_unless_zero() itself is a harmless idiom here; could the comment
be reworded so a later reader does not conclude that zero-refcount entries
are safely traversable?

> @@ -197,7 +207,14 @@ 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;
> +		/* 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.
> +		 */
>  		conn = parent->c_passive;
> +		if (conn)
> +			rds_conn_get(conn);
>  	}
>  	rcu_read_unlock();
>  	if (conn)

[Severity: Medium]
Nothing here tests c_destroy_in_prog on the passive child, and nothing
clears the parent's c_passive when the passive connection itself is
destroyed - rds_conn_destroy() only clears its own c_passive:

net/rds/connection.c:rds_conn_destroy() {
	...
	if (conn->c_destroy_in_prog) {
		spin_unlock_irq(&rds_conn_lock);
		return;
	}
	...
	passive = conn->c_passive;
	conn->c_passive = NULL;
}

Since a quiesced connection now stays alive until its last reference goes
away, can this hand an already-destroyed passive conn, with a fresh
reference, back to rds_ib_cm_handle_connect()?

The quiesce leaves the path in RDS_CONN_DOWN with ic->i_cm_id == NULL, so
the DOWN->CONNECTING transition succeeds and a new cm_id is installed,
rds_ib_setup_qp() runs and rdma_accept() is called.  A second
rds_conn_destroy() then returns early on the c_destroy_in_prog guard, so
that state is never torn down and rds_conn_destroy_fini() only calls
conn_free(), which kfree()s the rds_ib_connection with the QP and CQs still
armed.

The one single-conn destroy path for IB is the version-mismatch drop in
rds_ib_cm_connect_complete(); would refusing to return a conn with
c_destroy_in_prog set (or invalidating the parent's pointer when the child
is destroyed) close this?

Separately, even without reuse, does the new c_passive reference keep a
quiesced passive conn - its transport state and its per-path workqueue -
pinned for the whole lifetime of the parent?

> @@ -316,12 +333,32 @@ static struct rds_connection *__rds_conn_create(struct net *net,
>  	spin_lock_irqsave(&rds_conn_lock, flags);
>  	if (parent) {
>  		/* Creating passive conn */
> -		if (parent->c_passive) {
> +		if (READ_ONCE(parent->c_destroy_in_prog)) {
> +			/* The parent's destroy has begun (it sets the
> +			 * flag and snatches c_passive under this
> +			 * lock); do not install a new passive conn
> +			 * that nothing would ever destroy.
> +			 */
> +			trans->conn_free(conn->c_path[0].cp_transport_data);
> +			free_cp = conn->c_path;
> +			kmem_cache_free(rds_conn_slab, conn);
> +			conn = ERR_PTR(-ENETDOWN);
> +		} else if (parent->c_passive) {
> +			rds_conn_get(parent->c_passive);
>  			trans->conn_free(conn->c_path[0].cp_transport_data);
>  			free_cp = conn->c_path;
>  			kmem_cache_free(rds_conn_slab, conn);
>  			conn = parent->c_passive;
>  		} else {
> +			/* 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 */
>  			parent->c_passive = conn;
>  			rds_cong_add_conn(conn);
>  			rds_conn_count++;

[Severity: Medium]
The install of the passive conn is a plain store consumed by a reader that
holds only rcu_read_lock():

	parent->c_passive = conn;

Should this be rcu_assign_pointer(), with rcu_dereference() at the read
site above and a __rcu annotation on the field in struct rds_connection?

The initialising stores to c_trans, c_path and the transport data are
ordered only by the rds_conn_lock unlock, which gives nothing to a reader
that never acquires that lock.  On a weakly ordered CPU, can the RCU reader
in __rds_conn_create() see c_passive != NULL with those fields still stale,
and then pass that pointer to rds_ib_cm_handle_connect(), which
dereferences them?

[ ... ]

> @@ -646,6 +690,16 @@ 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 = conn->c_passive;
> +	conn->c_passive = NULL;
>  	spin_unlock_irq(&rds_conn_lock);
>  	synchronize_rcu();

[Severity: Medium]
This adds a second concurrent writer of c_passive as a plain store.  Before
this patch c_passive was written once and never cleared, so the unlocked
read in __rds_conn_create() had no competing writer.

The reader takes rcu_read_lock() but not rds_conn_lock, so the two accesses
share no lock.  Is this pair reportable by KCSAN in the same way as the
rs_conn race the changelog fixes, and would WRITE_ONCE()/rcu_assign_pointer()
here plus rcu_dereference() at the read site be preferable?

> @@ -663,6 +717,10 @@ void rds_conn_destroy(struct rds_connection *conn)
>  	 */
>  	rds_cong_remove_conn(conn);
>  
> +	/* drop the reference our c_passive pointer held, if any */
> +	if (passive)
> +		rds_conn_put(passive);
> +
>  	/* drop the initial reference; the connection is freed from
>  	 * rds_conn_destroy_fini() once every holder has dropped theirs
>  	 */

[Severity: High]
Now that real holders exist (the socket rs_conn cache, an in-flight
sendmsg, c_passive, lookup callers), rds_conn_destroy() can return with the
connection still alive and trans->conn_free() deferred to the final put.

Do the transport teardown helpers still rely on conn_free() unlinking the
per-connection node before they return?  They keep the node linked into a
list head on their own stack:

net/rds/ib_rdma.c:rds_ib_destroy_nodev_conns() {
	LIST_HEAD(tmp_list);

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

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

and the unlink happens much later, from the deferred free:

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

For a node whose neighbour is the tmp_list head, does that list_del() write
ic->ib_node.prev->next and .next->prev into a stack frame that has already
returned?

The same shape appears in rds_tcp_destroy_conns() and rds_tcp_kill_sock(),
where the node moved to tmp_list is not marked t_tcp_node_detached, so
rds_tcp_conn_free() takes the list_del() branch:

net/rds/tcp.c:rds_tcp_conn_free() {
	...
	if (!tc->t_tcp_node_detached)
		list_del(&tc->t_tcp_node);
}

and in rds_loop_kill_conns() with rds_loop_conn_free()'s
list_del(&lc->loop_node).

The later series patch that waits for connections to be freed on transport
unload does not appear to cover these: rds_ib_exit() calls
rds_ib_destroy_nodev_conns() and only then
rds_conn_wait_conns_freed(&rds_ib_transport), and rds_tcp_exit() waits
after rds_tcp_destroy_conns() has returned; the netns paths
(rds_tcp_kill_sock, rds_loop_kill_conns) get no wait at all.

Would unlinking the node (or marking it detached, as rds_tcp_kill_sock
already does for the secondary nodes) before rds_conn_destroy() can return,
or using a list head that outlives the pending frees, avoid this?

> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 4feb0edc360c8..24e538c253a7a 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -924,8 +924,14 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id,
>  		rds_ib_conn_error(conn, "rdma_accept failed\n");
>  
>  out:
> -	if (conn)
> +	if (conn) {
>  		mutex_unlock(&conn->c_cm_lock);
> +		/* The conn stays reachable through cm_id->context
> +		 * without a reference of its own: connection destroy
> +		 * shuts the cm_id down before the conn is freed.
> +		 */
> +		rds_conn_put(conn);
> +	}
>  	if (err)
>  		rdma_reject(cm_id, &err, sizeof(int),
>  			    IB_CM_REJ_CONSUMER_DEFINED);

[Severity: High]
Is anything enforcing the invariant in this comment, that destroy tears the
cm_id down before the conn can be freed?

The CM event handler picks up the back-pointer with no reference and locks
a mutex that lives inside the separately freed c_path array
(c_cm_lock is c_path[0].cp_cm_lock per net/rds/rds_single_path.h):

net/rds/rdma_transport.c:rds_rdma_cm_event_handler_cmn() {
	struct rds_connection *conn = cm_id->context;
	...
	if (conn)
		mutex_lock(&conn->c_cm_lock);
	...
	case RDMA_CM_EVENT_ESTABLISHED:
		... rds_ib_cm_connect_complete(conn, event);
	...
	if (conn)
		mutex_unlock(&conn->c_cm_lock);
}

and for an unsupported peer protocol version that ESTABLISHED handling
destroys the conn from inside the callback:

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

Since this hunk drops the caller reference at out:, can that destroy be the
final put, so rds_conn_destroy_fini() kfree()s c_path and the conn before
the handler's mutex_unlock(&conn->c_cm_lock) runs on freed memory?

On the same path, does destroy deadlock the rdma_cm event workqueue?

rds_conn_destroy()
  rds_conn_path_quiesce()
    rds_conn_path_drop(cp, true)
    flush_work(&cp->cp_down_w)
      rds_shutdown_worker() -> rds_conn_shutdown()
        mutex_lock(&cp->cp_cm_lock)   <- already held by this CM callback

Also, rds_ib_cm_handle_connect() does not re-check rds_destroy_pending(conn)
after taking c_cm_lock.  When a destroy's quiesce has already completed, the
path is back in RDS_CONN_DOWN, so can this still win the DOWN->CONNECTING
transition, install ic->i_cm_id and cm_id->context, set up the QP and
accept, after which the new rds_conn_put() here is the final put and frees
the conn and its rds_ib_connection with the cm_id and QP still armed?

Would a real reference for the CM back-pointer (kref_get_unless_zero()) or
an rds_destroy_pending() re-check under c_cm_lock be needed to make the
comment's claim hold?

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 1afa981e5c06d..036a68372e2f4 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c

[ ... ]

> @@ -1340,11 +1341,29 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
>  	rm->m_daddr = daddr;
>  
>  	/* rds_conn_create has a spinlock that runs with IRQ off.
> -	 * Caching the conn in the socket helps a lot. */
> -	if (rs->rs_conn && ipv6_addr_equal(&rs->rs_conn->c_faddr, &daddr) &&
> -	    rs->rs_tos == rs->rs_conn->c_tos) {
> -		conn = rs->rs_conn;
> +	 * Caching the conn in the socket helps a lot.
> +	 *
> +	 * The cached rs_conn holds a connection reference; take one of
> +	 * our own for the duration of this call (dropped on both exit
> +	 * paths), so that neither a concurrent sender replacing the
> +	 * cache nor rds_conn_destroy() can free the connection under
> +	 * us.  A cached connection whose destruction has begun is not
> +	 * reused: dropping it here lets the next sendmsg look up or
> +	 * create a live one instead of returning -EAGAIN forever.
> +	 */
> +	spin_lock_irqsave(&rs->rs_lock, flags);
> +	conn = rs->rs_conn;
> +	if (conn && ipv6_addr_equal(&conn->c_faddr, &daddr) &&
> +	    rs->rs_tos == conn->c_tos && !rds_destroy_pending(conn)) {
> +		rds_conn_get(conn);
>  	} else {
> +		conn = NULL;
> +	}
> +	spin_unlock_irqrestore(&rs->rs_lock, flags);
> +
> +	if (!conn) {
> +		struct rds_connection *old;
> +
>  		conn = rds_conn_create_outgoing(sock_net(sock->sk),
>  						&rs->rs_bound_addr, &daddr,
>  						rs->rs_transport, rs->rs_tos,
> @@ -1352,9 +1371,17 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
>  						scope_id);
>  		if (IS_ERR(conn)) {
>  			ret = PTR_ERR(conn);
> +			conn = NULL;
>  			goto out;
>  		}
> +		/* hand the cache its own reference */
> +		rds_conn_get(conn);
> +		spin_lock_irqsave(&rs->rs_lock, flags);
> +		old = rs->rs_conn;
>  		rs->rs_conn = conn;
> +		spin_unlock_irqrestore(&rs->rs_lock, flags);
> +		if (old)
> +			rds_conn_put(old);
>  	}

[Severity: Medium]
The changelog says "reads and updates of rs_conn are serialized by
rs_lock", but rds_ioctl(SIOCRDSSETTOS) still reads rs_conn and writes
rs_tos under an unrelated global lock:

net/rds/af_rds.c:rds_ioctl() {
	...
	spin_lock_bh(&rds_sock_lock);
	if (rs->rs_tos || rs->rs_conn) {
		spin_unlock_bh(&rds_sock_lock);
		return -EINVAL;
	}
	rs->rs_tos = tos;
	spin_unlock_bh(&rds_sock_lock);
}

The two paths share no lock.  Is the plain load of rs->rs_conn there still
racing with the plain store of rs->rs_conn here, on the same eight-byte
field that the KCSAN report names?

The ioctl's "TOS cannot change once a connection exists" test also looks
like a check-then-act across the two locks:

ioctl(SIOCRDSSETTOS)                sendmsg()
  reads rs->rs_conn == NULL
                                      rds_conn_create_outgoing(..., rs->rs_tos, ...)
  rs->rs_tos = tos;
                                      rs->rs_conn = conn;

Can that leave a cached connection whose c_tos no longer matches rs_tos, so
every later sendmsg fails the rs->rs_tos == conn->c_tos test above and
recreates or re-looks-up a connection?

[ ... ]

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

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

[netdev-bot+sashiko]

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

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

- [High] rds_conn_wait_conns_freed() (net/rds/connection.c) is the entire
  enforcement mechanism the commit adds, but it is a `void` function…
- [High] The IB call site violates the documented precondition of the new
  helper.
- [High] The new wait is placed in rds_tcp_exit()/rds_ib_exit() *after*
  rds_tcp_destroy_conns()/rds_ib_destroy_nodev_conns() have returned, so…
- [Medium] Series ordering: this commit repairs a hazard that the
  immediately preceding commit in the same series creates.

--

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

This adds a per-transport live connection count (t_conn_count) that is
incremented when a connection is published in __rds_conn_create() and
decremented at the end of rds_conn_destroy_fini().  The transport exit
paths then call a new rds_conn_wait_conns_freed() helper that waits, with
a 10 second bound, for that count to drop to zero before unloading.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index df26959b0fdce..90d660a45662e 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

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

[Severity: Medium]

A question about the ordering of this series rather than this hunk itself.

The commit "net/rds: split connection destroy into quiesce and kref-governed
free" is described as functionally equivalent, since the initial reference is
still the only one, so the free continues to happen inline in
rds_conn_destroy().

The commit "net/rds: hold connection references in lookup, sockets and
c_passive" appears to be the first point at which rds_conn_lookup(), rs_conn
and c_passive hold real references, i.e. the first point at which
rds_conn_destroy() can return with c_trans->conn_free() still pending.

The wait that covers that window only arrives here, one commit later.  Does
the tree therefore pass through a state where rmmod rds_tcp / rmmod rds_rdma
racing a pending free has no barrier at all?  Would it make sense to place
this wait ahead of (or squash it into) the commit that adds the extra
reference holders, or at least mention the dependency in that commit
message?

> +
> +/* Wait for all of @trans's connections to be freed; the free runs
> + * asynchronously once rds_conn_destroy() has quiesced a connection.
> + * Called on transport module unload, after the transport destroyed
> + * all of its connections: anything still holding a connection
> + * reference at that point is a short-lived lookup-style holder, so
> + * a bounded wait suffices - but warn if it expires, since the frees
> + * that follow the unload will then touch freed module text.
> + */
> +void rds_conn_wait_conns_freed(struct rds_transport *trans)
> +{
> +	if (!wait_event_timeout(rds_conn_freed_waitq,
> +				!atomic_read(&trans->t_conn_count),
> +				msecs_to_jiffies(RDS_CONN_FREE_TIMEOUT_MS)))
> +		WARN(1, "RDS/%s: %d connection(s) not freed\n",
> +		     trans->t_name, atomic_read(&trans->t_conn_count));
>  }
> +EXPORT_SYMBOL_GPL(rds_conn_wait_conns_freed);

[Severity: High]

Is the use-after-free this patch describes still reachable through the
timeout branch of rds_conn_wait_conns_freed()?

The helper returns void, so on expiry it warns and returns, and none of the
three callers can react.  Teardown then continues unconditionally:

  rds_tcp_exit()
	rds_conn_wait_conns_freed(&rds_tcp_transport);	/* may time out */
	rds_trans_unregister(&rds_tcp_transport);
	rds_tcp_recv_exit();
	kmem_cache_destroy(rds_tcp_conn_slab);

  rds_ib_exit()
	rds_conn_wait_conns_freed(&rds_ib_transport);	/* may time out */
	...
	rds_ib_recv_exit();		/* destroys rds_ib_incoming_slab,
					 * rds_ib_frag_slab */
	rds_trans_unregister(&rds_ib_transport);
	rds_ib_mr_exit();

  rds_loop_exit() returns into rds_exit()/rds_conn_exit(), which destroys
  rds_conn_slab.

When the pending free finally runs, rds_conn_destroy_fini() does two things
that touch the unloaded module:

	for (i = 0; i < npaths; i++)
		rds_conn_path_free(&conn->c_path[i]);	/* -> c_trans->conn_free() */
	...
	if (!atomic_dec_return(&trans->t_conn_count))

The first is an indirect call into freed module text; the second is a write
into the module's freed data image.  For TCP the deferred
rds_tcp_conn_free() also lands in kmem_cache_free() on the already destroyed
rds_tcp_conn_slab, and for IB rds_ib_recv_free_caches() frees into slabs
destroyed by rds_ib_recv_exit().

The comment added on t_conn_count in rds.h says unload "has to wait for this
to reach zero", and the commit message says "if it expires anyway, warn - the
pending frees will touch freed module text".  Since a WARN is a diagnostic
rather than synchronisation, would an unbounded wait, a per-connection
module reference on trans->t_owner, or moving the transport-owned frees off
the asynchronous path be a better fit here?

As a side effect, does rmmod now block for up to 10 seconds, and does the
WARN take down machines running with panic_on_warn=1?

[Severity: High]

Can the deferred free write into an already popped stack frame?

The wait is placed after rds_tcp_destroy_conns() and
rds_ib_destroy_nodev_conns() have returned, so it cannot keep their
stack-local list heads alive:

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

Entries whose free is deferred stay linked in tmp_list, and
t_tcp_node_detached is left false, so the deferred free later does:

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

For an entry adjacent to the dead &tmp_list head, list_del() writes two
pointers into that popped frame, while the exit thread is sitting in
rds_conn_wait_conns_freed() -> wait_event_timeout() -> schedule_timeout()
on the same stack region.

The IB side is the same shape, with an unconditional list_del():

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

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

Note that rds_conn_destroy_fini() calls conn_free() before decrementing
t_conn_count, so the exit thread cannot observe completion ahead of the
write.  A conn already marked c_destroy_in_prog is guaranteed not to be
freed inside the destroy loop, since rds_conn_destroy() returns immediately
for it.

Would moving the wait inside the destroy helpers, or splicing onto a
non-stack list head, address this?  rds_loop_exit() looks safe only
incidentally, because its tmp_list and the wait share one frame.  The same
pattern appears in rds_tcp_kill_sock() and rds_loop_kill_conns() on netns
teardown.

> diff --git a/net/rds/ib.c b/net/rds/ib.c
> index 9fe3b9951bd32..7556905833259 100644
> --- a/net/rds/ib.c
> +++ b/net/rds/ib.c
> @@ -538,6 +538,15 @@ void rds_ib_exit(void)
>  #endif
>  	rds_ib_unregister_client();
>  	rds_ib_destroy_nodev_conns();
> +	rds_conn_wait_conns_freed(&rds_ib_transport);

[Severity: High]

Does this call site meet the precondition stated in the new helper's
comment, that it is called "after the transport destroyed all of its
connections"?

Connections still attached to a device do not appear to be destroyed by
either preceding call.  ib_unregister_client() -> rds_ib_remove_one() ->
rds_ib_dev_shutdown() only queues work:

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

and rds_conn_path_drop() does queue_work(cp->cp_wq, &cp->cp_down_w) on the
per-path ordered workqueue, which the flush_workqueue(rds_wq) inside
rds_ib_unregister_client() does not drain.

An rds_ib_connection whose cp_down_w has not completed rds_ib_conn_shutdown()
-> rds_ib_remove_conn() is still on rds_ibdev->conn_list rather than
ib_nodev_conns, and rds_ib_destroy_nodev_conns() only walks ib_nodev_conns:

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

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

If such a connection's initial reference is never dropped, can t_conn_count
ever reach zero?  That would make the 10 second stall plus WARN the ordinary
outcome of rmmod rds_rdma with live IB connections, leave the module unloaded
with a live connection, and leak the rds_ib_connection along with its device
reference (so rds_ib_dev_free() never runs and the PD and MR pools are never
released).

Would draining the per-path workqueues, or destroying the per-device
conn_lists, before waiting cover these connections?

> +
> +	/* 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/rds.h b/net/rds/rds.h
> index e27e5e2e33293..35760dd6b077f 100644
> --- a/net/rds/rds.h
> +++ b/net/rds/rds.h
> @@ -553,6 +553,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;

[ ... ]

> diff --git a/net/rds/tcp.c b/net/rds/tcp.c
> index 774a71f88d375..2685ee21a22d1 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);
>  	rds_trans_unregister(&rds_tcp_transport);
>  	rds_tcp_recv_exit();
>  	kmem_cache_destroy(rds_tcp_conn_slab);

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

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

[netdev-bot+sashiko]

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

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

- [High] Module-unload barrier defeated: incs/messages parked on a socket
  now hold a `struct rds_connection` reference for a userspace-…
- [Medium] The commit message understates the cost and the residual
  exposure of the new reference.

--

net/rds: hold a connection reference from struct rds_incoming

This makes every holder of struct rds_incoming->i_conn own a connection
reference: the six assignment sites take one, and rds_inc_put() and the
new rds_message_free() helper drop it.  The stated goal is to stop
dereferences of i_conn from touching a connection that was already freed
by device removal, netns teardown or module unload.

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

First, a question about the commit message itself:

>     This keeps a connection allocated for as long as messages that arrived
>     over it are queued on sockets, which is longer than before but costs
>     only the connection's memory: rds_conn_destroy() still quiesces the
>     connection synchronously, so a lingering inc holds nothing running.

[Severity: Medium]
Is "costs only the connection's memory" accurate?  A lingering inc keeps
trans->t_conn_count non-zero, and that counter only reaches zero from
rds_conn_destroy_fini():

net/rds/connection.c:rds_conn_destroy_fini() {
	for (i = 0; i < npaths; i++)
		rds_conn_path_free(&conn->c_path[i]);
	...
	if (!atomic_dec_return(&trans->t_conn_count))
		wake_up_all(&rds_conn_freed_waitq);
}

so unloading a transport while a datagram sits unread stalls for
RDS_CONN_FREE_TIMEOUT_MS and then warns:

net/rds/connection.c:rds_conn_wait_conns_freed() {
	if (!wait_event_timeout(rds_conn_freed_waitq,
				!atomic_read(&trans->t_conn_count),
				msecs_to_jiffies(RDS_CONN_FREE_TIMEOUT_MS)))
		WARN(1, "RDS/%s: %d connection(s) not freed\n",
		     trans->t_name, atomic_read(&trans->t_conn_count));
}

Does "holds nothing running" also hold?  destroy_workqueue() and the
transport's per-connection state are released only at the final put:

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

	cp->cp_conn->c_trans->conn_free(cp->cp_transport_data);
}

so the per-path workqueues and, for IB, the connection's ic with its
receive caches stay allocated for as long as the new reference is held.
Could the message mention the 10 second unload stall plus WARN, and that
the per-path workqueues and transport state are kept alive too?

> diff --git a/net/rds/message.c b/net/rds/message.c
> index f25f2592586f7..29e95028e61e4 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);
> +}
> +

[ ... ]

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

[Severity: High]
Does the new reference outlive the transport unload barrier in the module
unload case this patch targets?

The get taken in rds_inc_init()/rds_inc_path_init() is dropped only from
rds_inc_put(), i.e. when userspace calls recvmsg() or close(), so the
hold time is chosen by the application.  The unload barrier, however, is
bounded:

net/rds/connection.c:rds_conn_wait_conns_freed() {
	if (!wait_event_timeout(rds_conn_freed_waitq,
				!atomic_read(&trans->t_conn_count),
				msecs_to_jiffies(RDS_CONN_FREE_TIMEOUT_MS)))
		WARN(1, "RDS/%s: %d connection(s) not freed\n",
		     trans->t_name, atomic_read(&trans->t_conn_count));
}

and its own comment states the assumption this patch appears to break -
"anything still holding a connection reference at that point is a
short-lived lookup-style holder".

Is there a configuration where the socket does not pin the transport of
the conn it holds?  rds_find_bound() matches on daddr, dport and
bound_if only, and rds_recv_incoming() re-points the inc at the arriving
conn, so a socket bound with one transport can end up queueing an inc
whose i_conn belongs to the other transport (for example an inbound
RDS/TCP conn to an IPoIB address on a host whose sockets prefer RDS/IB).
The socket's module reference from rds_set_transport() covers only its
own transport, so rmmod of the other one is not blocked, hits the
timeout and unloads anyway.

Once that happens, doesn't the later recvmsg() or close() run these two
calls against a module that is gone?

rds_inc_put()
    conn->c_trans->inc_free(inc);
    rds_conn_put(conn)
        rds_conn_destroy_fini()
            rds_conn_path_free()
                cp->cp_conn->c_trans->conn_free(cp->cp_transport_data);
            atomic_dec_return(&trans->t_conn_count);

By that point rds_tcp_exit() has already run rds_trans_unregister() and
kmem_cache_destroy(rds_tcp_conn_slab), and rds_ib_exit() has run
rds_ib_recv_exit(), which destroys rds_ib_incoming_slab and
rds_ib_frag_slab that the queued inc and its fragments came from.
rds_recvmsg() dereferences inc->i_conn->c_trans->inc_copy_to_user the
same way.

Would the barrier need to become unbounded, or the socket-held
references dropped or detached at quiesce time, so that the reachable
dereference is not simply moved from freed conn memory to freed module
text after the timeout?

The same question applies to the send side: a message parked on a socket
send queue drops its conn reference only from rds_message_free() at
close, so it extends the connection lifetime past the unload wait too.

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