* [PATCH net-next v8 01/13] net/rds: ib: don't enable interrupts in rds_ib_conn_free()
2026-10-03 16:32 [PATCH net-next v8 00/13] net/rds: make connection lifetime reference-counted Allison Henderson
@ 2026-10-03 16:32 ` Allison Henderson
2026-10-03 16:32 ` [PATCH net-next v8 02/13] net/rds: undo conn_alloc() the same way on every __rds_conn_create() exit Allison Henderson
` (11 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Allison Henderson @ 2026-10-03 16:32 UTC (permalink / raw)
To: netdev, linux-rdma, pabeni, edumazet, kuba, horms; +Cc: achender
rds_ib_conn_free() unlinks the connection from its device or nodev
list under spin_lock_irq()/spin_unlock_irq(). It is not only called
from the rmmod path, though: __rds_conn_create() calls
trans->conn_free() to undo a lost creation race while it still holds
rds_conn_lock, taken with spin_lock_irqsave(). The unconditional
spin_unlock_irq() then re-enables interrupts with rds_conn_lock held
and leaves them enabled when the caller's spin_unlock_irqrestore()
runs, defeating the irqsave the caller relied on.
Use the irqsave/irqrestore pair, as rds_tcp_conn_free() and
rds_loop_conn_free() already do.
Fixes: 745cbccac3fe ("RDS: Rewrite connection cleanup, fixing oops on rmmod")
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
net/rds/ib_cm.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 3ed03ad32812..8b16bd7c40ce 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -1284,6 +1284,7 @@ void rds_ib_conn_free(void *arg)
{
struct rds_ib_connection *ic = arg;
spinlock_t *lock_ptr;
+ unsigned long flags;
rdsdebug("ic %p\n", ic);
@@ -1291,12 +1292,16 @@ void rds_ib_conn_free(void *arg)
* Conn is either on a dev's list or on the nodev list.
* A race with shutdown() or connect() would cause problems
* (since rds_ibdev would change) but that should never happen.
+ *
+ * Callers may hold rds_conn_lock with interrupts disabled
+ * (__rds_conn_create() undoing a lost creation race), so do not
+ * re-enable interrupts unconditionally here.
*/
lock_ptr = ic->rds_ibdev ? &ic->rds_ibdev->spinlock : &ib_nodev_conns_lock;
- spin_lock_irq(lock_ptr);
+ spin_lock_irqsave(lock_ptr, flags);
list_del(&ic->ib_node);
- spin_unlock_irq(lock_ptr);
+ spin_unlock_irqrestore(lock_ptr, flags);
rds_ib_recv_free_caches(ic);
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH net-next v8 02/13] net/rds: undo conn_alloc() the same way on every __rds_conn_create() exit
2026-10-03 16:32 [PATCH net-next v8 00/13] net/rds: make connection lifetime reference-counted Allison Henderson
2026-10-03 16:32 ` [PATCH net-next v8 01/13] net/rds: ib: don't enable interrupts in rds_ib_conn_free() Allison Henderson
@ 2026-10-03 16:32 ` Allison Henderson
2026-10-03 16:32 ` [PATCH net-next v8 03/13] net/rds: ib: refuse to attach a connection to a device being removed Allison Henderson
` (10 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Allison Henderson @ 2026-10-03 16:32 UTC (permalink / raw)
To: netdev, linux-rdma, pabeni, edumazet, kuba, horms; +Cc: achender
trans->conn_alloc() may allocate transport data for every path of a
multipath connection - rds_tcp_conn_alloc() does - which is why the
lost-creation-race exit of __rds_conn_create() loops over all npaths
when it frees the connection it just built. The passive-connection
exit right above it frees only path 0.
That is not a leak today: a passive twin is only created for an IB
loopback connection (an incoming TCP connect to a local address is
refused with -EOPNOTSUPP before it gets here), and the IB transport is
not multipath, so npaths is 1 on that exit. But the two exits express
the same "undo conn_alloc()" step in two different ways, and the
following patches add another exit of the same kind. Move the loop
into a helper and use it everywhere, so that the step cannot silently
diverge if a multipath transport ever grows a passive twin.
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
net/rds/connection.c | 31 ++++++++++++++++++-------------
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/net/rds/connection.c b/net/rds/connection.c
index c752a8623cfc..95ff50f31d1e 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -161,6 +161,22 @@ static void __rds_conn_path_init(struct rds_connection *conn,
cp->cp_flags = 0;
}
+/* Undo trans->conn_alloc(): it may have allocated transport data for
+ * every path of a multipath connection, not just for path 0.
+ */
+static void rds_conn_free_transport_data(struct rds_connection *conn,
+ int npaths)
+{
+ struct rds_conn_path *cp;
+ int i;
+
+ for (i = 0; i < npaths; i++) {
+ cp = &conn->c_path[i];
+ if (cp->cp_transport_data)
+ conn->c_trans->conn_free(cp->cp_transport_data);
+ }
+}
+
/*
* There is only every one 'conn' for a given pair of addresses in the
* system at a time. They contain messages to be retransmitted and so
@@ -322,7 +338,7 @@ static struct rds_connection *__rds_conn_create(struct net *net,
if (parent) {
/* Creating passive conn */
if (parent->c_passive) {
- trans->conn_free(conn->c_path[0].cp_transport_data);
+ rds_conn_free_transport_data(conn, npaths);
free_cp = conn->c_path;
kmem_cache_free(rds_conn_slab, conn);
conn = parent->c_passive;
@@ -338,18 +354,7 @@ static struct rds_connection *__rds_conn_create(struct net *net,
found = rds_conn_lookup(net, head, laddr, faddr, trans,
tos, dev_if);
if (found) {
- struct rds_conn_path *cp;
- int i;
-
- for (i = 0; i < npaths; i++) {
- cp = &conn->c_path[i];
- /* The ->conn_alloc invocation may have
- * allocated resource for all paths, so all
- * of them may have to be freed here.
- */
- if (cp->cp_transport_data)
- trans->conn_free(cp->cp_transport_data);
- }
+ rds_conn_free_transport_data(conn, npaths);
free_cp = conn->c_path;
kmem_cache_free(rds_conn_slab, conn);
conn = found;
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH net-next v8 03/13] net/rds: ib: refuse to attach a connection to a device being removed
2026-10-03 16:32 [PATCH net-next v8 00/13] net/rds: make connection lifetime reference-counted Allison Henderson
2026-10-03 16:32 ` [PATCH net-next v8 01/13] net/rds: ib: don't enable interrupts in rds_ib_conn_free() Allison Henderson
2026-10-03 16:32 ` [PATCH net-next v8 02/13] net/rds: undo conn_alloc() the same way on every __rds_conn_create() exit Allison Henderson
@ 2026-10-03 16:32 ` Allison Henderson
2026-10-04 16:34 ` netdev-bot+sashiko
2026-10-03 16:32 ` [PATCH net-next v8 04/13] net/rds: guard every work-requeueing site with rds_destroy_pending() Allison Henderson
` (9 subsequent siblings)
12 siblings, 1 reply; 21+ messages in thread
From: Allison Henderson @ 2026-10-03 16:32 UTC (permalink / raw)
To: netdev, linux-rdma, pabeni, edumazet, kuba, horms; +Cc: achender
rds_ib_remove_one() drops every connection on the device's conn_list
in rds_ib_dev_shutdown(), then clears the client data so that no new
connect can find the device. A connect that is already past
rds_ib_get_client_data() when the walk runs is not covered by either:
rds_ib_setup_qp() goes on to rds_ib_add_conn(), which moves the
connection onto the conn_list the walk has just finished with, and
builds a QP on a device that is on its way out. Nothing drops that
connection afterwards - the device's shutdown walk is over, and the
connection never returns to ib_nodev_conns, which is the only list the
transport exit sweeps - so it outlives the device, and the module.
Make rds_ib_dev_shutdown() mark the device as shutting down under
rds_ibdev->spinlock before it walks conn_list, and have
rds_ib_add_conn() refuse, under the same lock, to attach a connection
to a device so marked. Every connection is then either on the list
when the walk drops it, or refused: the connect fails, the connection
stays on ib_nodev_conns, and either its own drop or the exit sweep
tears it down. rds_ib_setup_qp() has not taken anything from the
device at that point, so the failure needs no unwinding beyond the
client-data reference it already releases.
This mirrors the UEK gate on the device removal flag in
rds_ib_add_conn().
Fixes: fc19de38be92 ("RDS/IB: disconnect when IB devices are removed")
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
net/rds/ib.c | 5 +++++
net/rds/ib.h | 7 ++++++-
net/rds/ib_cm.c | 4 +++-
net/rds/ib_rdma.c | 17 +++++++++++++++--
4 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/net/rds/ib.c b/net/rds/ib.c
index 4ea9838d090c..a7647ec01a11 100644
--- a/net/rds/ib.c
+++ b/net/rds/ib.c
@@ -86,6 +86,11 @@ static void rds_ib_dev_shutdown(struct rds_ib_device *rds_ibdev)
unsigned long flags;
spin_lock_irqsave(&rds_ibdev->spinlock, flags);
+ /* Close the device to new connections under the same lock that
+ * rds_ib_add_conn() attaches them under, so that every
+ * connection is either dropped by the walk below or refused.
+ */
+ rds_ibdev->shutting_down = true;
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);
diff --git a/net/rds/ib.h b/net/rds/ib.h
index 1901226368c9..d1a3d421d439 100644
--- a/net/rds/ib.h
+++ b/net/rds/ib.h
@@ -258,6 +258,10 @@ struct rds_ib_device {
unsigned int max_initiator_depth;
unsigned int max_responder_resources;
spinlock_t spinlock; /* protect the above */
+ /* set under spinlock by rds_ib_dev_shutdown(): the device is
+ * going away and no connection may attach to it any more
+ */
+ bool shutting_down;
refcount_t refcount;
struct work_struct free_work;
int *vector_load;
@@ -384,7 +388,8 @@ void rds_ib_cm_connect_complete(struct rds_connection *conn,
struct rds_ib_device *rds_ib_get_device(__be32 ipaddr);
int rds_ib_update_ipaddr(struct rds_ib_device *rds_ibdev,
struct in6_addr *ipaddr);
-void rds_ib_add_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *conn);
+int rds_ib_add_conn(struct rds_ib_device *rds_ibdev,
+ struct rds_connection *conn);
void rds_ib_remove_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *conn);
void rds_ib_destroy_nodev_conns(void);
void rds_ib_mr_cqe_handler(struct rds_ib_connection *ic, struct ib_wc *wc);
diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 8b16bd7c40ce..82ecbb9a3da1 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -524,7 +524,9 @@ static int rds_ib_setup_qp(struct rds_connection *conn)
fr_queue_space = RDS_IB_DEFAULT_FR_WR;
/* add the conn now so that connection establishment has the dev */
- rds_ib_add_conn(rds_ibdev, conn);
+ ret = rds_ib_add_conn(rds_ibdev, conn);
+ if (ret)
+ goto out;
max_wrs = rds_ibdev->max_wrs < rds_ib_sysctl_max_send_wr + 1 ?
rds_ibdev->max_wrs - 1 : rds_ib_sysctl_max_send_wr;
diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
index db7e92e7bd29..50c02f47cf68 100644
--- a/net/rds/ib_rdma.c
+++ b/net/rds/ib_rdma.c
@@ -119,7 +119,8 @@ int rds_ib_update_ipaddr(struct rds_ib_device *rds_ibdev,
return 0;
}
-void rds_ib_add_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *conn)
+int rds_ib_add_conn(struct rds_ib_device *rds_ibdev,
+ struct rds_connection *conn)
{
struct rds_ib_connection *ic = conn->c_transport_data;
@@ -127,15 +128,27 @@ void rds_ib_add_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *con
spin_lock_irq(&ib_nodev_conns_lock);
BUG_ON(list_empty(&ib_nodev_conns));
BUG_ON(list_empty(&ic->ib_node));
- list_del(&ic->ib_node);
spin_lock(&rds_ibdev->spinlock);
+ /* rds_ib_dev_shutdown() has walked conn_list, or is about to
+ * with this lock held: a connection attached now would never be
+ * dropped by it, so leave the connection on the nodev list for
+ * the caller to fail and the transport exit to find.
+ */
+ if (rds_ibdev->shutting_down) {
+ spin_unlock(&rds_ibdev->spinlock);
+ spin_unlock_irq(&ib_nodev_conns_lock);
+ return -ENODEV;
+ }
+ list_del(&ic->ib_node);
list_add_tail(&ic->ib_node, &rds_ibdev->conn_list);
spin_unlock(&rds_ibdev->spinlock);
spin_unlock_irq(&ib_nodev_conns_lock);
ic->rds_ibdev = rds_ibdev;
refcount_inc(&rds_ibdev->refcount);
+
+ return 0;
}
void rds_ib_remove_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *conn)
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v8 03/13] net/rds: ib: refuse to attach a connection to a device being removed
2026-10-03 16:32 ` [PATCH net-next v8 03/13] net/rds: ib: refuse to attach a connection to a device being removed Allison Henderson
@ 2026-10-04 16:34 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-10-04 16:34 UTC (permalink / raw)
To: achender; +Cc: netdev, linux-rdma, pabeni, edumazet, kuba, horms
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message gets the trigger wrong.
--
net/rds: ib: refuse to attach a connection to a device being removed
This patch adds a shutting_down flag to struct rds_ib_device.
rds_ib_dev_shutdown() sets it under rds_ibdev->spinlock before walking
conn_list. rds_ib_add_conn() checks the flag under the same lock and
returns -ENODEV, which makes rds_ib_setup_qp() fail the connect.
> rds_ib_remove_one() drops every connection on the device's conn_list
> in rds_ib_dev_shutdown(), then clears the client data so that no new
> connect can find the device. A connect that is already past
> rds_ib_get_client_data() when the walk runs is not covered by either:
> rds_ib_setup_qp() goes on to rds_ib_add_conn(), which moves the
> connection onto the conn_list the walk has just finished with, and
> builds a QP on a device that is on its way out. Nothing drops that
> connection afterwards - the device's shutdown walk is over, and the
> connection never returns to ib_nodev_conns, which is the only list the
> transport exit sweeps - so it outlives the device, and the module.
[Severity: Low]
Does this description hold for a real IB device hot-removal?
disable_device() in drivers/infiniband/core/device.c removes clients in
LIFO order. That means rds_ib_remove_one() runs before rdma_cm's
cma_remove_one().
cma_process_remove() then sends DEVICE_REMOVAL to every cm_id still on the
device, including the late connection's ic->i_cm_id. It then waits for
those ids to be destroyed:
cma_remove_one()
cma_process_remove()
cma_send_device_removal_put(id_priv)
...
rds_rdma_cm_event_handler_cmn()
net/rds/rdma_transport.c:rds_rdma_cm_event_handler_cmn() {
...
case RDMA_CM_EVENT_DEVICE_REMOVAL:
case RDMA_CM_EVENT_ADDR_CHANGE:
if (conn)
rds_conn_drop(conn);
break;
...
}
During a plain hot-removal, rds_destroy_pending() is false, so this
rds_conn_drop() queues the shutdown. The shutdown tears down the QP and
cm_id, and rds_ib_remove_conn() puts the connection back on
ib_nodev_conns. So on hot-removal the connection does not seem to outlive
the device.
The leak described here seems to need the module unload path instead:
rds_ib_exit()
rds_ib_set_unloading()
rds_ib_unregister_client()
rds_ib_remove_one() <- device stays registered, no DEVICE_REMOVAL
rds_ib_destroy_nodev_conns()
On this path rds_destroy_pending() is true, so every non-destroy
rds_conn_drop() returns early. rds_ib_destroy_nodev_conns() only sweeps
ib_nodev_conns. A connection attached after the walk is then leaked.
The code change looks correct for both cases. Could the commit message
name the trigger as unloading rds_rdma while a connect is in flight,
rather than device removal in general? The patch carries a Fixes: tag, so
stable backporters will use this description to judge how the bug
reproduces and what it affects.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003163215.250253-1-achender%40kernel.org
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v8 04/13] net/rds: guard every work-requeueing site with rds_destroy_pending()
2026-10-03 16:32 [PATCH net-next v8 00/13] net/rds: make connection lifetime reference-counted Allison Henderson
` (2 preceding siblings ...)
2026-10-03 16:32 ` [PATCH net-next v8 03/13] net/rds: ib: refuse to attach a connection to a device being removed Allison Henderson
@ 2026-10-03 16:32 ` Allison Henderson
2026-10-04 16:34 ` netdev-bot+sashiko
2026-10-03 16:32 ` [PATCH net-next v8 05/13] net/rds: make rds_destroy_pending() report a connection's own destroy Allison Henderson
` (8 subsequent siblings)
12 siblings, 1 reply; 21+ messages in thread
From: Allison Henderson @ 2026-10-03 16:32 UTC (permalink / raw)
To: netdev, linux-rdma, pabeni, edumazet, kuba, horms; +Cc: achender
rds_conn_destroy() cancels the path works and then destroys the
per-path workqueue. The sites that can re-arm those works are
supposed to test rds_destroy_pending() under rcu_read_lock() first,
paired with the synchronize_rcu() in the destroy path, so that no new
work can be queued once the cancellation has begun.
Five arming sites never got that guard:
- rds_ib_send_cqe_handler() and rds_ib_send_add_credits() re-arm
cp_send_w when a send completion or a credit update clears
RDS_LL_SEND_FULL,
- rds_ib_recv_refill() re-arms cp_recv_w when the recv ring runs
low,
- rds_tcp_accept_one() kicks cp_recv_w on the freshly accepted
socket, and
- rds_sendmsg() arms cp_conn_w for a multipath connection whose
path 0 is not up yet.
A completion landing in the window between the cancel and
destroy_workqueue() in rds_conn_path_destroy() would re-arm a work on
a workqueue that is about to be destroyed: with delay 0 the work is
queued directly on the freed workqueue, and with delay 1 the timer
survives destroy_workqueue() unseen and fires afterwards, queueing
from a timer_list that lives in the freed c_path array. Today's
destroy triggers keep these sites out of that window - an IB
connection reaches rds_conn_destroy() only from the nodev list, with
its QP and CQs already gone, the TCP accept work is flushed before
any connection is destroyed, and a sending socket pins its netns and
its transport module - so this is preparation: the following patch
gives the predicate a per-connection term, and these are the sites
that would otherwise miss it.
Wrap all five sites in the same rcu_read_lock() +
rds_destroy_pending() pattern the other arming sites already use.
The four self-requeues in rds_send_worker() and rds_recv_worker() are
left alone on purpose: they run from inside the work item itself, and
cancel_delayed_work_sync() disables the work for the duration of the
cancel, so a requeue issued by the still-running callback is dropped
and none can follow once the cancel has returned.
With the predicate as it stands the guards cover the netns teardown and
module unload cases; the following patch extends it to the destroy of a
single connection.
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
net/rds/ib_recv.c | 6 +++++-
net/rds/ib_send.c | 18 ++++++++++++++----
net/rds/send.c | 11 ++++++++---
net/rds/tcp_listen.c | 10 +++++++---
4 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index bd6cb3ffaa57..7d45808544a0 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -458,7 +458,11 @@ void rds_ib_recv_refill(struct rds_connection *conn, int prefill, gfp_t gfp)
(must_wake ||
(can_wait && rds_ib_ring_low(&ic->i_recv_ring)) ||
rds_ib_ring_empty(&ic->i_recv_ring))) {
- queue_delayed_work(conn->c_path->cp_wq, &conn->c_recv_w, 1);
+ rcu_read_lock();
+ if (!rds_destroy_pending(conn))
+ queue_delayed_work(conn->c_path->cp_wq,
+ &conn->c_recv_w, 1);
+ rcu_read_unlock();
}
if (can_wait)
cond_resched();
diff --git a/net/rds/ib_send.c b/net/rds/ib_send.c
index d6be95542119..bc411e96ad12 100644
--- a/net/rds/ib_send.c
+++ b/net/rds/ib_send.c
@@ -298,8 +298,13 @@ void rds_ib_send_cqe_handler(struct rds_ib_connection *ic, struct ib_wc *wc)
rds_ib_sub_signaled(ic, nr_sig);
if (test_and_clear_bit(RDS_LL_SEND_FULL, &conn->c_flags) ||
- test_bit(0, &conn->c_map_queued))
- queue_delayed_work(conn->c_path->cp_wq, &conn->c_send_w, 0);
+ test_bit(0, &conn->c_map_queued)) {
+ rcu_read_lock();
+ if (!rds_destroy_pending(conn))
+ queue_delayed_work(conn->c_path->cp_wq,
+ &conn->c_send_w, 0);
+ rcu_read_unlock();
+ }
/* We expect errors as the qp is drained during shutdown */
if (wc->status != IB_WC_SUCCESS && rds_conn_up(conn)) {
@@ -420,8 +425,13 @@ void rds_ib_send_add_credits(struct rds_connection *conn, unsigned int credits)
test_bit(RDS_LL_SEND_FULL, &conn->c_flags) ? ", ll_send_full" : "");
atomic_add(IB_SET_SEND_CREDITS(credits), &ic->i_credits);
- if (test_and_clear_bit(RDS_LL_SEND_FULL, &conn->c_flags))
- queue_delayed_work(conn->c_path->cp_wq, &conn->c_send_w, 0);
+ if (test_and_clear_bit(RDS_LL_SEND_FULL, &conn->c_flags)) {
+ rcu_read_lock();
+ if (!rds_destroy_pending(conn))
+ queue_delayed_work(conn->c_path->cp_wq,
+ &conn->c_send_w, 0);
+ rcu_read_unlock();
+ }
WARN_ON(IB_GET_SEND_CREDITS(credits) >= 16384);
diff --git a/net/rds/send.c b/net/rds/send.c
index 1afa981e5c06..32c411d10e3e 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -1378,9 +1378,14 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
* outstanding.
*/
if (!test_and_set_bit(RDS_RECONNECT_PENDING,
- &conn->c_path[0].cp_flags))
- queue_delayed_work(conn->c_path[0].cp_wq,
- &conn->c_path[0].cp_conn_w, 0);
+ &conn->c_path[0].cp_flags)) {
+ rcu_read_lock();
+ if (!rds_destroy_pending(conn))
+ queue_delayed_work(conn->c_path[0].cp_wq,
+ &conn->c_path[0].cp_conn_w,
+ 0);
+ rcu_read_unlock();
+ }
rds_send_ping(conn, 0);
}
diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index 13fa60c1985b..8a0c54aced5e 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -316,10 +316,14 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
*/
if (READ_ONCE(sk->sk_state) == TCP_CLOSE_WAIT ||
READ_ONCE(sk->sk_state) == TCP_LAST_ACK ||
- READ_ONCE(sk->sk_state) == TCP_CLOSE)
+ READ_ONCE(sk->sk_state) == TCP_CLOSE) {
rds_conn_path_drop(cp, 0);
- else
- queue_delayed_work(cp->cp_wq, &cp->cp_recv_w, 0);
+ } else {
+ rcu_read_lock();
+ if (!rds_destroy_pending(cp->cp_conn))
+ queue_delayed_work(cp->cp_wq, &cp->cp_recv_w, 0);
+ rcu_read_unlock();
+ }
sock_put(sk);
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v8 04/13] net/rds: guard every work-requeueing site with rds_destroy_pending()
2026-10-03 16:32 ` [PATCH net-next v8 04/13] net/rds: guard every work-requeueing site with rds_destroy_pending() Allison Henderson
@ 2026-10-04 16:34 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-10-04 16:34 UTC (permalink / raw)
To: achender; +Cc: netdev, linux-rdma, pabeni, edumazet, kuba, horms
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message of "net/rds: guard every work-requeueing site
with rds_destroy_pending()" has no blank line between the last…
--
net/rds: guard every work-requeueing site with rds_destroy_pending()
This commit adds the rcu_read_lock() + rds_destroy_pending() guard to five
RDS sites that can re-arm path works: the IB send completion and credit
paths, the IB recv refill, the TCP accept path, and the multipath reconnect
in rds_sendmsg(). It prepares for a following patch that adds a
per-connection term to the predicate.
> With the predicate as it stands the guards cover the netns teardown and
> module unload cases; the following patch extends it to the destroy of a
> single connection.
> Assisted-by: Claude-Code:claude-fable-5
> Signed-off-by: Allison Henderson <achender@kernel.org>
[Severity: Low]
This isn't a bug, but there is no blank line between the last paragraph of
the body and the trailers. "single connection." is followed directly by the
Assisted-by: and Signed-off-by: lines.
git interpret-trailers and b4 treat the last paragraph as the trailer block.
In that paragraph, 2 of the 5 lines are trailers, which is over git's 25%
threshold. So the prose lines get counted as part of the trailer block too,
and any tags added later (Reviewed-by:, Link:, Cc: stable) would be appended
to that mixed paragraph.
Could a blank line be added before "Assisted-by:" when this is respun?
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003163215.250253-1-achender%40kernel.org
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v8 05/13] net/rds: make rds_destroy_pending() report a connection's own destroy
2026-10-03 16:32 [PATCH net-next v8 00/13] net/rds: make connection lifetime reference-counted Allison Henderson
` (3 preceding siblings ...)
2026-10-03 16:32 ` [PATCH net-next v8 04/13] net/rds: guard every work-requeueing site with rds_destroy_pending() Allison Henderson
@ 2026-10-03 16:32 ` Allison Henderson
2026-10-03 16:32 ` [PATCH net-next v8 06/13] net/rds: split connection destroy into quiesce and kref-governed free Allison Henderson
` (7 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Allison Henderson @ 2026-10-03 16:32 UTC (permalink / raw)
To: netdev, linux-rdma, pabeni, edumazet, kuba, horms; +Cc: achender
rds_conn_destroy() cancels the path works and then destroys the
per-path workqueue. However, nothing currently stops the
work-requeueing sites from queueing new work on the connection while
that happens. The existing code would suggest that this protection
is supposed to come from rds_destroy_pending(), since all of those
sites - apart from the workers' own self-requeues, which the sync
cancel in the destroy path already rejects, and the destroy == true
rds_conn_path_drop(), which the destroy itself issues and flushes and
IB device removal issues ahead of the module exit that destroys the
connection - guard the queueing with
rds_destroy_pending() under rcu_read_lock() (the last stragglers were
converted by the previous patch), and rds_conn_destroy() already issues a
synchronize_rcu() after unhashing the connection. But the predicate
only tests for the two global teardown cases (netns destruction via
check_net(), module unload via ->t_unloading). Because the conn
itself lacks any indication that a destroy is in progress, the
predicate does not cover the destruction of a single connection
outside these two cases.
One caller escapes both terms. When the core rds module unloads,
rds_conn_exit() runs rds_loop_net_exit() first, and unregistering the
pernet operations invokes rds_loop_exit_net() -> rds_loop_kill_conns()
-> rds_conn_destroy() for the loopback connections of every network
namespace that is still alive: check_net() is true for all of them,
and the loop transport's unloading flag is only set afterwards, by
rds_loop_exit(). For the duration of those destroys the predicate is
false, so a concurrent rds_cong_queue_updates() can still find the
connection on the congestion map's m_conn_list (the conn is only
removed from it after the paths are torn down) and call
queue_delayed_work() on a cp_wq that destroy_workqueue() has already
freed. Nothing can exploit that today: every socket pins the rds
module, so none exists by the time rds_exit() runs, and the works a
connection's own ordered workqueue re-arms are drained by
destroy_workqueue(). But the predicate is wrong for those destroys,
and the following patches need it to be right.
The predicate is also imprecise even where it is true: it answers "is
this connection's world going away", not "is this connection being
destroyed", and the following patches need the second answer. Once
the free is deferred to the last reference, a connection can be
handed to rds_conn_destroy() more than once (the IB unload path
re-sweeps its list until every connection is gone) and must
recognise its own destroy in progress, and the passive-twin creation
must refuse a parent whose destroy has begun.
The cp_flags bit that once served this purpose, RDS_DESTROY_PENDING
from commit c90ecbfaf50d2 ("rds: Use atomic flag to track connections
being destroyed"), was only ever set on the IB protocol-version path
and lost its last set_bit in commit cdc306a5c9cd3 ("rds: make v3.1 as
compat version"); it never covered the loopback path above, which
commit c809195f5523 ("rds: clean up loopback rds_connections on netns
deletion") added.
Record the destroy on the connection itself, where every caller is
covered: set
conn->c_destroy_in_prog before the unhash + synchronize_rcu() sequence
in rds_conn_destroy() and test it first in rds_destroy_pending(). The
existing rcu_read_lock() around every check-and-queue site pairs with
that synchronize_rcu(): once it returns, every new reader observes the
flag and refuses to queue, and anything queued before it is flushed or
cancelled by the existing teardown. Drop the now-unreferenced
RDS_DESTROY_PENDING bit and its dead test.
In the Oracle UEK kernel the equivalent conn->c_destroy_in_prog flag
is part of the larger connection refcounting rework ("net/rds: Add
krefs to struct rds_connection"), including ("net/rds: Merge uses of
conn->c_destroy_in_prog & RDS_DESTROY_PENDING"). This ports the
missing pieces of the requeue guard, which stand on their own.
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 | 9 +++++++++
net/rds/ib.c | 5 +----
net/rds/rds.h | 17 +++++++++++++++--
3 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/net/rds/connection.c b/net/rds/connection.c
index 95ff50f31d1e..97d470242d0f 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -585,6 +585,15 @@ 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 test rds_destroy_pending() under rcu_read_lock(),
+ * bar the exemptions noted at c_destroy_in_prog) 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 a7647ec01a11..d9879b6129e7 100644
--- a/net/rds/ib.c
+++ b/net/rds/ib.c
@@ -528,10 +528,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..5afdf5a8d93f 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,19 @@ struct rds_connection {
c_pad_to_32:29;
int c_npaths;
bool c_with_sport_idx;
+ /* Set once, by rds_conn_destroy(), before it cancels the path
+ * works; read through rds_destroy_pending(). A site that arms
+ * a path work must test the predicate and queue the work inside
+ * one rcu_read_lock() section: the synchronize_rcu() that
+ * follows the store is what keeps a queue issued after the
+ * cancellation from landing on a destroyed workqueue. Two kinds
+ * of site are exempt: the workers' own self-requeues, which the
+ * sync cancel in the destroy path rejects, and the destroy == true
+ * rds_conn_path_drop(): the destroy issues and flushes it itself,
+ * and IB device removal issues it ahead of the module exit, which
+ * destroys - and so flushes - that connection afterwards.
+ */
+ bool c_destroy_in_prog;
struct rds_connection *c_passive;
struct rds_transport *c_trans;
@@ -994,7 +1006,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
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH net-next v8 06/13] net/rds: split connection destroy into quiesce and kref-governed free
2026-10-03 16:32 [PATCH net-next v8 00/13] net/rds: make connection lifetime reference-counted Allison Henderson
` (4 preceding siblings ...)
2026-10-03 16:32 ` [PATCH net-next v8 05/13] net/rds: make rds_destroy_pending() report a connection's own destroy Allison Henderson
@ 2026-10-03 16:32 ` Allison Henderson
2026-10-03 16:32 ` [PATCH net-next v8 07/13] net/rds: unlink transport nodes before a possibly deferred connection free Allison Henderson
` (6 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Allison Henderson @ 2026-10-03 16:32 UTC (permalink / raw)
To: netdev, linux-rdma, pabeni, edumazet, kuba, horms; +Cc: achender
From: Sharath Srinivasan <sharath.srinivasan@oracle.com>
rds_conn_destroy() tears down the transport state and immediately
frees the connection, along with its paths and its workqueues. This
relies on the assumption (documented in the rds_conn_destroy()
comments) that "no one else is referencing the connection", which "we
can only ensure ... in the rmmod path". However, the callers stopped
honoring that long ago. Today, connections are also destroyed on
network namespace teardown (rds_tcp_kill_sock() and
rds_loop_kill_conns()), and the following patches deliberately let a
connection outlive the call.
This leaves loose ends since references to these destroyed
connections still exist. Sockets cache their connections in rs_conn,
and congestion updates will walk the maps' m_conn_list. The CM
callbacks and workers may also still hold the pointer.
Prepare to close those holes by making the connection refcounted:
- kref_init() the connection in __rds_conn_create(); the initial
reference belongs to whoever is responsible for destroying the
connection.
- rds_conn_destroy() still quiesces synchronously exactly as before
(workers cancelled, paths dropped and shut down, queued messages
purged, congestion list removal), but the frees - the transport's
conn_free, the path workqueues, the c_path array and the connection
slab object - move to rds_conn_destroy_fini(), which runs when the
last reference is dropped via rds_conn_put().
- Export rds_conn_get()/rds_conn_put(), plus an inline
rds_conn_get_unless_zero() for holders that may find a connection
already on its way out, for the reference holders introduced in the
following patches.
rds_conn_destroy() also gains a first-caller-wins guard on the new
c_destroy_in_prog flag. No caller in this tree hands a connection to
rds_conn_destroy() twice - every teardown sweep unlinks or claims its
node before the destroy - so the guard documents the contract rather
than closing a path: a second caller returns at once, without
quiescing anything and without waiting for the first. With the
initial reference the only one, it is inert here either way.
With no additional reference holders yet, this only sets up the
refcounting framework and is functionally equivalent to the current
code (the initial reference is the only one), so every free still
completes inside rds_conn_destroy(). The next two patches make a
deferred free safe - the transport unload wait and the unlinking of
the transport nodes ahead of the destroy - and only then do the
patches that follow take references at the places that today rely on
bare pointers.
Based on the Oracle UEK commit "net/rds: Add krefs to struct
rds_connection".
Signed-off-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>
[achender: substantial reimplementation for net-next: UEK's
rds_conn_destroy_init()/_fini() split redone against upstream's
rds_conn_destroy()/rds_conn_path_destroy() (no heartbeat/reap/trace
infrastructure, no rds_net, single conn hash); destroy keeps its
one-call external interface; holder coverage split out into follow-up
patches; rewrite commit message]
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
net/rds/connection.c | 99 ++++++++++++++++++++++++++++++++++++--------
net/rds/rds.h | 19 ++++++++-
2 files changed, 98 insertions(+), 20 deletions(-)
diff --git a/net/rds/connection.c b/net/rds/connection.c
index 97d470242d0f..258a6bc3c573 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -231,6 +231,7 @@ static struct rds_connection *__rds_conn_create(struct net *net,
goto out;
}
+ kref_init(&conn->c_refcount);
INIT_HLIST_NODE(&conn->c_hash_node);
conn->c_laddr = *laddr;
conn->c_isv6 = !ipv6_addr_v4mapped(laddr);
@@ -477,9 +478,10 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
* Quiesce the reconnect timer before bailing
* out, though. When a pending destroy did
* suppress the queue, no later pass runs, and
- * rds_conn_path_destroy() is about to flush
- * cp_down_w and free the path: it must not
- * find cp_conn_w still armed. A successor
+ * rds_conn_path_quiesce() is about to flush
+ * cp_down_w, ahead of the path's deferred
+ * free: it must not find cp_conn_w still
+ * armed. A successor
* pass, when there is one, re-arms the
* reconnect from its own tail.
*/
@@ -526,10 +528,12 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
conn->c_trans->conn_slots_available(conn, false);
}
-/* destroy a single rds_conn_path. rds_conn_destroy() iterates over
- * all paths using rds_conn_path_destroy()
+/* quiesce a single rds_conn_path: shut it down and tear down any
+ * queued messages. rds_conn_destroy() iterates over all paths using
+ * rds_conn_path_quiesce(); the transport state and the workqueue are
+ * freed later, from rds_conn_path_free().
*/
-static void rds_conn_path_destroy(struct rds_conn_path *cp)
+static void rds_conn_path_quiesce(struct rds_conn_path *cp)
{
struct rds_message *rm, *rtmp;
@@ -558,6 +562,16 @@ static void rds_conn_path_destroy(struct rds_conn_path *cp)
WARN_ON(delayed_work_pending(&cp->cp_recv_w));
WARN_ON(delayed_work_pending(&cp->cp_conn_w));
WARN_ON(work_pending(&cp->cp_down_w));
+}
+
+/* free a quiesced rds_conn_path's transport state and workqueue; runs
+ * from rds_conn_destroy_fini() once the last connection reference is
+ * dropped.
+ */
+static void rds_conn_path_free(struct rds_conn_path *cp)
+{
+ if (!cp->cp_transport_data)
+ return;
if (cp->cp_wq != rds_wq) {
destroy_workqueue(cp->cp_wq);
@@ -567,16 +581,55 @@ static void rds_conn_path_destroy(struct rds_conn_path *cp)
cp->cp_conn->c_trans->conn_free(cp->cp_transport_data);
}
+/* Free a connection. This runs from rds_conn_put() when the last
+ * reference is dropped, after rds_conn_destroy() has quiesced the
+ * connection and dropped the initial reference.
+ */
+static void rds_conn_destroy_fini(struct kref *kref)
+{
+ struct rds_connection *conn = container_of(kref, struct rds_connection,
+ c_refcount);
+ int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
+ unsigned long flags;
+ int i;
+
+ for (i = 0; i < npaths; i++)
+ rds_conn_path_free(&conn->c_path[i]);
+
+ kfree(conn->c_path);
+ kmem_cache_free(rds_conn_slab, conn);
+
+ spin_lock_irqsave(&rds_conn_lock, flags);
+ rds_conn_count--;
+ spin_unlock_irqrestore(&rds_conn_lock, flags);
+}
+
+void rds_conn_get(struct rds_connection *conn)
+{
+ kref_get(&conn->c_refcount);
+}
+EXPORT_SYMBOL_GPL(rds_conn_get);
+
+void rds_conn_put(struct rds_connection *conn)
+{
+ kref_put(&conn->c_refcount, rds_conn_destroy_fini);
+}
+EXPORT_SYMBOL_GPL(rds_conn_put);
+
/*
* Stop and free a connection.
*
- * This can only be used in very limited circumstances. It assumes that once
- * the conn has been shutdown that no one else is referencing the connection.
- * We can only ensure this in the rmmod path in the current code.
+ * Quiesces the connection synchronously (workers cancelled, transport
+ * connections shut down, queued messages dropped) and drops the
+ * initial reference. Only the first call for a connection does any
+ * of that: a later one finds c_destroy_in_prog already set and
+ * returns at once, without waiting for the first. The memory -
+ * including the transport's per-connection state and the path
+ * workqueues - is freed once the last rds_conn_put() runs, which may
+ * be after this returns.
*/
void rds_conn_destroy(struct rds_connection *conn)
{
- unsigned long flags;
int i;
struct rds_conn_path *cp;
int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
@@ -591,11 +644,23 @@ void rds_conn_destroy(struct rds_connection *conn)
* bar the exemptions noted at c_destroy_in_prog) from queueing
* new work on the path workqueues once we start cancelling and
* destroying them.
+ *
+ * Only the first caller proceeds. No caller in this tree hands
+ * a connection to rds_conn_destroy() twice - every teardown
+ * sweep unlinks or claims its node before the destroy - so this
+ * documents the contract rather than closing a path: a second
+ * caller returns at once, without quiescing or waiting. 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();
@@ -603,7 +668,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));
}
@@ -614,12 +679,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 5afdf5a8d93f..be882269a4d0 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -137,6 +137,12 @@ struct rds_conn_path {
/* One rds_connection per RDS address pair */
struct rds_connection {
struct hlist_node c_hash_node;
+ /* rds_conn_destroy() quiesces the connection synchronously;
+ * freeing it - the connection memory, the path workqueues and
+ * the transport's per-connection state - is deferred until the
+ * last reference is dropped via rds_conn_put().
+ */
+ struct kref c_refcount;
struct in6_addr c_laddr;
struct in6_addr c_faddr;
int c_dev_if; /* ifindex used for this conn */
@@ -147,8 +153,10 @@ struct rds_connection {
c_pad_to_32:29;
int c_npaths;
bool c_with_sport_idx;
- /* Set once, by rds_conn_destroy(), before it cancels the path
- * works; read through rds_destroy_pending(). A site that arms
+ /* Set once, by rds_conn_destroy() under rds_conn_lock - a
+ * test-and-set, so a second destroy of the same connection
+ * returns at once - before it cancels the path works. Read
+ * through rds_destroy_pending(). A site that arms
* a path work must test the predicate and queue the work inside
* one rcu_read_lock() section: the synchronize_rcu() that
* follows the store is what keeps a queue issued after the
@@ -831,6 +839,13 @@ struct rds_connection *rds_conn_create_outgoing(struct net *net,
u8 tos, gfp_t gfp, int dev_if);
void rds_conn_shutdown(struct rds_conn_path *cpath);
void rds_conn_destroy(struct rds_connection *conn);
+void rds_conn_get(struct rds_connection *conn);
+void rds_conn_put(struct rds_connection *conn);
+/* take a reference unless the connection is already being freed */
+static inline bool rds_conn_get_unless_zero(struct rds_connection *conn)
+{
+ return kref_get_unless_zero(&conn->c_refcount);
+}
void rds_conn_drop(struct rds_connection *conn);
void rds_conn_path_drop(struct rds_conn_path *cpath, bool destroy);
void rds_conn_connect_if_down(struct rds_connection *conn);
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH net-next v8 07/13] net/rds: unlink transport nodes before a possibly deferred connection free
2026-10-03 16:32 [PATCH net-next v8 00/13] net/rds: make connection lifetime reference-counted Allison Henderson
` (5 preceding siblings ...)
2026-10-03 16:32 ` [PATCH net-next v8 06/13] net/rds: split connection destroy into quiesce and kref-governed free Allison Henderson
@ 2026-10-03 16:32 ` Allison Henderson
2026-10-04 16:35 ` netdev-bot+sashiko
2026-10-03 16:32 ` [PATCH net-next v8 08/13] net/rds: wait for connections to be freed on transport unload Allison Henderson
` (5 subsequent siblings)
12 siblings, 1 reply; 21+ messages in thread
From: Allison Henderson @ 2026-10-03 16:32 UTC (permalink / raw)
To: netdev, linux-rdma, pabeni, edumazet, kuba, horms; +Cc: achender
The transport teardown helpers - rds_tcp_destroy_conns(),
rds_tcp_kill_sock(), rds_ib_destroy_nodev_conns(), rds_loop_exit() and
rds_loop_kill_conns() - gather the per-connection transport nodes onto
a list head on their own stack and call rds_conn_destroy() for each.
The node is unlinked much later, by the transport's conn_free():
rds_tcp_conn_free() and rds_loop_conn_free() list_del() it, and
rds_ib_conn_free() does so unconditionally.
That is fine for as long as rds_conn_destroy() frees the connection
before it returns, which is still the case at this point in the
series: the initial reference is the only one. The following patches
hand out references that outlive the teardown loop - a socket's
cached rs_conn, an inc parked on a receive queue - and with those, a
conn_free() deferred until after the helper has returned would
list_del() the node from a stack frame that no longer exists. Make
the helpers ready for that first.
Unlink each node right before its rds_conn_destroy() - under the
transport lock for TCP and loopback, and off the claimed stack list
for IB - so that nothing is left on the stack list for a later free
to touch. TCP marks the node detached, as
rds_tcp_kill_sock() already does for the secondary paths of a
multipath connection; loopback uses list_del_init() and has its
conn_free() skip a node that is already empty.
IB needs an explicit flag, i_ib_node_detached, because its node has
other movers: a connect worker moves it from the nodev list to a
device's list in rds_ib_add_conn(), and a shutdown moves it back in
rds_ib_remove_conn(). Either can run while the sweep holds the node
on its stack list, and "the node is linked" cannot tell that list
from the nodev list - an add_conn() that went by list emptiness would
unlink the node from under the sweep's lockless walk. So the sweep
sets the flag when it gathers the node, under ib_nodev_conns_lock,
and from then on add_conn(), remove_conn() and conn_free() leave the
node alone; the node belongs to the sweep, and its walk needs no
lock. Those movers used to assert that the node is linked (and
rds_ib_add_conn() that the nodev list is non-empty); a connect or
shutdown worker can still be running for a connection the sweep has
claimed, and such a connection is about to be destroyed anyway, so
the assertions go.
The walk itself must not lose the entries either. Once the following
patches hand out references, a connection on one of these lists can
be kept alive only by a reference that is dropped at any point during
the walk, freeing the transport node the iterator is about to read;
nothing in this tree destroys a connection behind the sweep's back
today, so this is preparation for those patches rather than a fix.
So the gather takes a reference on each connection it moves
onto the stack list, under the transport lock, and drops it after the
destroy; a connection whose free is already running gets no reference
and is left where it is, since that free unlinks the node itself once
the lock is released. The tmp_list gathering itself remains: it is
what keeps rds_conn_destroy() from being called with the transport
lock held.
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
net/rds/ib.h | 4 +++
net/rds/ib_cm.c | 13 ++++++---
net/rds/ib_rdma.c | 70 +++++++++++++++++++++++++++++++++++------------
net/rds/loop.c | 59 +++++++++++++++++++++++++++++++--------
net/rds/tcp.c | 52 +++++++++++++++++++++++++++++++----
5 files changed, 159 insertions(+), 39 deletions(-)
diff --git a/net/rds/ib.h b/net/rds/ib.h
index d1a3d421d439..07474fe7327b 100644
--- a/net/rds/ib.h
+++ b/net/rds/ib.h
@@ -143,6 +143,10 @@ struct rds_ib_device;
struct rds_ib_connection {
struct list_head ib_node;
+ /* set under ib_nodev_conns_lock once a transport teardown has
+ * claimed ib_node; from then on only the teardown touches it
+ */
+ bool i_ib_node_detached;
struct rds_ib_device *rds_ibdev;
struct rds_connection *conn;
diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 82ecbb9a3da1..7f3b78b1219f 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -1291,9 +1291,12 @@ void rds_ib_conn_free(void *arg)
rdsdebug("ic %p\n", ic);
/*
- * Conn is either on a dev's list or on the nodev list.
- * A race with shutdown() or connect() would cause problems
- * (since rds_ibdev would change) but that should never happen.
+ * Conn is on a dev's list or on the nodev list - or, once a
+ * transport teardown has claimed it (i_ib_node_detached), on
+ * neither, in which case the lock chosen here only guards the
+ * test below. A connect or shutdown still running for a
+ * claimed conn leaves the node alone, see rds_ib_add_conn() and
+ * rds_ib_remove_conn().
*
* Callers may hold rds_conn_lock with interrupts disabled
* (__rds_conn_create() undoing a lost creation race), so do not
@@ -1302,7 +1305,9 @@ void rds_ib_conn_free(void *arg)
lock_ptr = ic->rds_ibdev ? &ic->rds_ibdev->spinlock : &ib_nodev_conns_lock;
spin_lock_irqsave(lock_ptr, flags);
- list_del(&ic->ib_node);
+ /* a transport teardown that gathered us first owns the node */
+ if (!ic->i_ib_node_detached)
+ list_del(&ic->ib_node);
spin_unlock_irqrestore(lock_ptr, flags);
rds_ib_recv_free_caches(ic);
diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
index 50c02f47cf68..34e09525a10b 100644
--- a/net/rds/ib_rdma.c
+++ b/net/rds/ib_rdma.c
@@ -123,12 +123,13 @@ int rds_ib_add_conn(struct rds_ib_device *rds_ibdev,
struct rds_connection *conn)
{
struct rds_ib_connection *ic = conn->c_transport_data;
+ int ret = 0;
- /* conn was previously on the nodev_conns_list */
+ /* conn was previously on the nodev_conns_list, unless a teardown
+ * sweep has claimed it ahead of destroying it: then it is on its
+ * way out, and its node belongs to the sweep.
+ */
spin_lock_irq(&ib_nodev_conns_lock);
- BUG_ON(list_empty(&ib_nodev_conns));
- BUG_ON(list_empty(&ic->ib_node));
-
spin_lock(&rds_ibdev->spinlock);
/* rds_ib_dev_shutdown() has walked conn_list, or is about to
* with this lock held: a connection attached now would never be
@@ -136,14 +137,15 @@ int rds_ib_add_conn(struct rds_ib_device *rds_ibdev,
* the caller to fail and the transport exit to find.
*/
if (rds_ibdev->shutting_down) {
- spin_unlock(&rds_ibdev->spinlock);
- spin_unlock_irq(&ib_nodev_conns_lock);
- return -ENODEV;
+ ret = -ENODEV;
+ } else if (!ic->i_ib_node_detached) {
+ list_del(&ic->ib_node);
+ list_add_tail(&ic->ib_node, &rds_ibdev->conn_list);
}
- list_del(&ic->ib_node);
- list_add_tail(&ic->ib_node, &rds_ibdev->conn_list);
spin_unlock(&rds_ibdev->spinlock);
spin_unlock_irq(&ib_nodev_conns_lock);
+ if (ret)
+ return ret;
ic->rds_ibdev = rds_ibdev;
refcount_inc(&rds_ibdev->refcount);
@@ -155,15 +157,22 @@ void rds_ib_remove_conn(struct rds_ib_device *rds_ibdev, struct rds_connection *
{
struct rds_ib_connection *ic = conn->c_transport_data;
- /* place conn on nodev_conns_list */
+ bool detached;
+
+ /* place conn on nodev_conns_list - unless a teardown sweep has
+ * claimed it ahead of destroying it, in which case its node
+ * belongs to the sweep
+ */
spin_lock(&ib_nodev_conns_lock);
spin_lock_irq(&rds_ibdev->spinlock);
- BUG_ON(list_empty(&ic->ib_node));
- list_del(&ic->ib_node);
+ detached = ic->i_ib_node_detached;
+ if (!detached)
+ list_del(&ic->ib_node);
spin_unlock_irq(&rds_ibdev->spinlock);
- list_add_tail(&ic->ib_node, &ib_nodev_conns);
+ if (!detached)
+ list_add_tail(&ic->ib_node, &ib_nodev_conns);
spin_unlock(&ib_nodev_conns_lock);
@@ -176,13 +185,40 @@ void rds_ib_destroy_nodev_conns(void)
struct rds_ib_connection *ic, *_ic;
LIST_HEAD(tmp_list);
- /* avoid calling conn_destroy with irqs off */
+ struct rds_connection *conn;
+
+ /* Gather the connections and take a reference on each, so that
+ * none is freed under the walk below once the deferred frees
+ * introduced later in the series can drop a connection's last
+ * reference behind this sweep. One whose free
+ * is already running gets no reference: its free unlinks the
+ * node itself, under this lock, once we drop it. Marking the
+ * node detached claims it for this sweep: rds_ib_add_conn(),
+ * rds_ib_remove_conn() and rds_ib_conn_free() leave a claimed
+ * node alone, so the walk over tmp_list below needs no lock.
+ * Avoid calling conn_destroy with irqs off.
+ */
spin_lock_irq(&ib_nodev_conns_lock);
- list_splice(&ib_nodev_conns, &tmp_list);
+ list_for_each_entry_safe(ic, _ic, &ib_nodev_conns, ib_node) {
+ if (rds_conn_get_unless_zero(ic->conn)) {
+ ic->i_ib_node_detached = true;
+ list_move_tail(&ic->ib_node, &tmp_list);
+ }
+ }
spin_unlock_irq(&ib_nodev_conns_lock);
- list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node)
- rds_conn_destroy(ic->conn);
+ /* rds_conn_destroy() can return before the connection is freed,
+ * and it is the free - rds_ib_conn_free() - that would unlink
+ * ib_node. tmp_list lives on this stack frame, so take each node
+ * off it before its destroy; the free then leaves it alone.
+ */
+ list_for_each_entry_safe(ic, _ic, &tmp_list, ib_node) {
+ conn = ic->conn;
+ list_del_init(&ic->ib_node);
+
+ rds_conn_destroy(conn);
+ rds_conn_put(conn);
+ }
}
void rds_ib_get_mr_info(struct rds_ib_device *rds_ibdev, struct rds_info_rdma_connection *iinfo)
diff --git a/net/rds/loop.c b/net/rds/loop.c
index e6b0750bbeda..3d063a23bd0b 100644
--- a/net/rds/loop.c
+++ b/net/rds/loop.c
@@ -156,6 +156,45 @@ static int rds_loop_conn_alloc(struct rds_connection *conn, gfp_t gfp)
return 0;
}
+/* Destroy the connections whose nodes were gathered on @tmp_list.
+ *
+ * rds_conn_destroy() can return before the connection is freed, and
+ * it is the free - rds_loop_conn_free() - that unlinks loop_node.
+ * @tmp_list lives on the caller's stack, so unlink each node before
+ * its destroy; the free then finds it empty and leaves it alone.
+ */
+static void rds_loop_destroy_gathered_conns(struct list_head *tmp_list)
+{
+ struct rds_loop_connection *lc, *_lc;
+ struct rds_connection *conn;
+
+ list_for_each_entry_safe(lc, _lc, tmp_list, loop_node) {
+ conn = lc->conn;
+ WARN_ON(conn->c_passive);
+
+ spin_lock_irq(&loop_conns_lock);
+ list_del_init(&lc->loop_node);
+ spin_unlock_irq(&loop_conns_lock);
+
+ rds_conn_destroy(conn);
+ rds_conn_put(conn);
+ }
+}
+
+/* Gather @lc's connection for destruction: move the node to the
+ * caller's @tmp_list and take a reference that keeps the connection,
+ * and so the node, alive until rds_loop_destroy_gathered_conns() has
+ * dealt with it. Called with loop_conns_lock held. A connection
+ * whose free is already running gets no reference; its free unlinks
+ * the node itself, under the same lock, once we drop it.
+ */
+static void rds_loop_gather_conn(struct rds_loop_connection *lc,
+ struct list_head *tmp_list)
+{
+ if (rds_conn_get_unless_zero(lc->conn))
+ list_move_tail(&lc->loop_node, tmp_list);
+}
+
static void rds_loop_conn_free(void *arg)
{
struct rds_loop_connection *lc = arg;
@@ -163,7 +202,9 @@ static void rds_loop_conn_free(void *arg)
rdsdebug("lc %p\n", lc);
spin_lock_irqsave(&loop_conns_lock, flags);
- list_del(&lc->loop_node);
+ /* already unlinked if a transport teardown gathered us first */
+ if (!list_empty(&lc->loop_node))
+ list_del(&lc->loop_node);
spin_unlock_irqrestore(&loop_conns_lock, flags);
kfree(lc);
}
@@ -187,14 +228,11 @@ void rds_loop_exit(void)
synchronize_rcu();
/* avoid calling conn_destroy with irqs off */
spin_lock_irq(&loop_conns_lock);
- list_splice(&loop_conns, &tmp_list);
- INIT_LIST_HEAD(&loop_conns);
+ list_for_each_entry_safe(lc, _lc, &loop_conns, loop_node)
+ rds_loop_gather_conn(lc, &tmp_list);
spin_unlock_irq(&loop_conns_lock);
- list_for_each_entry_safe(lc, _lc, &tmp_list, loop_node) {
- WARN_ON(lc->conn->c_passive);
- rds_conn_destroy(lc->conn);
- }
+ rds_loop_destroy_gathered_conns(&tmp_list);
}
static void rds_loop_kill_conns(struct net *net)
@@ -208,14 +246,11 @@ static void rds_loop_kill_conns(struct net *net)
if (net != c_net)
continue;
- list_move_tail(&lc->loop_node, &tmp_list);
+ rds_loop_gather_conn(lc, &tmp_list);
}
spin_unlock_irq(&loop_conns_lock);
- list_for_each_entry_safe(lc, _lc, &tmp_list, loop_node) {
- WARN_ON(lc->conn->c_passive);
- rds_conn_destroy(lc->conn);
- }
+ rds_loop_destroy_gathered_conns(&tmp_list);
}
static void __net_exit rds_loop_exit_net(struct net *net)
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 774a71f88d37..8df4a7d80048 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -502,6 +502,48 @@ static bool rds_tcp_is_unloading(struct rds_connection *conn)
return atomic_read(&rds_tcp_unloading) != 0;
}
+/* Gather @tc's connection for destruction: move the node to the
+ * caller's @tmp_list and take a reference that keeps the connection,
+ * and so the node, alive until rds_tcp_destroy_gathered_conns() has
+ * dealt with it. Called with rds_tcp_conn_lock held. A connection
+ * whose free is already running gets no reference; its free unlinks
+ * the node itself, under the same lock, once we drop it.
+ */
+static void rds_tcp_gather_conn(struct rds_tcp_connection *tc,
+ struct list_head *tmp_list)
+{
+ if (rds_conn_get_unless_zero(tc->t_cpath->cp_conn))
+ list_move_tail(&tc->t_tcp_node, tmp_list);
+}
+
+/* Destroy the connections whose nodes were gathered on @tmp_list.
+ *
+ * rds_conn_destroy() can return before the connection is freed, and
+ * it is the free - rds_tcp_conn_free() - that unlinks t_tcp_node.
+ * Since @tmp_list lives on the caller's stack, unlink each node here
+ * and mark it detached before its destroy, so that a free that runs
+ * after the caller has returned does not write into a dead frame.
+ * Every entry holds a reference taken by rds_tcp_gather_conn(), so
+ * none can be freed under the walk; each is dropped after its destroy.
+ */
+static void rds_tcp_destroy_gathered_conns(struct list_head *tmp_list)
+{
+ struct rds_tcp_connection *tc, *_tc;
+ struct rds_connection *conn;
+
+ list_for_each_entry_safe(tc, _tc, tmp_list, t_tcp_node) {
+ conn = tc->t_cpath->cp_conn;
+
+ spin_lock_irq(&rds_tcp_conn_lock);
+ list_del_init(&tc->t_tcp_node);
+ tc->t_tcp_node_detached = true;
+ spin_unlock_irq(&rds_tcp_conn_lock);
+
+ rds_conn_destroy(conn);
+ rds_conn_put(conn);
+ }
+}
+
static void rds_tcp_destroy_conns(void)
{
struct rds_tcp_connection *tc, *_tc;
@@ -511,12 +553,11 @@ static void rds_tcp_destroy_conns(void)
spin_lock_irq(&rds_tcp_conn_lock);
list_for_each_entry_safe(tc, _tc, &rds_tcp_conn_list, t_tcp_node) {
if (!list_has_conn(&tmp_list, tc->t_cpath->cp_conn))
- list_move_tail(&tc->t_tcp_node, &tmp_list);
+ rds_tcp_gather_conn(tc, &tmp_list);
}
spin_unlock_irq(&rds_tcp_conn_lock);
- list_for_each_entry_safe(tc, _tc, &tmp_list, t_tcp_node)
- rds_conn_destroy(tc->t_cpath->cp_conn);
+ rds_tcp_destroy_gathered_conns(&tmp_list);
}
static void rds_tcp_exit(void);
@@ -691,15 +732,14 @@ static void rds_tcp_kill_sock(struct net *net)
if (net != c_net)
continue;
if (!list_has_conn(&tmp_list, tc->t_cpath->cp_conn)) {
- list_move_tail(&tc->t_tcp_node, &tmp_list);
+ rds_tcp_gather_conn(tc, &tmp_list);
} else {
list_del(&tc->t_tcp_node);
tc->t_tcp_node_detached = true;
}
}
spin_unlock_irq(&rds_tcp_conn_lock);
- list_for_each_entry_safe(tc, _tc, &tmp_list, t_tcp_node)
- rds_conn_destroy(tc->t_cpath->cp_conn);
+ rds_tcp_destroy_gathered_conns(&tmp_list);
}
static void __net_exit rds_tcp_exit_net(struct net *net)
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v8 07/13] net/rds: unlink transport nodes before a possibly deferred connection free
2026-10-03 16:32 ` [PATCH net-next v8 07/13] net/rds: unlink transport nodes before a possibly deferred connection free Allison Henderson
@ 2026-10-04 16:35 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-10-04 16:35 UTC (permalink / raw)
To: achender; +Cc: netdev, linux-rdma, pabeni, edumazet, kuba, horms
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message says the whole change is groundwork: the old
helpers were "fine for as long as rds_conn_destroy() frees the…
- [Low] After this patch, rds_ib_add_conn() (net/rds/ib_rdma.c) has a
third outcome.
--
net/rds: unlink transport nodes before a possibly deferred connection free
This patch changes the RDS TCP, loopback and IB teardown helpers. Each
helper now takes a reference on every connection it gathers and unlinks
the transport node from the on-stack list before calling
rds_conn_destroy(), so a deferred conn_free() later in the series does not
touch a dead stack frame. IB also gains an i_ib_node_detached flag, which
makes rds_ib_add_conn(), rds_ib_remove_conn() and rds_ib_conn_free() leave
a node alone once the sweep has claimed it.
> diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
> index 50c02f47cf683..34e09525a10b2 100644
> --- a/net/rds/ib_rdma.c
> +++ b/net/rds/ib_rdma.c
[ ... ]
> @@ -136,14 +137,15 @@ int rds_ib_add_conn(struct rds_ib_device *rds_ibdev,
> * the caller to fail and the transport exit to find.
> */
> if (rds_ibdev->shutting_down) {
> - spin_unlock(&rds_ibdev->spinlock);
> - spin_unlock_irq(&ib_nodev_conns_lock);
> - return -ENODEV;
> + ret = -ENODEV;
> + } else if (!ic->i_ib_node_detached) {
> + list_del(&ic->ib_node);
> + list_add_tail(&ic->ib_node, &rds_ibdev->conn_list);
> }
> - list_del(&ic->ib_node);
> - list_add_tail(&ic->ib_node, &rds_ibdev->conn_list);
> spin_unlock(&rds_ibdev->spinlock);
> spin_unlock_irq(&ib_nodev_conns_lock);
> + if (ret)
> + return ret;
>
> ic->rds_ibdev = rds_ibdev;
> refcount_inc(&rds_ibdev->refcount);
[Severity: Low]
Take the case where the device is not shutting_down but the sweep has
already set i_ib_node_detached. Here ret stays 0 and the node is not
moved onto rds_ibdev->conn_list. The function still sets ic->rds_ibdev,
takes a device reference and returns success.
Does that third outcome fit the promise made in rds_ib_dev_shutdown()?
/* Close the device to new connections under the same lock that
* rds_ib_add_conn() attaches them under, so that every
* connection is either dropped by the walk below or refused.
*/
As far as I can tell, this branch can't be reached at the moment.
rds_ib_destroy_nodev_conns() only runs from rds_ib_exit(), after
rds_ib_unregister_client(). That call runs
rds_ib_remove_one()->rds_ib_dev_shutdown() for every device, which sets
shutting_down under rds_ibdev->spinlock. rds_ib_add_conn() tests
shutting_down first, under the same lock, so a claimed connection always
gets -ENODEV.
Would it be more robust to return -ENODEV for the detached case as well?
The commit message already says "such a connection is about to be
destroyed anyway", and that change would make the invariant hold by
construction.
The existing comment above, "leave the connection on the nodev list for
the caller to fail and the transport exit to find", is also slightly
inexact for a claimed node. Such a node is on the sweep's stack list or
already unlinked.
[ ... ]
> @@ -176,13 +185,40 @@ void rds_ib_destroy_nodev_conns(void)
> struct rds_ib_connection *ic, *_ic;
> LIST_HEAD(tmp_list);
>
> - /* avoid calling conn_destroy with irqs off */
> + struct rds_connection *conn;
> +
[ ... ]
> spin_lock_irq(&ib_nodev_conns_lock);
> - list_splice(&ib_nodev_conns, &tmp_list);
> + list_for_each_entry_safe(ic, _ic, &ib_nodev_conns, ib_node) {
> + if (rds_conn_get_unless_zero(ic->conn)) {
> + ic->i_ib_node_detached = true;
> + list_move_tail(&ic->ib_node, &tmp_list);
> + }
> + }
> spin_unlock_irq(&ib_nodev_conns_lock);
[Severity: Low]
The commit message describes the whole change as groundwork:
That is fine for as long as rds_conn_destroy() frees the connection
before it returns
and:
nothing in this tree destroys a connection behind the sweep's back
today, so this is preparation for those patches rather than a fix.
For IB, does this hunk also fix a race that exists before this patch?
The old list_splice() never re-initialised ib_nodev_conns. Loopback, by
contrast, follows its splice with INIT_LIST_HEAD(&loop_conns). After the
splice, ib_nodev_conns.next and .prev still point at nodes that now sit
on the stack tmp_list.
In rds_ib_exit(), rds_ib_unregister_client() leads to
rds_ib_dev_shutdown()->rds_conn_path_drop(cp, true), which queues the
shutdown on the per-path cp_wq. The flush_workqueue(rds_wq) in
rds_ib_unregister_client() doesn't flush cp_wq, so this path can overlap
the sweep:
rds_ib_conn_path_shutdown()
rds_ib_remove_conn()
list_add_tail(&ic->ib_node, &ib_nodev_conns);
Because it goes through the stale head, that insert links the node after
the last entry of tmp_list. That entry's ->next then points to
&ib_nodev_conns rather than &tmp_list.
The lockless walk in rds_ib_destroy_nodev_conns() would then treat the
global head as an rds_ib_connection and pass garbage to
rds_conn_destroy().
In the same way, a connect worker's rds_ib_add_conn() could list_del() a
spliced node off tmp_list during the unlocked walk. The commit message
mentions this overlap itself ("Either can run while the sweep holds the
node on its stack list").
Both cases are closed by the per-entry list_move_tail() under
ib_nodev_conns_lock together with the i_ib_node_detached claim.
Could the commit message say that the IB part fixes an existing list
corruption race on module unload? It may also be worth adding a Fixes:
tag so stable backporters can find it.
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003163215.250253-1-achender%40kernel.org
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v8 08/13] net/rds: wait for connections to be freed on transport unload
2026-10-03 16:32 [PATCH net-next v8 00/13] net/rds: make connection lifetime reference-counted Allison Henderson
` (6 preceding siblings ...)
2026-10-03 16:32 ` [PATCH net-next v8 07/13] net/rds: unlink transport nodes before a possibly deferred connection free Allison Henderson
@ 2026-10-03 16:32 ` Allison Henderson
2026-10-04 16:35 ` netdev-bot+sashiko
2026-10-03 16:32 ` [PATCH net-next v8 09/13] net/rds: hold a connection reference from struct rds_incoming Allison Henderson
` (4 subsequent siblings)
12 siblings, 1 reply; 21+ messages in thread
From: Allison Henderson @ 2026-10-03 16:32 UTC (permalink / raw)
To: netdev, linux-rdma, pabeni, edumazet, kuba, horms; +Cc: achender
From: Sharath Srinivasan <sharath.srinivasan@oracle.com>
Since connection free became asynchronous, rds_conn_destroy() only
quiesces the connection; the actual free - including the transport's
conn_free, which lives in the transport module - runs when the last
reference is dropped. The transports' exit paths destroy all of
their connections and then proceed to unload, so a free that is still
pending - once the following patches hand out references to lookups,
sockets and incs - would execute transport module code after that
module's text is gone. At this point in the series the initial
reference is the only one and the wait returns at once; the guarantee
becomes load-bearing with those patches.
Count each transport's live connections in t_conn_count (incremented
when a connection is published in __rds_conn_create(), decremented as
the last step of rds_conn_destroy_fini()) and make the transport exit
paths - rds_ib_exit(), rds_tcp_exit() and rds_loop_exit() - wait for
the count to drop to zero after destroying their connections.
A bound socket holds a module reference on its own transport
(rds_trans_get_preferred()), but that does not bound the wait: once
the following patches make incs hold a connection reference, an
unread datagram pins its connection for as long as the application
leaves it on the receive queue, and rds_find_bound() does not filter
on transport, so that socket may well belong to a different transport
than the connection and pin nothing that stops this unload.
Proceeding after a timeout would turn a leak into a use-after-free in
this module's text, so the wait is unbounded: it polls the count,
warns every ten seconds naming the transport and the number of
connections outstanding, and returns only when the count reaches
zero. rmmod therefore blocks while data is queued and unread, which
is the historical RDS contract - teardown does not discard queued
data.
The wait is deliberately not interruptible: the exit function has
already passed the point of no return, and a killed rmmod would leave
the module half torn down and unloadable, which is worse than a
blocked one. The polling wakes every 100 ms, so the hung task detector
does not fire.
The wake at the end of rds_conn_destroy_fini() can run from a thread
executing transport module text, but never as that thread's last use
of it: the CM event handler's reference (added later in the series) is
always dropped before the rdma_destroy_id() in the connection's own
shutdown returns, and rds_rdma_exit() stops the listener - waiting for
any handler running on it - before rds_ib_exit() starts to wait.
rds_ib_exit() has one more wrinkle, and an existing hole.
rds_ib_dev_shutdown() only drops the connections still attached to a
device, and each of them moves itself to ib_nodev_conns from its own
shutdown work, asynchronously; the flush_workqueue(rds_wq) in
rds_ib_unregister_client() does not wait for those per-connection
workqueues. A connection that had not migrated by the time
rds_ib_destroy_nodev_conns() made its single sweep was never
destroyed: it outlived the module, still pointing at
rds_ib_transport. With the count, it would hold the count up for
good instead. So the wait takes a resweep callback, which
rds_ib_exit() points at rds_ib_destroy_nodev_conns(): late arrivals
get destroyed on the next poll instead of being waited on. The sweep
claims and unlinks every node it gathers (see the previous patch), so
a resweep never hands a connection to rds_conn_destroy() twice.
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 - nothing flushed rds_wq again after
rds_ib_unregister_client() before, so that free could run after the
module was gone; rds_ib_dev_free() queues nothing further on rds_wq,
so a single pass drains it.
Based on the Oracle UEK commits "net/rds: wait_event_timeout
until zero connections during rmmod" and "net/rds:
Each RDS transport should keep its own connection count".
Signed-off-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>
[achender: reimplementation for net-next: t_conn_count did not exist
upstream and is introduced here; single global waitqueue instead of
per-transport (the loop transport never goes through
rds_trans_register()); unbounded wait with a periodic warning in
place of UEK's wait_event_timeout() + WARN_ON(), plus the resweep for
IB's asynchronous device detach; also cover rds_loop_exit(); rewrite
commit message]
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
net/rds/connection.c | 52 ++++++++++++++++++++++++++++++++++++++++++++
net/rds/ib.c | 17 +++++++++++++++
net/rds/loop.c | 2 ++
net/rds/rds.h | 14 ++++++++++++
net/rds/tcp.c | 1 +
5 files changed, 86 insertions(+)
diff --git a/net/rds/connection.c b/net/rds/connection.c
index 258a6bc3c573..c7655e9339ca 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -48,6 +48,8 @@
/* converting this to RCU is a chore for another day.. */
static DEFINE_SPINLOCK(rds_conn_lock);
static unsigned long rds_conn_count;
+/* woken whenever a transport's t_conn_count drops to zero */
+static DECLARE_WAIT_QUEUE_HEAD(rds_conn_freed_waitq);
static struct hlist_head rds_conn_hash[RDS_CONNECTION_HASH_ENTRIES];
static struct kmem_cache *rds_conn_slab;
@@ -347,6 +349,7 @@ static struct rds_connection *__rds_conn_create(struct net *net,
parent->c_passive = conn;
rds_cong_add_conn(conn);
rds_conn_count++;
+ atomic_inc(&conn->c_trans->t_conn_count);
}
} else {
/* Creating normal conn */
@@ -365,6 +368,7 @@ static struct rds_connection *__rds_conn_create(struct net *net,
hlist_add_head_rcu(&conn->c_hash_node, head);
rds_cong_add_conn(conn);
rds_conn_count++;
+ atomic_inc(&conn->c_trans->t_conn_count);
}
}
spin_unlock_irqrestore(&rds_conn_lock, flags);
@@ -590,6 +594,7 @@ static void rds_conn_destroy_fini(struct kref *kref)
struct rds_connection *conn = container_of(kref, struct rds_connection,
c_refcount);
int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
+ struct rds_transport *trans = conn->c_trans;
unsigned long flags;
int i;
@@ -602,7 +607,54 @@ static void rds_conn_destroy_fini(struct kref *kref)
spin_lock_irqsave(&rds_conn_lock, flags);
rds_conn_count--;
spin_unlock_irqrestore(&rds_conn_lock, flags);
+
+ /* only after everything the transport module owns has been
+ * freed above may its unload proceed
+ */
+ if (!atomic_dec_return(&trans->t_conn_count))
+ wake_up_all(&rds_conn_freed_waitq);
+}
+
+/* Wait for all of @trans's connections to be freed; the free runs
+ * asynchronously once rds_conn_destroy() has quiesced a connection.
+ * Called on transport module unload after an initial sweep has
+ * destroyed the transport's connections; @resweep, when given, is
+ * called on every poll to destroy connections that were still
+ * detaching from a device when the sweep ran (IB). A connection
+ * reference can be held for an application-controlled time - once
+ * incs hold one, an unread datagram pins the inc that carries it, and
+ * thus the connection - so the wait is unbounded: the frees that run
+ * after unload call into this module's text (conn_free, inc_free) and
+ * free into its slabs, so proceeding while any remain would be a
+ * use-after-free, not a leak. Warn periodically so a stuck count is
+ * diagnosable, but never stop waiting. This matches the historical
+ * RDS contract that teardown does not discard queued data.
+ */
+void rds_conn_wait_conns_freed(struct rds_transport *trans,
+ void (*resweep)(void))
+{
+ unsigned long warn_interval =
+ msecs_to_jiffies(RDS_CONN_FREE_WARN_INTERVAL_MS);
+ unsigned long warn_at = jiffies + warn_interval;
+
+ while (!wait_event_timeout(rds_conn_freed_waitq,
+ !atomic_read(&trans->t_conn_count),
+ msecs_to_jiffies(RDS_CONN_FREE_POLL_MS))) {
+ /* A transport whose teardown is asynchronous (IB moves a
+ * connection off its device from the shutdown work) gives
+ * us a resweep to destroy what has arrived since.
+ */
+ if (resweep)
+ resweep();
+ if (time_after_eq(jiffies, warn_at)) {
+ pr_warn("RDS/%s: still waiting for %d connection(s) to be freed before unload\n",
+ trans->t_name,
+ atomic_read(&trans->t_conn_count));
+ warn_at = jiffies + warn_interval;
+ }
+ }
}
+EXPORT_SYMBOL_GPL(rds_conn_wait_conns_freed);
void rds_conn_get(struct rds_connection *conn)
{
diff --git a/net/rds/ib.c b/net/rds/ib.c
index d9879b6129e7..7b1f611c0a3e 100644
--- a/net/rds/ib.c
+++ b/net/rds/ib.c
@@ -540,7 +540,24 @@ void rds_ib_exit(void)
rds_info_deregister_func(RDS6_INFO_IB_CONNECTIONS, rds6_ib_ic_info);
#endif
rds_ib_unregister_client();
+
+ /* rds_ib_dev_shutdown() only dropped the connections still
+ * attached to a device; each moves itself to ib_nodev_conns
+ * from its shutdown work. Destroy what is there now and keep
+ * sweeping the list while the wait sees connections outstanding,
+ * so a late arrival is destroyed rather than waited on forever.
+ */
rds_ib_destroy_nodev_conns();
+ rds_conn_wait_conns_freed(&rds_ib_transport,
+ rds_ib_destroy_nodev_conns);
+
+ /* Tearing down the last connection may have dropped the final
+ * reference on a device, deferring rds_ib_dev_free() to rds_wq.
+ * Drain it before the module goes away; it queues nothing
+ * further on rds_wq.
+ */
+ flush_workqueue(rds_wq);
+
rds_ib_sysctl_exit();
rds_ib_recv_exit();
rds_trans_unregister(&rds_ib_transport);
diff --git a/net/rds/loop.c b/net/rds/loop.c
index 3d063a23bd0b..71f760ccd458 100644
--- a/net/rds/loop.c
+++ b/net/rds/loop.c
@@ -233,6 +233,8 @@ void rds_loop_exit(void)
spin_unlock_irq(&loop_conns_lock);
rds_loop_destroy_gathered_conns(&tmp_list);
+
+ rds_conn_wait_conns_freed(&rds_loop_transport, NULL);
}
static void rds_loop_kill_conns(struct net *net)
diff --git a/net/rds/rds.h b/net/rds/rds.h
index be882269a4d0..c781298993f9 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -564,6 +564,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);
@@ -846,6 +852,14 @@ static inline bool rds_conn_get_unless_zero(struct rds_connection *conn)
{
return kref_get_unless_zero(&conn->c_refcount);
}
+
+/* transport unload waits for its connections to be freed, polling at
+ * the first interval and warning at the second
+ */
+#define RDS_CONN_FREE_POLL_MS 100
+#define RDS_CONN_FREE_WARN_INTERVAL_MS 10000
+void rds_conn_wait_conns_freed(struct rds_transport *trans,
+ void (*resweep)(void));
void rds_conn_drop(struct rds_connection *conn);
void rds_conn_path_drop(struct rds_conn_path *cpath, bool destroy);
void rds_conn_connect_if_down(struct rds_connection *conn);
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 8df4a7d80048..552b32278e30 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -845,6 +845,7 @@ static void rds_tcp_exit(void)
#endif
unregister_pernet_device(&rds_tcp_net_ops);
rds_tcp_destroy_conns();
+ rds_conn_wait_conns_freed(&rds_tcp_transport, NULL);
rds_trans_unregister(&rds_tcp_transport);
rds_tcp_recv_exit();
kmem_cache_destroy(rds_tcp_conn_slab);
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v8 08/13] net/rds: wait for connections to be freed on transport unload
2026-10-03 16:32 ` [PATCH net-next v8 08/13] net/rds: wait for connections to be freed on transport unload Allison Henderson
@ 2026-10-04 16:35 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-10-04 16:35 UTC (permalink / raw)
To: achender; +Cc: netdev, linux-rdma, pabeni, edumazet, kuba, horms
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] This patch fixes two real, pre-existing unload bugs in the IB
transport, but carries no Fixes: tag.
--
net/rds: wait for connections to be freed on transport unload
This patch adds a per-transport t_conn_count of live connections. The IB,
TCP and loop exit paths now wait for that count to reach zero before
unloading. For IB, the wait also calls rds_ib_destroy_nodev_conns() on
every poll, and rds_ib_exit() flushes rds_wq again once the connections
are gone.
> diff --git a/net/rds/ib.c b/net/rds/ib.c
> index d9879b6129e73..7b1f611c0a3e5 100644
> --- a/net/rds/ib.c
> +++ b/net/rds/ib.c
> @@ -540,7 +540,24 @@ void rds_ib_exit(void)
> rds_info_deregister_func(RDS6_INFO_IB_CONNECTIONS, rds6_ib_ic_info);
> #endif
> rds_ib_unregister_client();
> +
> + /* rds_ib_dev_shutdown() only dropped the connections still
> + * attached to a device; each moves itself to ib_nodev_conns
> + * from its shutdown work. Destroy what is there now and keep
> + * sweeping the list while the wait sees connections outstanding,
> + * so a late arrival is destroyed rather than waited on forever.
> + */
> rds_ib_destroy_nodev_conns();
> + rds_conn_wait_conns_freed(&rds_ib_transport,
> + rds_ib_destroy_nodev_conns);
> +
> + /* Tearing down the last connection may have dropped the final
> + * reference on a device, deferring rds_ib_dev_free() to rds_wq.
> + * Drain it before the module goes away; it queues nothing
> + * further on rds_wq.
> + */
> + flush_workqueue(rds_wq);
> +
[Severity: Low]
The resweep and the second flush_workqueue(rds_wq) seem to fix two IB
unload bugs that already exist in the tree before this patch. Should this
patch have a Fixes: tag?
The first bug starts in rds_ib_dev_shutdown(), which queues cp_down_w on
the per-path cp_wq:
list_for_each_entry(ic, &rds_ibdev->conn_list, ib_node)
rds_conn_path_drop(&ic->conn->c_path[0], true);
Since commit 4716af3897e9 ("net/rds: Give each connection path its own
workqueue"), cp_wq is a private ordered workqueue instead of rds_wq. So
the flush_workqueue(rds_wq) in rds_ib_unregister_client() does not wait
for those shutdown works.
If rds_ib_remove_conn() moves a connection to ib_nodev_conns after the
single rds_ib_destroy_nodev_conns() sweep, nothing destroys it. It
outlives the module and still points at rds_ib_transport.
The second bug follows from that. rds_ib_remove_conn() then calls
rds_ib_dev_put(), which does this when it drops the last device
reference:
if (refcount_dec_and_test(&rds_ibdev->refcount))
queue_work(rds_wq, &rds_ibdev->free_work);
That queues rds_ib_dev_free() after the only flush of rds_wq. It could
then run after the module text is gone.
The commit message already calls the first bug "an existing hole". Of
the second it says "nothing flushed rds_wq again after
rds_ib_unregister_client() before". Other fixes in this series carry a
Fixes: tag. Would a Fixes: 4716af3897e9 tag fit here as well?
The opening paragraph of the commit message also says:
At this point in the series the initial reference is the only one
and the wait returns at once; the guarantee becomes load-bearing
with those patches.
Is that accurate for IB? A connection that reaches ib_nodev_conns late
keeps t_conn_count above zero. The loop in rds_conn_wait_conns_freed()
then has to call the resweep to destroy it, so the patch already changes
behavior at this point in the series.
Could the opening paragraph be reworded so backport readers don't take
the patch as a no-op?
It may also be worth splitting out a small standalone fix that doesn't
depend on the kref/t_conn_count infrastructure. One option is to flush
the per-connection shutdown works, or loop until the device lists and
ib_nodev_conns are empty, and then flush rds_wq.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003163215.250253-1-achender%40kernel.org
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v8 09/13] net/rds: hold a connection reference from struct rds_incoming
2026-10-03 16:32 [PATCH net-next v8 00/13] net/rds: make connection lifetime reference-counted Allison Henderson
` (7 preceding siblings ...)
2026-10-03 16:32 ` [PATCH net-next v8 08/13] net/rds: wait for connections to be freed on transport unload Allison Henderson
@ 2026-10-03 16:32 ` Allison Henderson
2026-10-04 16:35 ` netdev-bot+sashiko
2026-10-03 16:32 ` [PATCH net-next v8 10/13] net/rds: take cp_lock to purge cp_send_queue in the quiesce Allison Henderson
` (3 subsequent siblings)
12 siblings, 1 reply; 21+ messages in thread
From: Allison Henderson @ 2026-10-03 16:32 UTC (permalink / raw)
To: netdev, linux-rdma, pabeni, edumazet, kuba, horms; +Cc: achender
From: Sharath Srinivasan <sharath.srinivasan@oracle.com>
struct rds_incoming->i_conn stores a pointer to the connection a message
it belongs to, for both received messages and messages the socket sends.
But without taking a reference, nothing keeps that connection alive.
Embedded as the messages m_inc, an inc routinely outlives the connection
it points at, by sitting in the socket's receive queue until the
application reads it, while the connection is destroyed by netns
teardown or module unload - and every dereference of i_conn after that
point touches freed memory.
Chengfeng Ye reported one way to reach it, where the socket info
callbacks walk a receive queue after rmmod freed the connections:
BUG: KASAN: slab-use-after-free in rds6_inc_info_copy+0x459/0x530 [rds]
Read of size 1 at addr ffff888106031c50 by task poc/101
Call Trace:
rds6_inc_info_copy+0x459/0x530 [rds]
rds6_sock_inc_info+0x2b9/0x3c0 [rds]
rds_info_getsockopt+0x19d/0x380 [rds]
do_sock_getsockopt+0x2ac/0x480
__sys_getsockopt+0x128/0x210
Freed by task 102:
kmem_cache_free+0x1b5/0x3d0
rds_conn_destroy+0x484/0x600 [rds]
rds_loop_exit_net+0x32/0x50 [rds]
unregister_pernet_device+0x2c/0x50
rds_conn_exit+0x13/0xa0 [rds]
rds_exit+0x1a/0xc40 [rds]
__do_sys_delete_module+0x30a/0x4d0
Closing the socket gets there too, with no reader of i_conn other than
RDS itself: freeing an IB inc dereferences i_conn to hand the inc and
its fragments back to the connection's recycle cache, so draining the
receive queue of a socket whose connection is gone crashes in the
transport:
panic
...
rds_ib_recv_cache_put (net/rds/ib_recv.c:703)
rds_ib_inc_free (net/rds/ib_recv.c:207)
rds_clear_recv_queue (net/rds/recv.c:909)
rds_release (net/rds/af_rds.c:212)
__sock_release (net/socket.c:649)
sock_close (net/socket.c:1336)
with the freed connection confirmed by its now-zero reference count:
-trace[9]["inc"].i_conn.c_refcount
(struct kref){
.refcount = (refcount_t){
.refs = (atomic_t){
.counter = (int)0,
},
},
}
Now that connections are reference counted, make every holder of i_conn
own a reference. The pointer is assigned in six places - rds_inc_init()
and rds_inc_path_init() for received messages, rds_recv_incoming() when
it re-points an inc at the connection it arrived on, and
rds_send_queue_rm(), rds_send_probe() and the congestion-map path of
rds_send_xmit() for m_inc - and each of them now takes a reference. The
references are dropped from rds_inc_put() and rds_message_put(), which
are the points where the last user of the pointer goes away.
rds_recv_incoming() takes the new reference before dropping the old one,
so re-pointing an inc at the connection it already refers to cannot free
it. rds_inc_put() drops its reference through a local copy, since
inc_free() may free the memory the inc lives in.
This keeps a connection allocated for as long as messages that arrived
over it are queued on sockets, which is longer than before. What
lingers is the quiesced connection, its per-path workqueues (idle,
every work cancelled) and the transport's per-connection state,
including an IB connection's receive caches: rds_conn_destroy() still
quiesces the connection synchronously, so nothing runs on any of it.
It also means the transport cannot unload while such a datagram is
unread - rds_conn_wait_conns_freed() waits, warning every ten seconds,
until the application reads or closes. That is the intended
behaviour: teardown does not discard queued data, and unloading over a
live reference would be a use-after-free in the transport's text.
The final rds_conn_put() runs the free path, which destroys the per-path
workqueues and therefore may sleep, so the last reference has to be
dropped from process context. It always is. While a connection is
alive its hash-table entry holds the initial reference, so a put from a
completion handler or tasklet can never be the last one; that reference
is dropped by rds_conn_destroy(), from process context, after the
connection has been quiesced and its queued messages freed. The
references that survive that point are the ones held by incs on socket
receive queues and by messages on socket send queues, and those are
dropped from recvmsg and from close - process context in both cases.
This is not a stable candidate: reaching the use-after-free requires
freeing a connection out from under a live socket, which needs
CAP_SYS_MODULE, and the fix
depends on the connection reference counting introduced earlier in this
series.
Based on the Oracle UEK commit "net/rds: fix crash by
expanding kref coverage to rds_incoming.i_conn".
Reported-by: Chengfeng Ye <nicoyip.dev@gmail.com>
Closes: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/
Signed-off-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>
Signed-off-by: Samasth Norway Ananda <samasth.norway.ananda@oracle.com>
[achender: port to net-next: same six assignment sites, but upstream
splits the message free across rds_message_unpin_worker(), so the
m_inc reference is dropped from a shared rds_message_free() helper
that both paths call; rds_recv_incoming() takes the new reference
before dropping the old; rewrite commit message]
Assisted-by: Claude-Code:claude-opus-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
net/rds/message.c | 16 ++++++++++++++--
net/rds/recv.c | 26 +++++++++++++++++++++++---
net/rds/send.c | 4 ++++
3 files changed, 41 insertions(+), 5 deletions(-)
diff --git a/net/rds/message.c b/net/rds/message.c
index 47d5e9ab9b10..cea48ad745ca 100644
--- a/net/rds/message.c
+++ b/net/rds/message.c
@@ -182,6 +182,18 @@ static void rds_message_purge(struct rds_message *rm)
kref_put(&rm->atomic.op_rdma_mr->r_kref, __rds_put_mr_final);
}
+static void rds_message_free(struct rds_message *rm)
+{
+ /* get in rds_send_queue_rm(), rds_send_probe() or the congestion
+ * map path of rds_send_xmit(). Messages that were never queued on
+ * a connection have no reference to drop.
+ */
+ if (rm->m_inc.i_conn)
+ rds_conn_put(rm->m_inc.i_conn);
+
+ kfree(rm);
+}
+
static void rds_message_unpin_worker(struct work_struct *work)
{
struct rds_message *rm = container_of(work, struct rds_message,
@@ -192,7 +204,7 @@ static void rds_message_unpin_worker(struct work_struct *work)
if (rm->atomic.op_unpin_deferred)
rds_atomic_op_unpin_page(&rm->atomic);
- kfree(rm);
+ rds_message_free(rm);
}
void rds_message_put(struct rds_message *rm)
@@ -217,7 +229,7 @@ void rds_message_put(struct rds_message *rm)
return;
}
- kfree(rm);
+ rds_message_free(rm);
}
}
EXPORT_SYMBOL_GPL(rds_message_put);
diff --git a/net/rds/recv.c b/net/rds/recv.c
index 6204e577a90a..1fcd4483d3be 100644
--- a/net/rds/recv.c
+++ b/net/rds/recv.c
@@ -46,6 +46,7 @@ void rds_inc_init(struct rds_incoming *inc, struct rds_connection *conn,
{
refcount_set(&inc->i_refcount, 1);
INIT_LIST_HEAD(&inc->i_item);
+ rds_conn_get(conn); /* put in rds_inc_put() */
inc->i_conn = conn;
inc->i_conn_path = NULL;
inc->i_saddr = *saddr;
@@ -61,6 +62,7 @@ void rds_inc_path_init(struct rds_incoming *inc, struct rds_conn_path *cp,
{
refcount_set(&inc->i_refcount, 1);
INIT_LIST_HEAD(&inc->i_item);
+ rds_conn_get(cp->cp_conn); /* put in rds_inc_put() */
inc->i_conn = cp->cp_conn;
inc->i_conn_path = cp;
inc->i_saddr = *saddr;
@@ -81,9 +83,19 @@ void rds_inc_put(struct rds_incoming *inc)
{
rdsdebug("put inc %p ref %d\n", inc, refcount_read(&inc->i_refcount));
if (refcount_dec_and_test(&inc->i_refcount)) {
+ struct rds_connection *conn = inc->i_conn;
+
BUG_ON(!list_empty(&inc->i_item));
- inc->i_conn->c_trans->inc_free(inc);
+ /* inc_free() can free the memory @inc lives in, so the
+ * connection reference has to be dropped through the
+ * copy taken above.
+ */
+ conn->c_trans->inc_free(inc);
+ /* get in rds_inc_init(), rds_inc_path_init() or
+ * rds_recv_incoming()
+ */
+ rds_conn_put(conn);
}
}
EXPORT_SYMBOL_GPL(rds_inc_put);
@@ -325,6 +337,13 @@ void rds_recv_incoming(struct rds_connection *conn, struct in6_addr *saddr,
unsigned long flags;
struct rds_conn_path *cp;
+ /* every caller initialized @inc with rds_inc_init() or
+ * rds_inc_path_init() first, so i_conn already holds a reference.
+ * Take the new one before dropping the old, so that re-pointing an
+ * inc at the connection it already refers to cannot free it.
+ */
+ rds_conn_get(conn);
+ rds_conn_put(inc->i_conn);
inc->i_conn = conn;
inc->i_rx_jiffies = jiffies;
if (conn->c_trans->t_mp_capable)
@@ -406,8 +425,9 @@ void rds_recv_incoming(struct rds_connection *conn, struct in6_addr *saddr,
* rds_find_bound() uses a global (netns-agnostic) hash table.
* An RDS connection created in netns A can match a socket bound
* in the init netns, delivering inc cross-netns with inc->i_conn
- * pointing into netns A. When cleanup_net() then frees that conn,
- * any subsequent dereference of inc->i_conn is a use-after-free.
+ * pointing into netns A. Cross-netns delivery is wrong on its
+ * own, and the inc's connection reference would keep a
+ * connection of a dead netns around, with a stale c_net.
* Drop the inc if the receiving socket lives in a different netns.
*/
if (!net_eq(sock_net(rds_rs_to_sk(rs)), rds_conn_net(conn))) {
diff --git a/net/rds/send.c b/net/rds/send.c
index 32c411d10e3e..7235974343dd 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -290,6 +290,8 @@ int rds_send_xmit(struct rds_conn_path *cp)
}
rm->data.op_active = 1;
rm->m_inc.i_conn_path = cp;
+ /* put in rds_message_put() */
+ rds_conn_get(cp->cp_conn);
rm->m_inc.i_conn = cp->cp_conn;
cp->cp_xmit_rm = rm;
@@ -947,6 +949,7 @@ static int rds_send_queue_rm(struct rds_sock *rs, struct rds_connection *conn,
/* The code ordering is a little weird, but we're
trying to minimize the time we hold c_lock */
rds_message_populate_header(&rm->m_inc.i_hdr, sport, dport, 0);
+ rds_conn_get(conn); /* put in rds_message_put() */
rm->m_inc.i_conn = conn;
rm->m_inc.i_conn_path = cp;
rds_message_addref(rm);
@@ -1527,6 +1530,7 @@ rds_send_probe(struct rds_conn_path *cp, __be16 sport,
list_add_tail(&rm->m_conn_item, &cp->cp_send_queue);
set_bit(RDS_MSG_ON_CONN, &rm->m_flags);
rds_message_addref(rm);
+ rds_conn_get(cp->cp_conn); /* put in rds_message_put() */
rm->m_inc.i_conn = cp->cp_conn;
rm->m_inc.i_conn_path = cp;
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v8 09/13] net/rds: hold a connection reference from struct rds_incoming
2026-10-03 16:32 ` [PATCH net-next v8 09/13] net/rds: hold a connection reference from struct rds_incoming Allison Henderson
@ 2026-10-04 16:35 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-10-04 16:35 UTC (permalink / raw)
To: achender; +Cc: netdev, linux-rdma, pabeni, edumazet, kuba, horms
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] The commit message gives the wrong triggers for the use-after-
free.
- [Low] The commit fixes a documented use-after-free and panic (KASAN
slab-use-after-free in rds6_inc_info_copy, and the…
- [Low] In rds_inc_on_sock_queue() (net/rds/recv.c:505), the comment "/*
XXX make sure this i_conn is reliable */" still sits right before…
--
net/rds: hold a connection reference from struct rds_incoming
This patch makes every assignment of rds_incoming->i_conn take a
connection reference. Received incs drop it in rds_inc_put(), and
messages drop it through a new rds_message_free() helper. The goal is to
keep a connection allocated for as long as incs or messages that point
at it remain queued on sockets.
Closes: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/
> Embedded as the messages m_inc, an inc routinely outlives the connection
> it points at, by sitting in the socket's receive queue until the
> application reads it, while the connection is destroyed by netns
> teardown or module unload - and every dereference of i_conn after that
> point touches freed memory.
[Severity: Low]
Are netns teardown and core module unload the right triggers here?
rds_create() allocates user RDS sockets with kern == 0:
sk = sk_alloc(net, AF_RDS, GFP_KERNEL, &rds_proto, kern);
So each socket holds a reference on its netns. rds_recv_incoming() also
already drops incs whose socket lives in a different netns from the
connection:
if (!net_eq(sock_net(rds_rs_to_sk(rs)), rds_conn_net(conn))) {
Taken together, it doesn't look like rds_loop_exit_net() or
rds_tcp_exit_net() can destroy a connection that an inc on a live
socket's receive queue still points at.
For the first KASAN trace, rds_proto.owner and rds_proto_ops.owner are
both THIS_MODULE in af_rds.c. Every open RDS socket therefore pins
rds.ko, and delete_module() can't reach rds_exit() while task 101 is in
getsockopt() on one. Does that trace need a forced unload
(CONFIG_MODULE_FORCE_UNLOAD)?
The path that does look reachable without a forced unload is a
cross-transport one. rds_find_bound() looks up sockets by address, port
and scope_id only, and doesn't check rs_transport. A socket bound with
one transport (for example TCP chosen via SO_RDS_TRANSPORT on an IPoIB
address) can then have incs queued that arrived on an IB connection.
That socket pins only rds.ko and its own rs_transport module, so
rmmod rds_rdma succeeds:
rds_ib_exit()
rds_ib_destroy_nodev_conns()
rds_conn_destroy()
A later close then reaches the freed connection:
rds_release()->rds_clear_recv_queue()->rds_inc_put()->rds_ib_inc_free()
This matches the second trace.
Could the commit message describe this cross-transport unload case
instead of netns teardown or unloading rds.ko? The conclusion about
CAP_SYS_MODULE and stable still holds either way.
> Reported-by: Chengfeng Ye <nicoyip.dev@gmail.com>
> Closes: https://lore.kernel.org/netdev/20260720184955.3008978-1-nicoyip.dev@gmail.com/
[Severity: Low]
This isn't a bug, but the trailers have no Fixes: tag. The underlying
defect is that rds_incoming->i_conn never held a connection reference,
and that goes back to the original RDS code.
The message explains why this isn't a stable candidate, so leaving out
Fixes: may be deliberate, to keep AUTOSEL from picking it up. If so,
could the commit message say that explicitly?
> diff --git a/net/rds/recv.c b/net/rds/recv.c
> index 6204e577a90ae..1fcd4483d3be4 100644
> --- a/net/rds/recv.c
> +++ b/net/rds/recv.c
[ ... ]
> @@ -406,8 +425,9 @@ void rds_recv_incoming(struct rds_connection *conn, struct in6_addr *saddr,
> * rds_find_bound() uses a global (netns-agnostic) hash table.
> * An RDS connection created in netns A can match a socket bound
> * in the init netns, delivering inc cross-netns with inc->i_conn
> - * pointing into netns A. When cleanup_net() then frees that conn,
> - * any subsequent dereference of inc->i_conn is a use-after-free.
> + * pointing into netns A. Cross-netns delivery is wrong on its
> + * own, and the inc's connection reference would keep a
> + * connection of a dead netns around, with a stale c_net.
> * Drop the inc if the receiving socket lives in a different netns.
> */
> if (!net_eq(sock_net(rds_rs_to_sk(rs)), rds_conn_net(conn))) {
[Severity: Low]
This isn't a bug, but this comment was updated for the new reference
rules while a nearby one in the same file was left as is.
rds_inc_on_sock_queue() still has:
net/rds/recv.c:rds_inc_on_sock_queue() {
...
if (drop) {
/* XXX make sure this i_conn is reliable */
rds_recv_rcvbuf_delta(rs, sk, inc->i_conn->c_lcong,
-be32_to_cpu(inc->i_hdr.h_len),
inc->i_hdr.h_dport);
...
}
With this patch, an inc linked on rs_recv_queue holds its own reference
on i_conn. That reference is taken in rds_inc_init(), rds_inc_path_init()
or rds_recv_incoming(), and dropped only in rds_inc_put() after
inc_free(). The kernel-doc for rds_conn_wait_conns_freed() also says
that incs now pin their connection.
Should this XXX comment be removed or reworded, since it now contradicts
that contract? It is still there at the end of the series.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003163215.250253-1-achender%40kernel.org
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v8 10/13] net/rds: take cp_lock to purge cp_send_queue in the quiesce
2026-10-03 16:32 [PATCH net-next v8 00/13] net/rds: make connection lifetime reference-counted Allison Henderson
` (8 preceding siblings ...)
2026-10-03 16:32 ` [PATCH net-next v8 09/13] net/rds: hold a connection reference from struct rds_incoming Allison Henderson
@ 2026-10-03 16:32 ` Allison Henderson
2026-10-03 16:32 ` [PATCH net-next v8 11/13] net/rds: hold connection references in lookup, sockets and c_passive Allison Henderson
` (2 subsequent siblings)
12 siblings, 0 replies; 21+ messages in thread
From: Allison Henderson @ 2026-10-03 16:32 UTC (permalink / raw)
To: netdev, linux-rdma, pabeni, edumazet, kuba, horms; +Cc: achender
rds_conn_path_quiesce() empties cp_send_queue by walking it with no
lock held, while every path that adds to that queue -
rds_send_queue_rm(), rds_send_probe(), the retransmit requeue in
rds_send_path_reset() - does so under cp_lock. The unlocked walk was
justified by the destroy running with nothing else alive: at every
destroy trigger there is - netns teardown and module unload - no
socket can still be sending on the connection, since a bound socket
pins its transport module and a namespace closes its sockets before
its RDS connections are torn down.
That argument still holds, but it is an argument about the callers,
not a property of the code. Splice the queue away under cp_lock and
drop the message references outside it, so that the one walker of the
list follows the same lock discipline as its adders. This does not
by itself make a sender that is still running safe - nothing here
tells such a sender that the queue is closed - and it does not need
to, since no such sender exists at any destroy trigger.
While at it, make the purge coherent with rds_send_drop_to(), the
other path that removes messages from a connection queue. drop_to
decides whether it owns the queue's reference by test_and_clear on
RDS_MSG_ON_CONN, and unlinks m_conn_item from whatever list the
message is on. The purge used to BUG_ON() a message that a socket
still had queued, and that assertion is also what kept a drop_to
racing it from putting the queue's reference a second time or
unlinking from the purge list; the lock above is what closes the
stale-next hazard of the old walk, not the assertion. Clear the bit
under cp_lock as part of the splice, so that drop_to leaves a purged
message alone, and turn the BUG_ON() into a WARN_ON_ONCE(): a socket
with messages queued at destroy is still a condition worth reporting -
no sender or closer can be running at any destroy trigger today - but
not one worth a panic, since the socket side keeps its own reference
and retires the message on close.
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
net/rds/connection.c | 29 ++++++++++++++++++++++++-----
1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/net/rds/connection.c b/net/rds/connection.c
index c7655e9339ca..9c8c4b28d2b2 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -540,6 +540,8 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
static void rds_conn_path_quiesce(struct rds_conn_path *cp)
{
struct rds_message *rm, *rtmp;
+ unsigned long flags;
+ LIST_HEAD(purge);
if (!cp->cp_transport_data)
return;
@@ -551,12 +553,29 @@ static void rds_conn_path_quiesce(struct rds_conn_path *cp)
rds_conn_path_drop(cp, true);
flush_work(&cp->cp_down_w);
- /* tear down queued messages */
- list_for_each_entry_safe(rm, rtmp,
- &cp->cp_send_queue,
- m_conn_item) {
+ /* Tear down queued messages. Every path that adds to
+ * cp_send_queue does so under cp_lock; take it here too. No
+ * sender can still be running at any destroy trigger, so this is
+ * lock discipline rather than a race fix: nothing here tells a
+ * sender that the queue is closed.
+ */
+ spin_lock_irqsave(&cp->cp_lock, flags);
+ list_splice_init(&cp->cp_send_queue, &purge);
+ /* Give up the queue's claim on each message while still under
+ * the lock, so that rds_send_drop_to(), which decides ownership
+ * of the connection-queue reference by this bit, neither drops
+ * it a second time nor unlinks the message from our list.
+ */
+ list_for_each_entry(rm, &purge, m_conn_item)
+ clear_bit(RDS_MSG_ON_CONN, &rm->m_flags);
+ spin_unlock_irqrestore(&cp->cp_lock, flags);
+ list_for_each_entry_safe(rm, rtmp, &purge, m_conn_item) {
+ /* No socket can still have messages queued on a connection
+ * at any destroy trigger; say so if one does, since the
+ * socket side then retires the message on its own.
+ */
+ WARN_ON_ONCE(!list_empty(&rm->m_sock_item));
list_del_init(&rm->m_conn_item);
- BUG_ON(!list_empty(&rm->m_sock_item));
rds_message_put(rm);
}
if (cp->cp_xmit_rm)
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* [PATCH net-next v8 11/13] net/rds: hold connection references in lookup, sockets and c_passive
2026-10-03 16:32 [PATCH net-next v8 00/13] net/rds: make connection lifetime reference-counted Allison Henderson
` (9 preceding siblings ...)
2026-10-03 16:32 ` [PATCH net-next v8 10/13] net/rds: take cp_lock to purge cp_send_queue in the quiesce Allison Henderson
@ 2026-10-03 16:32 ` Allison Henderson
2026-10-04 16:35 ` netdev-bot+sashiko
2026-10-03 16:32 ` [PATCH net-next v8 12/13] net/rds: pin the connection across RDMA-CM event handling Allison Henderson
2026-10-03 16:32 ` [PATCH net-next v8 13/13] net/rds: drop rds_conn_count in favor of t_conn_count Allison Henderson
12 siblings, 1 reply; 21+ messages in thread
From: Allison Henderson @ 2026-10-03 16:32 UTC (permalink / raw)
To: netdev, linux-rdma, pabeni, edumazet, kuba, horms; +Cc: achender
From: Sharath Srinivasan <sharath.srinivasan@oracle.com>
Hand out real references everywhere a struct rds_connection pointer
previously escaped bare:
- rds_conn_lookup() takes a reference on the connection it returns
(kref_get_unless_zero(), so that it only ever hands out a live
reference), and
__rds_conn_create() returns the connection with a reference held
for the caller on every path: lookup hit, fresh creation, lost
creation race, and the passive-loopback lookup, which now also
holds the parent while it dereferences parent->c_passive.
- The rs->rs_conn sendmsg cache owns a reference, which is dropped
when the cache is replaced or the socket is released.
rds_sendmsg() itself holds a reference for the duration of the
call, during which reads and updates of rs_conn are serialized by
rs_lock. So neither a concurrent rds_conn_destroy() nor another
sender replacing the cache can free the connection under a sender.
No destroy trigger in this tree - netns teardown, module unload -
can run while a socket is still sending, since a socket holds its
netns and pins its transport module; today the functional change
is the rs_lock serialization that closes the data race below, and
the reference is the discipline the later patches rely on. A
cached connection whose destruction has begun is no longer reused,
which likewise no trigger reaches today.
Instead, sendmsg drops it and looks up or creates a live one, so a
socket cannot get stuck returning -EAGAIN forever against a
quiesced connection; the cache's own reference on it is dropped
right there, so a socket that never sends again does not pin a
quiesced connection until it is closed. Both ToS ioctls use the
same lock now (they
used the unrelated global rds_sock_lock before), and since the
create in rds_sendmsg() samples rs_tos without the lock, the install
re-checks under it that the new connection's ToS still matches the
socket's and returns -EAGAIN if a SIOCRDSSETTOS slipped in between,
rather than sending on, and caching, a connection with the old ToS.
- parent->c_passive owns a reference, dropped when the parent is
destroyed. The pointer is read under rcu_read_lock() and written
under rds_conn_lock, so it is RCU-annotated and accessed through
rcu_dereference()/rcu_assign_pointer(). A passive connection whose
own destroy has begun is neither handed out nor left dangling in
the parent: __rds_conn_create() refuses it, and the child's destroy
clears the parent's pointer and drops that reference itself, so a
quiesced passive conn cannot be revived by a later connect request.
Serializing the rs_conn cache under rs_lock also resolves a
syzbot-reported KCSAN data race between concurrent rds_sendmsg()
calls on the same socket, each installing the connection it created
into rs->rs_conn with a plain store:
BUG: KCSAN: data-race in rds_sendmsg / rds_sendmsg
write to 0xffff888101dec818 of 8 bytes by task 30904 on cpu 0:
rds_sendmsg+0xc1f/0x1580 net/rds/send.c:1332
write to 0xffff888101dec818 of 8 bytes by task 30905 on cpu 1:
rds_sendmsg+0xc1f/0x1580 net/rds/send.c:1332
value changed: 0x0000000000000000 -> 0xffff88811b61faf0
cm_id->context still carries no reference of its own after this
patch; the following patch pins the connection for the duration of
the CM event handler.
rds_tcp_accept_one() needs no destroy check of its own even though it
now gets a referenced connection back from rds_conn_create(): TCP
connections are only destroyed on netns teardown and module unload,
and both go through rds_tcp_kill_sock(), which clears the listen
socket the accept tests first and then flushes the accept work in
rds_tcp_listen_stop(), before destroying anything.
This is not a stable candidate on its own: it depends on the
connection reference counting introduced by the preceding patches,
and the data race it closes has no consequence beyond the report
while no destroy can run under a live socket, which is the case in
this tree.
Based on the Oracle UEK commits "net/rds: Add krefs to
struct rds_connection" and "net/rds: rds_sendmsg must
use rs_conn only when not being destroyed".
Reported-by: syzbot+879c1877016972360186@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=879c1877016972360186
Signed-off-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>
[achender: substantial reimplementation for net-next: upstream has no
conn reaper, per-conn workers hold no references (destroy cancels
them synchronously before the final put), and the sendmsg cache is
serialized with rs_lock instead of UEK's socket flag; rewrite commit
message]
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
net/rds/af_rds.c | 26 ++++++---
net/rds/connection.c | 125 +++++++++++++++++++++++++++++++++++++++++--
net/rds/ib_cm.c | 9 +++-
net/rds/loop.c | 2 +-
net/rds/rds.h | 18 +++++--
net/rds/send.c | 65 +++++++++++++++++++---
net/rds/tcp_listen.c | 12 ++++-
7 files changed, 233 insertions(+), 24 deletions(-)
diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
index d5defe9172e3..99c44d88d2e7 100644
--- a/net/rds/af_rds.c
+++ b/net/rds/af_rds.c
@@ -80,6 +80,14 @@ static int rds_release(struct socket *sock)
rds_notify_queue_get(rs, NULL);
rds_notify_msg_zcopy_purge(&rs->rs_zcookie_queue);
+ /* drop the cached connection reference; no sendmsg can race
+ * with us here, the socket is going away
+ */
+ if (rs->rs_conn) {
+ rds_conn_put(rs->rs_conn);
+ rs->rs_conn = NULL;
+ }
+
spin_lock_bh(&rds_sock_lock);
list_del_init(&rs->rs_item);
spin_unlock_bh(&rds_sock_lock);
@@ -255,6 +263,7 @@ static int rds_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
{
struct rds_sock *rs = rds_sk_to_rs(sock->sk);
rds_tos_t utos, tos = 0;
+ unsigned long flags;
switch (cmd) {
case SIOCRDSSETTOS:
@@ -267,18 +276,23 @@ static int rds_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg)
else
return -ENOIOCTLCMD;
- spin_lock_bh(&rds_sock_lock);
+ /* rs_conn is serialized by rs_lock (see rds_sendmsg());
+ * hold it across the "no connection yet" check and the
+ * rs_tos store so a racing sendmsg cannot cache a conn
+ * whose c_tos then disagrees with rs_tos.
+ */
+ spin_lock_irqsave(&rs->rs_lock, flags);
if (rs->rs_tos || rs->rs_conn) {
- spin_unlock_bh(&rds_sock_lock);
+ spin_unlock_irqrestore(&rs->rs_lock, flags);
return -EINVAL;
}
- rs->rs_tos = tos;
- spin_unlock_bh(&rds_sock_lock);
+ WRITE_ONCE(rs->rs_tos, tos);
+ spin_unlock_irqrestore(&rs->rs_lock, flags);
break;
case SIOCRDSGETTOS:
- spin_lock_bh(&rds_sock_lock);
+ spin_lock_irqsave(&rs->rs_lock, flags);
tos = rs->rs_tos;
- spin_unlock_bh(&rds_sock_lock);
+ spin_unlock_irqrestore(&rs->rs_lock, flags);
if (put_user(tos, (rds_tos_t __user *)arg))
return -EFAULT;
break;
diff --git a/net/rds/connection.c b/net/rds/connection.c
index 9c8c4b28d2b2..7d03d53a3101 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -81,7 +81,10 @@ static struct hlist_head *rds_conn_bucket(const struct in6_addr *laddr,
var |= RDS_INFO_CONNECTION_FLAG_##suffix; \
} while (0)
-/* rcu read lock must be held or the connection spinlock */
+/* rcu read lock must be held or the connection spinlock.
+ * On success a reference is taken on the returned connection; the
+ * caller must drop it with rds_conn_put().
+ */
static struct rds_connection *rds_conn_lookup(struct net *net,
struct hlist_head *head,
const struct in6_addr *laddr,
@@ -98,6 +101,17 @@ static struct rds_connection *rds_conn_lookup(struct net *net,
conn->c_tos == tos &&
net == rds_conn_net(conn) &&
conn->c_dev_if == dev_if) {
+ /* Only ever hand out a live reference.
+ * rds_conn_destroy() unhashes under
+ * rds_conn_lock and waits a grace period
+ * before dropping the initial reference, so
+ * an entry this traversal reaches still holds
+ * at least that one; the conditional get
+ * documents the contract rather than
+ * papering over a zero-refcount entry.
+ */
+ if (!kref_get_unless_zero(&conn->c_refcount))
+ continue;
ret = conn;
break;
}
@@ -163,6 +177,14 @@ static void __rds_conn_path_init(struct rds_connection *conn,
cp->cp_flags = 0;
}
+/* c_passive is written under rds_conn_lock and read under RCU */
+static struct rds_connection *
+rds_conn_passive_locked(struct rds_connection *conn)
+{
+ return rcu_dereference_protected(conn->c_passive,
+ lockdep_is_held(&rds_conn_lock));
+}
+
/* Undo trans->conn_alloc(): it may have allocated transport data for
* every path of a multipath connection, not just for path 0.
*/
@@ -215,7 +237,20 @@ static struct rds_connection *__rds_conn_create(struct net *net,
* We need a second connection object into which we
* can stick the other QP. */
parent = conn;
- conn = parent->c_passive;
+ /* The c_passive pointer holds a reference which is only
+ * dropped one synchronize_rcu() after the pointer is
+ * cleared, so within this RCU section a fetched pointer
+ * is always safe to take a reference on. A passive conn
+ * whose own destroy has begun is not handed out, though:
+ * it is quiesced and about to clear the parent's pointer
+ * itself, and reusing it would re-arm a connection that
+ * nothing will tear down again.
+ */
+ conn = rcu_dereference(parent->c_passive);
+ if (conn && rds_destroy_pending(conn))
+ conn = NULL;
+ if (conn)
+ rds_conn_get(conn);
}
rcu_read_unlock();
if (conn)
@@ -340,13 +375,42 @@ static struct rds_connection *__rds_conn_create(struct net *net,
spin_lock_irqsave(&rds_conn_lock, flags);
if (parent) {
/* Creating passive conn */
- if (parent->c_passive) {
+ if (rds_destroy_pending(parent)) {
+ /* The parent's destroy has begun (it sets the
+ * flag and snatches c_passive under this
+ * lock); do not install a new passive conn
+ * that nothing would ever destroy.
+ */
+ rds_conn_free_transport_data(conn, npaths);
+ free_cp = conn->c_path;
+ kmem_cache_free(rds_conn_slab, conn);
+ conn = ERR_PTR(-ENETDOWN);
+ } else if (rcu_access_pointer(parent->c_passive)) {
+ struct rds_connection *passive;
+
+ passive = rds_conn_passive_locked(parent);
rds_conn_free_transport_data(conn, npaths);
free_cp = conn->c_path;
kmem_cache_free(rds_conn_slab, conn);
- conn = parent->c_passive;
+ /* A passive conn still installed here cannot have
+ * its own destroy begun: rds_conn_destroy() sets
+ * c_destroy_in_prog and clears the parent's pointer
+ * in one rds_conn_lock section, and the netns and
+ * unload cases were ruled out by the parent above.
+ */
+ rds_conn_get(passive);
+ conn = passive;
} else {
- parent->c_passive = conn;
+ /* The initial reference belongs to whoever
+ * destroys the conn (the transport's conn
+ * lists, as for any other conn). Take one
+ * for the c_passive pointer - dropped when
+ * the parent is destroyed - and one for our
+ * caller.
+ */
+ rds_conn_get(conn); /* c_passive */
+ rds_conn_get(conn); /* caller */
+ rcu_assign_pointer(parent->c_passive, conn);
rds_cong_add_conn(conn);
rds_conn_count++;
atomic_inc(&conn->c_trans->t_conn_count);
@@ -365,6 +429,10 @@ static struct rds_connection *__rds_conn_create(struct net *net,
} else {
conn->c_my_gen_num = rds_gen_num;
conn->c_peer_gen_num = 0;
+ /* the initial reference belongs to whoever
+ * destroys the conn; take one for our caller
+ */
+ rds_conn_get(conn);
hlist_add_head_rcu(&conn->c_hash_node, head);
rds_cong_add_conn(conn);
rds_conn_count++;
@@ -375,6 +443,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)
@@ -702,6 +772,9 @@ EXPORT_SYMBOL_GPL(rds_conn_put);
void rds_conn_destroy(struct rds_connection *conn)
{
int i;
+ struct rds_connection *passive, *parent;
+ struct hlist_head *head;
+ bool was_passive = false;
struct rds_conn_path *cp;
int npaths = (conn->c_trans->t_mp_capable ? RDS_MPATH_WORKERS : 1);
@@ -733,7 +806,38 @@ void rds_conn_destroy(struct rds_connection *conn)
/* Ensure conn will not be scheduled for reconnect */
hlist_del_init_rcu(&conn->c_hash_node);
+
+ /* Snatch c_passive while holding the lock:
+ * __rds_conn_create() dereferences it under rcu_read_lock()
+ * (and refuses to install a new one once c_destroy_in_prog is
+ * set, which it checks under this lock). After the
+ * synchronize_rcu() below no one can pick the pointer up any
+ * more and its reference can be dropped.
+ */
+ passive = rds_conn_passive_locked(conn);
+ RCU_INIT_POINTER(conn->c_passive, NULL);
+
+ /* If we are a parent's passive twin, invalidate its pointer to
+ * us as well, so that __rds_conn_create() cannot hand out a
+ * connection whose teardown has begun. The parent is the
+ * hashed connection for our key (a passive conn is never
+ * hashed, and we unhashed ourselves above); it holds its
+ * initial reference for as long as it is hashed, so the lookup
+ * reference dropped below cannot be its last.
+ */
+ head = rds_conn_bucket(&conn->c_laddr, &conn->c_faddr);
+ rcu_read_lock();
+ parent = rds_conn_lookup(rds_conn_net(conn), head, &conn->c_laddr,
+ &conn->c_faddr, conn->c_trans, conn->c_tos,
+ conn->c_dev_if);
+ rcu_read_unlock();
+ if (parent && rds_conn_passive_locked(parent) == conn) {
+ RCU_INIT_POINTER(parent->c_passive, NULL);
+ was_passive = true;
+ }
spin_unlock_irq(&rds_conn_lock);
+ if (parent)
+ rds_conn_put(parent);
synchronize_rcu();
/* shut the connection down */
@@ -750,6 +854,17 @@ void rds_conn_destroy(struct rds_connection *conn)
*/
rds_cong_remove_conn(conn);
+ /* Drop the reference our c_passive pointer held, if any, and
+ * the one a parent's c_passive pointer held on us. Either may
+ * be the last one - the twin's own destroy may already have run
+ * - so these must stay here, in sleepable context with no lock
+ * held, where the free that follows the last put is allowed.
+ */
+ if (passive)
+ rds_conn_put(passive);
+ if (was_passive)
+ rds_conn_put(conn);
+
/* drop the initial reference; the connection is freed from
* rds_conn_destroy_fini() once every holder has dropped theirs
*/
diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 7f3b78b1219f..165a29d4196e 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -926,8 +926,15 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id,
rds_ib_conn_error(conn, "rdma_accept failed\n");
out:
- if (conn)
+ if (conn) {
mutex_unlock(&conn->c_cm_lock);
+ /* Drop the reference rds_conn_create() handed us. The
+ * conn stays reachable through cm_id->context without a
+ * reference of its own for now; the CM event handler is
+ * given one of its own by a following patch.
+ */
+ rds_conn_put(conn);
+ }
if (err)
rdma_reject(cm_id, &err, sizeof(int),
IB_CM_REJ_CONSUMER_DEFINED);
diff --git a/net/rds/loop.c b/net/rds/loop.c
index 71f760ccd458..5bac858df87e 100644
--- a/net/rds/loop.c
+++ b/net/rds/loop.c
@@ -170,7 +170,7 @@ static void rds_loop_destroy_gathered_conns(struct list_head *tmp_list)
list_for_each_entry_safe(lc, _lc, tmp_list, loop_node) {
conn = lc->conn;
- WARN_ON(conn->c_passive);
+ WARN_ON(rcu_access_pointer(conn->c_passive));
spin_lock_irq(&loop_conns_lock);
list_del_init(&lc->loop_node);
diff --git a/net/rds/rds.h b/net/rds/rds.h
index c781298993f9..38f7988e45a3 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -156,7 +156,10 @@ struct rds_connection {
/* Set once, by rds_conn_destroy() under rds_conn_lock - a
* test-and-set, so a second destroy of the same connection
* returns at once - before it cancels the path works. Read
- * through rds_destroy_pending(). A site that arms
+ * through rds_destroy_pending(), which also reports netns
+ * teardown and module unload; the c_passive handling in
+ * __rds_conn_create() uses the same predicate, since those rule
+ * a passive connection out just as well. A site that arms
* a path work must test the predicate and queue the work inside
* one rcu_read_lock() section: the synchronize_rcu() that
* follows the store is what keeps a queue issued after the
@@ -168,7 +171,7 @@ struct rds_connection {
* destroys - and so flushes - that connection afterwards.
*/
bool c_destroy_in_prog;
- struct rds_connection *c_passive;
+ struct rds_connection __rcu *c_passive;
struct rds_transport *c_trans;
struct rds_cong_map *c_lcong;
@@ -676,7 +679,10 @@ struct rds_sock {
/*
* rds_sendmsg caches the conn it used the last time around.
- * This helps avoid costly lookups.
+ * This helps avoid costly lookups. The cache owns a connection
+ * reference, dropped when it is replaced or the socket is
+ * released, and is read and written under rs_lock - except by
+ * rds_release(), which runs once no one else can reach the socket.
*/
struct rds_connection *rs_conn;
@@ -685,7 +691,11 @@ struct rds_sock {
/* seen congestion (ENOBUFS) when sending? */
int rs_seen_congestion;
- /* rs_lock protects all these adjacent members before the newline */
+ /* rs_lock protects all these adjacent members before the newline,
+ * as well as rs_conn above and rs_tos at the end of the struct -
+ * except that rds_sendmsg() samples rs_tos locklessly, with
+ * READ_ONCE(), for the create, and re-checks it under the lock.
+ */
spinlock_t rs_lock;
struct list_head rs_send_queue;
u32 rs_snd_bytes;
diff --git a/net/rds/send.c b/net/rds/send.c
index 7235974343dd..bedcd8b836f6 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -1162,13 +1162,14 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
DECLARE_SOCKADDR(struct sockaddr_in *, usin, msg->msg_name);
__be16 dport;
struct rds_message *rm = NULL;
- struct rds_connection *conn;
+ struct rds_connection *conn = NULL;
int ret = 0;
int queued = 0, allocated_mr = 0;
int nonblock = msg->msg_flags & MSG_DONTWAIT;
long timeo = sock_sndtimeo(sk, nonblock);
struct rds_conn_path *cpath;
struct in6_addr daddr;
+ unsigned long flags;
__u32 scope_id = 0;
size_t rdma_payload_len = 0;
bool zcopy = ((msg->msg_flags & MSG_ZEROCOPY) &&
@@ -1343,21 +1344,68 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
rm->m_daddr = daddr;
/* rds_conn_create has a spinlock that runs with IRQ off.
- * Caching the conn in the socket helps a lot. */
- if (rs->rs_conn && ipv6_addr_equal(&rs->rs_conn->c_faddr, &daddr) &&
- rs->rs_tos == rs->rs_conn->c_tos) {
- conn = rs->rs_conn;
+ * Caching the conn in the socket helps a lot.
+ *
+ * The cached rs_conn holds a connection reference; take one of
+ * our own for the duration of this call (dropped on both exit
+ * paths), so that neither a concurrent sender replacing the
+ * cache nor rds_conn_destroy() can free the connection under
+ * us. A cached connection whose destruction has begun is not
+ * reused: dropping it here lets the next sendmsg look up or
+ * create a live one instead of returning -EAGAIN forever.
+ */
+ spin_lock_irqsave(&rs->rs_lock, flags);
+ conn = rs->rs_conn;
+ if (conn && rds_destroy_pending(conn)) {
+ /* drop the cache's reference right here, or the socket
+ * would pin the quiesced connection until it is closed
+ */
+ rs->rs_conn = NULL;
+ spin_unlock_irqrestore(&rs->rs_lock, flags);
+ rds_conn_put(conn);
+ conn = NULL;
} else {
+ if (conn && ipv6_addr_equal(&conn->c_faddr, &daddr) &&
+ rs->rs_tos == conn->c_tos)
+ rds_conn_get(conn);
+ else
+ conn = NULL;
+ spin_unlock_irqrestore(&rs->rs_lock, flags);
+ }
+
+ if (!conn) {
+ struct rds_connection *old;
+
conn = rds_conn_create_outgoing(sock_net(sock->sk),
&rs->rs_bound_addr, &daddr,
- rs->rs_transport, rs->rs_tos,
+ rs->rs_transport,
+ READ_ONCE(rs->rs_tos),
sock->sk->sk_allocation,
scope_id);
if (IS_ERR(conn)) {
ret = PTR_ERR(conn);
+ conn = NULL;
+ goto out;
+ }
+ /* rs_tos was sampled without rs_lock for the create above,
+ * and SIOCRDSSETTOS only refuses a change once rs_conn is
+ * set, so it can have changed underneath us. Do not
+ * install - or send on - a connection whose ToS no longer
+ * matches the socket's; the retry uses the new one.
+ */
+ spin_lock_irqsave(&rs->rs_lock, flags);
+ if (conn->c_tos != rs->rs_tos) {
+ spin_unlock_irqrestore(&rs->rs_lock, flags);
+ ret = -EAGAIN;
goto out;
}
+ /* hand the cache its own reference */
+ rds_conn_get(conn);
+ old = rs->rs_conn;
rs->rs_conn = conn;
+ spin_unlock_irqrestore(&rs->rs_lock, flags);
+ if (old)
+ rds_conn_put(old);
}
if (conn->c_trans->t_mp_capable) {
@@ -1477,6 +1525,8 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
kfree(vct.vec[ind].iov);
kfree(vct.vec);
+ rds_conn_put(conn);
+
return payload_len;
out:
@@ -1484,6 +1534,9 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
kfree(vct.vec[ind].iov);
kfree(vct.vec);
+ if (conn)
+ rds_conn_put(conn);
+
/* If the user included a RDMA_MAP cmsg, we allocated a MR on the fly.
* If the sendmsg goes through, we keep the MR. If it fails with EAGAIN
* or in any other way, we need to destroy the MR again */
diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index 8a0c54aced5e..bb4f01c07169 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -153,7 +153,7 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
{
struct socket *listen_sock = rtn->rds_tcp_listen_sock;
struct socket *new_sock = NULL;
- struct rds_connection *conn;
+ struct rds_connection *conn = NULL;
int ret;
struct inet_sock *inet;
struct rds_tcp_connection *rs_tcp = NULL;
@@ -229,6 +229,7 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
if (IS_ERR(conn)) {
ret = PTR_ERR(conn);
+ conn = NULL;
goto out;
}
/* An incoming SYN request came in, and TCP just accepted it.
@@ -277,6 +278,13 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
cp = rs_tcp->t_cpath;
conn_state = rds_conn_path_state(cp);
WARN_ON(conn_state == RDS_CONN_UP);
+ /* A connection whose destroy has begun cannot be found here:
+ * TCP connections are only destroyed on netns teardown and on
+ * module unload, and both go through rds_tcp_kill_sock() - which
+ * clears the listen socket that the top of this function tests
+ * and then flushes this work in rds_tcp_listen_stop() - before
+ * any connection is destroyed.
+ */
if (conn_state != RDS_CONN_CONNECTING && conn_state != RDS_CONN_ERROR) {
rds_conn_path_drop(cp, 0);
goto rst_nsk;
@@ -347,6 +355,8 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
mutex_unlock(&rs_tcp->t_conn_path_lock);
if (new_sock)
sock_release(new_sock);
+ if (conn)
+ rds_conn_put(conn);
mutex_unlock(&rtn->rds_tcp_accept_lock);
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v8 11/13] net/rds: hold connection references in lookup, sockets and c_passive
2026-10-03 16:32 ` [PATCH net-next v8 11/13] net/rds: hold connection references in lookup, sockets and c_passive Allison Henderson
@ 2026-10-04 16:35 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-10-04 16:35 UTC (permalink / raw)
To: achender; +Cc: netdev, linux-rdma, pabeni, edumazet, kuba, horms
Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.
Low priority / nit picks for your consideration:
- [Low] Two new comments in this patch don't match what the rs_conn cache
code does.
Pre-existing issues:
- [Medium] The rs_conn cache-hit check in rds_sendmsg() (net/rds/send.c)
compares only the destination address and ToS…
--
net/rds: hold connection references in lookup, sockets and c_passive
This patch makes rds_conn_lookup(), __rds_conn_create(), the rs->rs_conn
sendmsg cache and parent->c_passive hold real connection references.
rs_lock now serializes the rs_conn cache, which also resolves a syzbot
KCSAN data race between concurrent rds_sendmsg() calls.
Closes: https://syzkaller.appspot.com/bug?extid=879c1877016972360186
> diff --git a/net/rds/send.c b/net/rds/send.c
> index 7235974343dda..bedcd8b836f66 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c
[ ... ]
> @@ -1343,21 +1344,68 @@ int rds_sendmsg(struct socket *sock, struct msghdr *msg, size_t payload_len)
> rm->m_daddr = daddr;
>
> /* rds_conn_create has a spinlock that runs with IRQ off.
> - * Caching the conn in the socket helps a lot. */
> - if (rs->rs_conn && ipv6_addr_equal(&rs->rs_conn->c_faddr, &daddr) &&
> - rs->rs_tos == rs->rs_conn->c_tos) {
> - conn = rs->rs_conn;
> + * Caching the conn in the socket helps a lot.
> + *
> + * The cached rs_conn holds a connection reference; take one of
> + * our own for the duration of this call (dropped on both exit
> + * paths), so that neither a concurrent sender replacing the
> + * cache nor rds_conn_destroy() can free the connection under
> + * us. A cached connection whose destruction has begun is not
> + * reused: dropping it here lets the next sendmsg look up or
> + * create a live one instead of returning -EAGAIN forever.
> + */
> + spin_lock_irqsave(&rs->rs_lock, flags);
> + conn = rs->rs_conn;
> + if (conn && rds_destroy_pending(conn)) {
> + /* drop the cache's reference right here, or the socket
> + * would pin the quiesced connection until it is closed
> + */
> + rs->rs_conn = NULL;
> + spin_unlock_irqrestore(&rs->rs_lock, flags);
> + rds_conn_put(conn);
> + conn = NULL;
[Severity: Low]
This isn't a bug, but do the new comments match what this path does?
The comment above says that dropping the cached connection "lets the next
sendmsg look up or create a live one". After conn = NULL here, though,
execution goes straight into the if (!conn) block below and calls
rds_conn_create_outgoing() in this same call. A later sendmsg only has to
do the work if that create fails.
The new rs_conn comment in net/rds/rds.h also says:
The cache owns a connection
reference, dropped when it is replaced or the socket is
released, ...
That misses this eviction case, where rds_sendmsg() sets rs_conn to NULL
whenever rds_destroy_pending(conn) is true. If the create that follows
fails, rs_conn stays NULL and nothing replaces it.
Could both comments be updated to cover the eviction path?
> } else {
> + if (conn && ipv6_addr_equal(&conn->c_faddr, &daddr) &&
> + rs->rs_tos == conn->c_tos)
> + rds_conn_get(conn);
[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch, but
should the cache-hit check also compare scope_id against conn->c_dev_if?
rds_conn_lookup() tells connections apart with conn->c_dev_if == dev_if.
The TCP connect path in net/rds/tcp_connect.c uses
sin6.sin6_scope_id = conn->c_dev_if. So a cached connection is tied to
one interface.
If the socket is bound to a global IPv6 address, rs_bound_scope_id is 0.
This earlier check in rds_sendmsg() then accepts any nonzero scope_id:
if (scope_id != rs->rs_bound_scope_id) {
if (!scope_id) {
scope_id = rs->rs_bound_scope_id;
} else if (rs->rs_bound_scope_id) {
A send to fe80::X with sin6_scope_id = A creates and caches a conn with
c_dev_if = A. A later send to fe80::X with sin6_scope_id = B hits the
cache here and reuses the interface A connection.
Would that message go to fe80::X on link A instead of link B? The
baseline predicate used the same key, and this rewrite keeps it. Adding
conn->c_dev_if == scope_id to the check looks like it would fix this.
> + else
> + conn = NULL;
> + spin_unlock_irqrestore(&rs->rs_lock, flags);
> + }
[ ... ]
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003163215.250253-1-achender%40kernel.org
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v8 12/13] net/rds: pin the connection across RDMA-CM event handling
2026-10-03 16:32 [PATCH net-next v8 00/13] net/rds: make connection lifetime reference-counted Allison Henderson
` (10 preceding siblings ...)
2026-10-03 16:32 ` [PATCH net-next v8 11/13] net/rds: hold connection references in lookup, sockets and c_passive Allison Henderson
@ 2026-10-03 16:32 ` Allison Henderson
2026-10-04 16:35 ` netdev-bot+sashiko
2026-10-03 16:32 ` [PATCH net-next v8 13/13] net/rds: drop rds_conn_count in favor of t_conn_count Allison Henderson
12 siblings, 1 reply; 21+ messages in thread
From: Allison Henderson @ 2026-10-03 16:32 UTC (permalink / raw)
To: netdev, linux-rdma, pabeni, edumazet, kuba, horms; +Cc: achender
rds_rdma_cm_event_handler_cmn() picks the connection up from
cm_id->context, which carries no reference, and holds c_cm_lock - a
mutex that lives in the connection's path array - across the transport
callbacks. Before this series that was already a use-after-free
whenever a callback destroyed the connection, since rds_conn_destroy()
freed it synchronously and the handler's mutex_unlock() ran on freed
memory; the one such callback, rds_ib_cm_connect_complete() on a
protocol version below 3.1, has meanwhile been switched to
rds_conn_drop() by commit f97d8c7bab78 ("rds: ib: use rds_conn_drop()
on protocol version mismatch"), which also removed the deadlock that
destroy took on c_cm_lock.
Now that a connection is freed by its last reference, none of the
callbacks the handler dispatches drops a reference on the connection
it was handed: the version-mismatch path only drops the connection,
and rds_ib_cm_handle_connect() puts the reference rds_conn_create()
gave it, on a connection the listener's cm_id never pointed at. What
can reach zero while an event is in flight are the holders outside
the handler - the destroy's initial reference, a socket's cache, a
parent's c_passive, an inc. Today the shutdown pass destroys the
cm_id, and rdma_destroy_id() waits for a running handler, before the
initial reference is dropped, so every event is ordered ahead of the
free; the pin is defensive, keeping the handler correct without
leaning on that ordering. Take a reference for the duration of the
handler, and ignore the event if the connection is already at zero
references rather than handle it.
rds_ib_cm_initiate_connect() has the same hole on the active side: a
connection whose destroy began while its address and route were
resolving reaches RDMA_CM_EVENT_ROUTE_RESOLVED and sets up a QP -
taking a device reference in rds_ib_add_conn() - after the destroy's
shutdown pass has run, or with that pass waiting on c_cm_lock behind
the handler. Nothing would ever release the QP, the cm_id or the
device reference, and rds_ib_exit() would wait forever for the
device. Return before the QP is set up when the destroy is pending;
the cm_id is still ic->i_cm_id, so the shutdown destroys it. An
unload that begins after this check has passed is covered too:
rds_ib_dev_shutdown() marks every device before the exit sweep runs,
and rds_ib_add_conn() refuses a marked device, so such a connect
fails and its connection stays on the nodev list for the sweep.
rds_ib_cm_handle_connect() has the mirror-image hole: a connection
whose destroy has already quiesced it sits in RDS_CONN_DOWN with no
cm_id, which is exactly the state the DOWN -> CONNECTING transition
claims. A connect request arriving then would install a new cm_id and
QP on a connection that is only waiting for its last reference to go
away, and nothing would tear them down again. Re-check
rds_destroy_pending() under c_cm_lock and reject the request instead.
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
net/rds/ib_cm.c | 22 ++++++++++++++++++++--
net/rds/rdma_transport.c | 19 ++++++++++++++++++-
2 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
index 165a29d4196e..6307c88c3143 100644
--- a/net/rds/ib_cm.c
+++ b/net/rds/ib_cm.c
@@ -876,6 +876,13 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id,
* see the comment above rds_queue_reconnect()
*/
mutex_lock(&conn->c_cm_lock);
+ /* A destroy that has already quiesced this conn leaves it in
+ * RDS_CONN_DOWN with no cm_id, exactly what the transition
+ * below would happily claim; nothing would tear the new cm_id
+ * and QP down again before the conn is freed. Reject instead.
+ */
+ if (rds_destroy_pending(conn))
+ goto out;
if (!rds_conn_transition(conn, RDS_CONN_DOWN, RDS_CONN_CONNECTING)) {
if (rds_conn_state(conn) == RDS_CONN_UP) {
rdsdebug("incoming connect while connecting\n");
@@ -930,8 +937,8 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id,
mutex_unlock(&conn->c_cm_lock);
/* Drop the reference rds_conn_create() handed us. The
* conn stays reachable through cm_id->context without a
- * reference of its own for now; the CM event handler is
- * given one of its own by a following patch.
+ * reference of its own; rds_rdma_cm_event_handler_cmn()
+ * takes one for the duration of each event it handles.
*/
rds_conn_put(conn);
}
@@ -950,6 +957,17 @@ int rds_ib_cm_initiate_connect(struct rdma_cm_id *cm_id, bool isv6)
union rds_ib_conn_priv dp;
int ret;
+ /* A destroy that began while the address and route were being
+ * resolved has already quiesced this conn, or is waiting on
+ * c_cm_lock to do so. Setting up a QP now would leave it - and
+ * the device reference rds_ib_add_conn() takes - with no
+ * shutdown pass left to tear them down. The id we were handed
+ * is still ic->i_cm_id, so return success and let that shutdown
+ * destroy it, rather than have the rdma_cm destroy it on error.
+ */
+ if (rds_destroy_pending(conn))
+ return 0;
+
/* If the peer doesn't do protocol negotiation, we must
* default to RDSv3.0 */
rds_ib_set_protocol(conn, RDS_PROTOCOL_4_1);
diff --git a/net/rds/rdma_transport.c b/net/rds/rdma_transport.c
index 3f853004c490..a789104725c9 100644
--- a/net/rds/rdma_transport.c
+++ b/net/rds/rdma_transport.c
@@ -63,6 +63,21 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
if (cm_id->device->node_type == RDMA_NODE_IB_CA)
trans = &rds_ib_transport;
+ /* cm_id->context carries no reference of its own. Pin the
+ * connection for the duration of the handler, since the mutex
+ * released at out: lives in the connection's path array. None
+ * of the callbacks below drops a reference on this connection,
+ * and the shutdown destroys the cm_id - waiting for a running
+ * handler - before the last reference can go, so this is
+ * defensive. A connection already at zero references gets no
+ * events handled.
+ */
+ if (conn && !rds_conn_get_unless_zero(conn)) {
+ rdsdebug("conn %p id %p is being freed, ignoring event\n",
+ conn, cm_id);
+ return 0;
+ }
+
/* Prevent shutdown from tearing down the connection
* while we're executing. */
if (conn) {
@@ -171,8 +186,10 @@ static int rds_rdma_cm_event_handler_cmn(struct rdma_cm_id *cm_id,
}
out:
- if (conn)
+ if (conn) {
mutex_unlock(&conn->c_cm_lock);
+ rds_conn_put(conn);
+ }
rdsdebug("id %p event %u (%s) handling ret %d\n", cm_id, event->event,
rdma_event_msg(event->event), ret);
--
2.25.1
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH net-next v8 12/13] net/rds: pin the connection across RDMA-CM event handling
2026-10-03 16:32 ` [PATCH net-next v8 12/13] net/rds: pin the connection across RDMA-CM event handling Allison Henderson
@ 2026-10-04 16:35 ` netdev-bot+sashiko
0 siblings, 0 replies; 21+ messages in thread
From: netdev-bot+sashiko @ 2026-10-04 16:35 UTC (permalink / raw)
To: achender; +Cc: netdev, linux-rdma, pabeni, edumazet, kuba, horms
Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] The commit message and the new comment in
rds_ib_cm_initiate_connect() (net/rds/ib_cm.c) describe a resource…
--
net/rds: pin the connection across RDMA-CM event handling
This patch makes rds_rdma_cm_event_handler_cmn() take a reference on the
connection for the duration of each RDMA-CM event. It also adds
rds_destroy_pending() checks to rds_ib_cm_initiate_connect() and
rds_ib_cm_handle_connect(), so a connection that is being destroyed does
not get a new QP or cm_id set up.
> diff --git a/net/rds/ib_cm.c b/net/rds/ib_cm.c
> index 165a29d4196e8..6307c88c31431 100644
> --- a/net/rds/ib_cm.c
> +++ b/net/rds/ib_cm.c
> @@ -876,6 +876,13 @@ int rds_ib_cm_handle_connect(struct rdma_cm_id *cm_id,
> * see the comment above rds_queue_reconnect()
> */
> mutex_lock(&conn->c_cm_lock);
> + /* A destroy that has already quiesced this conn leaves it in
> + * RDS_CONN_DOWN with no cm_id, exactly what the transition
> + * below would happily claim; nothing would tear the new cm_id
> + * and QP down again before the conn is freed. Reject instead.
> + */
> + if (rds_destroy_pending(conn))
> + goto out;
[ ... ]
> @@ -950,6 +957,17 @@ int rds_ib_cm_initiate_connect(struct rdma_cm_id *cm_id, bool isv6)
> union rds_ib_conn_priv dp;
> int ret;
>
> + /* A destroy that began while the address and route were being
> + * resolved has already quiesced this conn, or is waiting on
> + * c_cm_lock to do so. Setting up a QP now would leave it - and
> + * the device reference rds_ib_add_conn() takes - with no
> + * shutdown pass left to tear them down. The id we were handed
> + * is still ic->i_cm_id, so return success and let that shutdown
> + * destroy it, rather than have the rdma_cm destroy it on error.
> + */
> + if (rds_destroy_pending(conn))
> + return 0;
> +
[Severity: Medium]
Can the leak described in this comment and in the commit message actually
happen? The commit message says:
"a connection whose destroy began while its address and route were
resolving reaches RDMA_CM_EVENT_ROUTE_RESOLVED and sets up a QP -
taking a device reference in rds_ib_add_conn() - after the destroy's
shutdown pass has run, or with that pass waiting on c_cm_lock behind
the handler. Nothing would ever release the QP, the cm_id or the
device reference, and rds_ib_exit() would wait forever for the
device."
First, the case where the shutdown pass is waiting on c_cm_lock. The
quiesce calls rds_conn_path_drop(cp, true), which sets RDS_CONN_ERROR and
queues cp_down_w. Nothing in the ROUTE_RESOLVED path moves the state away
from ERROR.
Once the handler releases c_cm_lock, rds_conn_shutdown() moves ERROR to
DISCONNECTING and calls rds_ib_conn_path_shutdown(). Because ic->i_cm_id
is set, that function already tears everything down:
rds_ib_conn_path_shutdown() {
...
rdma_destroy_id(ic->i_cm_id);
...
if (ic->rds_ibdev)
rds_ib_remove_conn(ic->rds_ibdev, conn);
ic->i_cm_id = NULL;
...
}
So the QP, CQs, cm_id and device reference are all released, with or
without this patch.
Second, the case where the shutdown pass has already run. By then
ic->i_cm_id has been destroyed and cleared, and rdma_destroy_id() waits
for a running handler and blocks later callbacks on that id.
A ROUTE_RESOLVED event on some other id fails the ibic->i_cm_id == cm_id
test in rds_rdma_cm_event_handler_cmn() and only calls rds_conn_drop().
While the pass is running, the handler exits early on
RDS_CONN_DISCONNECTING. A conn that is unhashed and destroy-pending can't
be reconnected either.
Third, the device reference. IB conns are only destroyed from
rds_ib_destroy_nodev_conns(), which rds_ib_exit() calls after
rds_ib_unregister_client(). By that point rds_ib_dev_shutdown() has set
shutting_down on every device, and rds_ib_add_conn() bails out before it
takes the reference:
rds_ib_add_conn() {
...
if (rds_ibdev->shutting_down) {
ret = -ENODEV;
...
}
As far as I can tell, the new early return only fires when
rds_destroy_pending() is true through t_unloading (rds_ib_is_unloading())
before any destroy has started. Neither the comment nor the commit
message mentions that case, and the exit sweep would tear such a conn
down anyway.
Also, suppose the "already quiesced" case could be reached. Would
returning 0 here help? It would leave a cm_id whose context points at a
conn that is about to be freed, and rds_ib_conn_free() never destroys
ic->i_cm_id.
The check in rds_ib_cm_handle_connect() has the same problem. The commit
message calls it "the mirror-image hole", but rds_rdma_exit() stops the
listeners before rds_ib_exit() runs:
rds_rdma_exit() {
rds_rdma_listen_stop();
rds_ib_exit();
}
So the listeners are destroyed before the unloading flag is set or any
IB conn is destroyed. In addition, the lookup in rds_conn_create() can't
return a conn once rds_conn_destroy() has unhashed it under
rds_conn_lock.
Both checks look harmless. Could the commit message and these two
comments describe them as defensive, the way the handler pin is
described? Right now they read as fixes for a reachable QP/cm_id/device
reference leak and an rds_ib_exit() hang.
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261003163215.250253-1-achender%40kernel.org
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH net-next v8 13/13] net/rds: drop rds_conn_count in favor of t_conn_count
2026-10-03 16:32 [PATCH net-next v8 00/13] net/rds: make connection lifetime reference-counted Allison Henderson
` (11 preceding siblings ...)
2026-10-03 16:32 ` [PATCH net-next v8 12/13] net/rds: pin the connection across RDMA-CM event handling Allison Henderson
@ 2026-10-03 16:32 ` Allison Henderson
12 siblings, 0 replies; 21+ messages in thread
From: Allison Henderson @ 2026-10-03 16:32 UTC (permalink / raw)
To: netdev, linux-rdma, pabeni, edumazet, kuba, horms; +Cc: achender
The patch "net/rds: wait for connections to be freed on transport
unload", earlier in this series, gave each transport its own
connection count in t_conn_count, incremented and decremented at
exactly the points where the global rds_conn_count is. That leaves
rds_conn_count with a single remaining consumer: the seed of the
per-path workqueue names in __rds_conn_create().
Switch the name seed to t_conn_count, as UEK does, and remove
rds_conn_count. The numbering becomes per-transport instead of
global, so connections of different transports can now receive the
same seed; workqueue names carry no uniqueness requirement, and the
seed was already reused as the count rose and fell. Removing the
counter also removes the rds_conn_lock round-trip that
rds_conn_destroy_fini() took solely to decrement it. (The free path
still sleeps in destroy_workqueue(), so the last reference has to be
dropped from process context as before; only that one lock goes.)
Based on the Oracle UEK commit "net/rds: Each RDS transport
should keep its own connection count".
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
net/rds/connection.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/net/rds/connection.c b/net/rds/connection.c
index 7d03d53a3101..8c4df7d6b294 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -47,7 +47,6 @@
/* converting this to RCU is a chore for another day.. */
static DEFINE_SPINLOCK(rds_conn_lock);
-static unsigned long rds_conn_count;
/* woken whenever a transport's t_conn_count drops to zero */
static DECLARE_WAIT_QUEUE_HEAD(rds_conn_freed_waitq);
static struct hlist_head rds_conn_hash[RDS_CONNECTION_HASH_ENTRIES];
@@ -338,12 +337,13 @@ static struct rds_connection *__rds_conn_create(struct net *net,
init_waitqueue_head(&conn->c_hs_waitq);
for (i = 0; i < npaths; i++) {
+ int seq = atomic_read(&trans->t_conn_count);
+
__rds_conn_path_init(conn, &conn->c_path[i],
is_outgoing);
conn->c_path[i].cp_index = i;
conn->c_path[i].cp_wq =
- alloc_ordered_workqueue("krds_cp_wq#%lu/%d", 0,
- rds_conn_count, i);
+ alloc_ordered_workqueue("krds_cp_wq#%d/%d", 0, seq, i);
if (!conn->c_path[i].cp_wq)
conn->c_path[i].cp_wq = rds_wq;
}
@@ -412,7 +412,6 @@ static struct rds_connection *__rds_conn_create(struct net *net,
rds_conn_get(conn); /* caller */
rcu_assign_pointer(parent->c_passive, conn);
rds_cong_add_conn(conn);
- rds_conn_count++;
atomic_inc(&conn->c_trans->t_conn_count);
}
} else {
@@ -435,7 +434,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);
}
}
@@ -684,7 +682,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++)
@@ -693,10 +690,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
^ permalink raw reply related [flat|nested] 21+ messages in thread