[PATCH net v3 0/5] net/rds: own the fastpath locks across connection teardown

[PATCH net v3 0/5] net/rds: own the fastpath locks across connection teardown

[Allison Henderson]

Hi all,

This is v3 of the follow-up set to "net/rds: Bug fix ports, part 2"
[1] (v1 of this set is at [2], v2 at [3]).  During review of part 2,
the later half of that series needed more work than a respin, so it
was split off into this set together with the companion fixes
identified along the way.  As discussed on the v2 thread, it is now
targeted at net.

RDS connection teardown quiesces the transmit and receive-refill fast
paths by waiting for the RDS_IN_XMIT/RDS_RECV_REFILL bits to be
sampled clear.  Sampling a bit clear is not owning it: the fast path
can re-take its bit right after the wait returns and then run
concurrently with the transport shutdown and the send-state reset.
Oracle UEK closed this by making teardown acquire the bits as locks
("rds: Make sure transmit path and connection tear-down does not run
concurrently"); patches 4 and 5 do the same for the two
rds_send_path_reset() call sites upstream.  These two are effectively
v3 of patches 4 and 3 of "net/rds: Bug fix ports, part 2" [1].

Making teardown block on the bits as locks promotes three latent
ordering bugs from rare to load-bearing, so they are fixed first:

  Patch 1: release_in_xmit() checks waitqueue_active() after
  clear_bit_unlock(), which does not order that read; the wake-up of
  the (now uninterruptible, untimed) teardown wait can be lost.  Use
  wq_has_sleeper().

  Patch 2: rds_conn_path_reset() wipes the whole cp_flags word with a
  plain store.  Once teardown owns bits in that word across the
  reset, a blanket store would end lock ownership early - and it
  already races atomic RMWs on the same word today.  Clear the bits
  the reset is responsible for individually, as Oracle UEK also does.

  Patch 3: rds_tcp_reset_callbacks() stores RDS_CONN_RESETTING
  unconditionally, which can overwrite the RDS_CONN_ERROR or
  RDS_CONN_DISCONNECTING of a shutdown already in progress on the
  same path and send that shutdown through an extra drop cycle.  Once
  the accept path can park for the duration of a teardown (patch 5)
  that window widens, so make the transition conditional first, as
  Oracle UEK does.

With those in place, patch 4 converts rds_tcp_reset_callbacks() from
waiting on RDS_IN_XMIT to acquiring it, holding it across the socket
swap and rds_send_path_reset(), and patch 5 has rds_conn_shutdown()
hold both bit locks across the transport shutdown and path reset.
The order matters: with the accept path owning the lock first, no
intermediate commit leaves it resuming on a socket pointer that a
lock-holding teardown has already released.

[PATCH net 1/5] net/rds: use wq_has_sleeper() in release_in_xmit()
  Restore full barrier before wake-up checks in release_in_xmit()

[PATCH net 2/5] net/rds: clear cp_flags bits individually in rds_conn_path_reset()
  Partial port of commit d04896037223 ("net/rds: Preserve essential connection state flags")
  https://github.com/oracle/linux-uek/commit/d04896037223

[PATCH net 3/5] net/rds: tcp: don't force RDS_CONN_RESETTING over a concurrent shutdown
  Port of commit 72c176a1d9ac ("net/rds: Don't force state RDS_CONN_RESETTING")
  https://github.com/oracle/linux-uek/commit/72c176a1d9ac

[PATCH net 4/5] net/rds: acquire RDS_IN_XMIT in rds_tcp_reset_callbacks()
  Extend the port in patch 5 to the second rds_send_path_reset() call site

[PATCH net 5/5] net/rds: acquire the fastpath locks in rds_conn_shutdown()
  Port of commit 2b8aaa4f163b ("rds: Make sure transmit path and connection tear-down does not run concurrently")
  https://github.com/oracle/linux-uek/commit/2b8aaa4f163b

Changes since v2 [3]:
  - Retargeted at net; rebased onto net/main.
  - No functional changes apart from patch 5 now releasing
    RDS_RECV_REFILL with clear_bit_unlock(), matching the RDS_IN_XMIT
    release beside it, so the teardown's ring and send-state writes
    are ordered before the bit is seen clear.
  - Comment and changelog corrections from the v2 review pass:
    patch 2 no longer claims a quiescence guarantee that only patch 5
    delivers; patch 3 spells out that the fallback drop replaces the
    shutdown's state with RDS_CONN_ERROR (which rds_conn_shutdown()
    tolerates) and queues one more down-work pass; patch 4's block
    comment names all three t_sock writers and what serializes each;
    patch 5 refreshes the rds_conn_path_reset() header, the
    acquire_refill() comment in rds_ib_recv_refill() and the
    rds_tcp_reset_callbacks() comment to name the teardown as an
    owner of the bits, and its changelog describes the knock-on
    effect of the parked accept worker on the shared krdsd workqueue
    and why that is a bounded stall rather than a deadlock.

The cong.c wq_has_sleeper() conversion mentioned on the v2 thread is
a pre-existing issue independent of this set and is sent separately.

Questions and comments appreciated!

Thanks,
Allison

[1] https://lore.kernel.org/netdev/20260806072045.1092968-1-achender@kernel.org/
[2] https://lore.kernel.org/netdev/20260814013501.43760-1-achender@kernel.org/
[3] https://lore.kernel.org/netdev/20260816001510.73645-1-achender@kernel.org/

Allison Henderson (3):
  net/rds: use wq_has_sleeper() in release_in_xmit()
  net/rds: clear cp_flags bits individually in rds_conn_path_reset()
  net/rds: acquire RDS_IN_XMIT in rds_tcp_reset_callbacks()

Gerd Rausch (1):
  net/rds: tcp: don't force RDS_CONN_RESETTING over a concurrent
    shutdown

Håkon Bugge (1):
  net/rds: acquire the fastpath locks in rds_conn_shutdown()

 net/rds/connection.c | 46 +++++++++++++++++++----
 net/rds/ib_recv.c    |  4 +-
 net/rds/send.c       | 12 ++++--
 net/rds/tcp.c        | 87 +++++++++++++++++++++++++++++++-------------
 4 files changed, 112 insertions(+), 37 deletions(-)


base-commit: 4e15e89faac9f308baeb01f46c13a051814d2449
-- 
2.25.1

[PATCH net v3 1/5] net/rds: use wq_has_sleeper() in release_in_xmit()

[Allison Henderson]

release_in_xmit() clears RDS_IN_XMIT with clear_bit_unlock() and then
checks waitqueue_active() to decide whether anyone needs waking.
clear_bit_unlock() is only a release operation: it orders the
critical section before the bit clear, but does not order the
subsequent plain load of the wait queue head after it.  The waiter
side does the mirror image - it adds itself to the wait queue and
then tests the bit.  That is the classic store-buffering pattern: the
releasing CPU can read the wait queue as empty while the waiting CPU
still reads the bit as set, so the sleeper is never woken.

The waiters are rds_conn_shutdown() and rds_tcp_reset_callbacks(),
both in uninterruptible wait_event() with no timeout.  A lost wake-up
strands the shutdown worker on its single-threaded workqueue until
some other sender releases the bit again - and on a connection that
is being torn down precisely because it failed, there may never be
another sender.

The barrier used to be there: release_in_xmit() did clear_bit()
followed by smp_mb__after_atomic() until commit 1422f28826d2 ("rds:
introduce acquire/release ordering in acquire/release_in_xmit()")
folded both into clear_bit_unlock(), which strengthened the lock
hand-off but silently dropped the full barrier the wake-up check
depends on.  The refill counterpart, release_refill() in
net/rds/ib_recv.c, still carries its smp_mb__after_atomic() for
exactly this reason.

Use wq_has_sleeper(), which is waitqueue_active() preceded by the
required full barrier.

Fixes: 1422f28826d2 ("rds: introduce acquire/release ordering in acquire/release_in_xmit()")
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
v3: unchanged
v2: unchanged
 net/rds/send.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/net/rds/send.c b/net/rds/send.c
index 15a1b97f13e7..8aad185e4b1a 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -114,8 +114,13 @@ static void release_in_xmit(struct rds_conn_path *cp)
 	 * hot path and finding waiters is very rare.  We don't want to walk
 	 * the system-wide hashed waitqueue buckets in the fast path only to
 	 * almost never find waiters.
+	 *
+	 * wq_has_sleeper() supplies the full barrier that orders the wait
+	 * queue read after the bit clear; clear_bit_unlock() alone is only
+	 * a release and would let this check read a stale empty queue,
+	 * losing the wake-up.
 	 */
-	if (waitqueue_active(&cp->cp_waitq))
+	if (wq_has_sleeper(&cp->cp_waitq))
 		wake_up_all(&cp->cp_waitq);
 }
 
-- 
2.25.1

[PATCH net v3 2/5] net/rds: clear cp_flags bits individually in rds_conn_path_reset()

[Allison Henderson]

rds_conn_path_reset() wipes the whole flag word with a plain
cp->cp_flags = 0 store.  Every other accessor of that word uses
atomic bitops, and some of them can run concurrently with the reset:
RDS_LL_SEND_FULL is set from rds_send_xmit() and cleared from the
transport completion paths, neither of which holds anything that
excludes the shutdown worker.  A plain store racing an atomic
read-modify-write on the same word is a data race, and whichever
side loses has its update silently discarded.

Clear the two bits the reset is actually responsible for instead.
RDS_IN_XMIT and RDS_RECV_REFILL need no store at all here: they
belong to the caller, rds_conn_shutdown(), which waits for both to be
clear before calling the transport shutdown and this reset.

This also gives every bit in cp_flags a single well-defined writer
discipline, which the following patches rely on when they turn
RDS_IN_XMIT and RDS_RECV_REFILL into bit locks held across the
teardown: a blanket store mid-teardown would destroy lock ownership
that an atomic clear preserves.

Oracle UEK carries the same conversion ("net/rds: Preserve essential
connection state flags"), motivated by its asynchronous shutdown
state machine, whose progress and destroy flags must survive the
reset.  UEK's variant also clears RDS_IN_XMIT and RDS_RECV_REFILL
because there the reset runs as the final step of a teardown that
owns both bits, making those clears its unlock.  Upstream that
release belongs in rds_conn_shutdown(): once a later patch in this
series turns the two bits into locks held across the teardown, ending
ownership needs release semantics and a wake-up that a plain clear
inside the reset would not provide.

Based on Oracle UEK commit "net/rds: Preserve essential connection
state flags" by Gerd Rausch.

Fixes: 00e0f34c6166 ("RDS: Connection handling")
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
v3: reword the comment and changelog so they no longer claim a
    quiescence guarantee that only patch 5 delivers; the bits are
    described as belonging to rds_conn_shutdown() and left alone here
v2: add Fixes tag
 net/rds/connection.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index 7c8ab8e973e1..46ac72088f84 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -120,7 +120,15 @@ static void rds_conn_path_reset(struct rds_conn_path *cp)
 
 	rds_stats_inc(s_conn_reset);
 	rds_send_path_reset(cp);
-	cp->cp_flags = 0;
+
+	/* Clear the bits the reset is responsible for individually: a
+	 * blanket cp_flags = 0 is a plain store that can clobber a
+	 * concurrent atomic read-modify-write on the same word.
+	 * RDS_IN_XMIT and RDS_RECV_REFILL belong to the caller,
+	 * rds_conn_shutdown(), and are left alone here.
+	 */
+	clear_bit(RDS_LL_SEND_FULL, &cp->cp_flags);
+	clear_bit(RDS_RECONNECT_PENDING, &cp->cp_flags);
 
 	/* Do not clear next_rx_seq here, else we cannot distinguish
 	 * retransmitted packets from new packets, and will hand all
-- 
2.25.1

[PATCH net v3 3/5] net/rds: tcp: don't force RDS_CONN_RESETTING over a concurrent shutdown

[Allison Henderson]

From: Gerd Rausch <gerd.rausch@oracle.com>

rds_tcp_reset_callbacks() resolves a duelling SYN by storing
RDS_CONN_RESETTING into cp_state unconditionally.  Nothing serializes
that store against the shutdown path: rds_tcp_accept_one() checks
for RDS_CONN_CONNECTING or RDS_CONN_ERROR under t_conn_path_lock, but
neither rds_conn_path_drop(), which forces RDS_CONN_ERROR, nor
rds_conn_shutdown(), which moves the path to RDS_CONN_DISCONNECTING
under cp_cm_lock, takes that lock.  The store can therefore land on
top of a shutdown that is already in progress, or that gets queued
right after the accept-side check.

When it does, the shutdown worker's final DISCONNECTING -> DOWN
transition fails and the path goes through rds_conn_path_error() and
a second drop/shutdown cycle instead of a clean reconnect, tearing
down the socket the accept path has just installed.  Before commit
ad22d24be635 ("net/rds: No shortcut out of RDS_CONN_ERROR") a path
found in RDS_CONN_RESETTING even made rds_conn_shutdown() bail out
altogether.

Make the transition conditional: move CONNECTING -> RESETTING (or
stay in RESETTING from an earlier duel), and drop the path in any
other state.  The drop has side effects of its own: it replaces the
shutdown's RDS_CONN_DISCONNECTING (or RDS_CONN_ERROR) with
RDS_CONN_ERROR and queues one more cp_down_w run.  The difference is
that rds_conn_shutdown() accepts RDS_CONN_ERROR in its final
transition to RDS_CONN_DOWN, so the shutdown in flight completes
normally instead of through rds_conn_path_error(); the extra
down-work pass then finds the path already down and falls through to
the reconnect check, or catches a reconnect that has already started
and restarts it.  The accept path still installs the new socket,
rds_connect_path_complete() then fails its RESETTING -> UP transition
and drops it: the raced socket ends up torn down as it does today.

The state can change again between the failed transitions and the
drop.  That is inherent to rds_conn_path_drop(), which the socket
state-change callbacks also call unconditionally, and costs at most
one extra drop/reconnect cycle.

Based on Oracle UEK commit "net/rds: Don't force state
RDS_CONN_RESETTING" by Gerd Rausch.

Fixes: 9c79440e2c5e ("RDS: TCP: fix race windows in send-path quiescence by rds_tcp_accept_one()")
Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
[achender: port to net-next: use the two-argument
 rds_conn_path_transition()/rds_conn_path_drop() and rewrite the
 changelog for the upstream shutdown path]
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
v3: comment and changelog now say what the fallback drop actually
    does (replaces the shutdown's state with RDS_CONN_ERROR, which
    rds_conn_shutdown() tolerates, and queues one more down-work pass)
    instead of claiming the shutdown state is left untouched; note that
    the state can move again before the drop and why that is benign
v2: new in this version, port of an Oracle UEK fix for the
    accept-vs-shutdown state race
 net/rds/tcp.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index b263634ac750..ad14217867a4 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -150,9 +150,22 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 	 * end up deadlocking with tcp_sendmsg(), and the RDS_IN_XMIT
 	 * would not get set. As a result, we set c_state to
 	 * RDS_CONN_RESETTTING, to ensure that rds_tcp_state_change
-	 * cannot mark rds_conn_path_up() in the window before lock_sock()
+	 * cannot mark rds_conn_path_up() in the window before lock_sock().
+	 *
+	 * Only make that transition if the path is still connecting
+	 * (or already resetting from an earlier duel).  A path in any
+	 * other state - typically RDS_CONN_DISCONNECTING or
+	 * RDS_CONN_ERROR with a shutdown in flight - is dropped
+	 * instead.  That still replaces its state, with RDS_CONN_ERROR,
+	 * and queues one more shutdown pass, but rds_conn_shutdown()
+	 * accepts RDS_CONN_ERROR in its final transition to
+	 * RDS_CONN_DOWN, so the shutdown in flight completes normally.
 	 */
-	atomic_set(&cp->cp_state, RDS_CONN_RESETTING);
+	if (!rds_conn_path_transition(cp, RDS_CONN_CONNECTING,
+				      RDS_CONN_RESETTING) &&
+	    !rds_conn_path_transition(cp, RDS_CONN_RESETTING,
+				      RDS_CONN_RESETTING))
+		rds_conn_path_drop(cp, 0);
 	wait_event(cp->cp_waitq, !test_bit(RDS_IN_XMIT, &cp->cp_flags));
 	/* reset receive side state for rds_tcp_data_recv() for osock  */
 	cancel_delayed_work_sync(&cp->cp_send_w);
-- 
2.25.1

[PATCH net v3 4/5] net/rds: acquire RDS_IN_XMIT in rds_tcp_reset_callbacks()

[Allison Henderson]

rds_tcp_reset_callbacks() quiesces the transmit path by setting the
path state to RDS_CONN_RESETTING and then waiting for RDS_IN_XMIT to
be sampled clear before swapping the underlying socket and calling
rds_send_path_reset().

Sampling the bit clear is not the same as owning it: rds_send_xmit()
can re-acquire RDS_IN_XMIT right after the wait_event() returns.  Its
state recheck after taking the lock is a store-buffering pattern (the
resetter writes the state and reads the bit, the sender writes the
bit and reads the state) and acquire_in_xmit() is only an acquire
operation, so on weakly ordered architectures both sides can miss
each other's write and the transmit path then runs concurrently with
rds_send_path_reset() rewriting cp_xmit_* state - which is exactly
what the comment above rds_send_path_reset() tells its callers to
prevent.

Take the lock instead, hold it across the socket swap and
rds_send_path_reset(), and release it with a wake-up at the end.  The
lock-ordering constraint documented above the wait still holds: the
lock is acquired before lock_sock(), so a sender inside tcp_sendmsg()
can never be waited on while we hold the socket lock.

Two details of the old code go away with the same change:

 - t_sock is now read only after the lock is acquired.  The old code
   cached it before waiting; the teardown in rds_conn_shutdown()
   releases that socket and clears t_sock, so a pointer cached before
   the wait can be stale by the time the accept path resumes.  Reading
   it under RDS_IN_XMIT is what makes the exclusion complete once the
   teardown owns the same lock, which the next patch arranges; until
   then the teardown still only samples the bit, and the two paths
   remain as exposed to each other as they are today.

 - The old !osock early path called rds_send_path_reset() with no
   serialization at all.  It now runs under the lock like the normal
   path.  The conditional RDS_CONN_RESETTING transition of the
   previous patch happens before the socket check either way: a path
   found without a socket is either still connecting (its reconnect
   worker blocked on t_conn_path_lock) and legitimately goes
   RESETTING -> UP on the new socket, or it has been torn down
   meanwhile and is dropped.

The in-function comment describing the old wait-based quiesce is
rewritten to describe the lock-based one, and the stale block comment
above the function (which still described a return value and an
incomplete list of t_sock writers) is refreshed to name all three
writers and what serializes each of them.

Fixes: 335b48d980f6 ("RDS: TCP: Add/use rds_tcp_reset_callbacks to reset tcp socket safely")
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
v3: the block comment names all three t_sock writers (including
    rds_tcp_conn_path_connect(), serialized by t_conn_path_lock rather
    than RDS_IN_XMIT); the t_sock read comment no longer implies the
    teardown already owns the lock; changelog scopes that to the next
    patch explicitly
v2: reordered ahead of the rds_conn_shutdown() change so that no
    intermediate commit leaves this function resuming on a released
    socket; block comment above the function refreshed
    v1: https://lore.kernel.org/netdev/20260814013501.43760-5-achender@kernel.org/
    part 2: https://lore.kernel.org/netdev/20260806072045.1092968-5-achender@kernel.org/
 net/rds/tcp.c | 68 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 43 insertions(+), 25 deletions(-)

diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index ad14217867a4..771fc56d6c26 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -115,42 +115,46 @@ void rds_tcp_restore_callbacks(struct socket *sock,
 }
 
 /*
- * rds_tcp_reset_callbacks() switches the to the new sock and
- * returns the existing tc->t_sock.
+ * rds_tcp_reset_callbacks() switches a path to a new socket and
+ * releases the old one it finds in tc->t_sock, resolving a duelling
+ * SYN.
  *
- * The only functions that set tc->t_sock are rds_tcp_set_callbacks
- * and rds_tcp_reset_callbacks.  Send and receive trust that
- * it is set.  The absence of RDS_CONN_UP bit protects those paths
- * from being called while it isn't set.
+ * tc->t_sock is set by rds_tcp_set_callbacks() and cleared by
+ * rds_tcp_restore_callbacks().  Three paths write it: the active
+ * connect in rds_tcp_conn_path_connect(), which sets it and clears it
+ * again on failure and is serialized against the accept path by
+ * t_conn_path_lock; the teardown in rds_tcp_conn_path_shutdown(),
+ * which clears it; and the swap done here, which does both.  Send and
+ * receive trust that it is set: the absence of RDS_CONN_UP protects
+ * those paths from being called while it isn't, and the swap done
+ * here runs under RDS_IN_XMIT so that it cannot interleave with a
+ * sender already inside rds_send_xmit().
  */
 void rds_tcp_reset_callbacks(struct socket *sock,
 			     struct rds_conn_path *cp)
 {
 	struct rds_tcp_connection *tc = cp->cp_transport_data;
-	struct socket *osock = tc->t_sock;
-
-	if (!osock)
-		goto newsock;
+	struct socket *osock;
 
 	/* Need to resolve a duelling SYN between peers.
 	 * We have an outstanding SYN to this peer, which may
 	 * potentially have transitioned to the RDS_CONN_UP state,
 	 * so we must quiesce any send threads before resetting
-	 * cp_transport_data. We quiesce these threads by setting
-	 * cp_state to something other than RDS_CONN_UP, and then
-	 * waiting for any existing threads in rds_send_xmit to
-	 * complete release_in_xmit(). (Subsequent threads entering
-	 * rds_send_xmit() will bail on !rds_conn_up().
+	 * cp_transport_data.  Setting cp_state to something other
+	 * than RDS_CONN_UP stops new senders, and owning RDS_IN_XMIT
+	 * excludes any thread already inside rds_send_xmit() for the
+	 * whole socket swap and the rds_send_path_reset() below.
 	 *
-	 * However an incoming syn-ack at this point would end up
-	 * marking the conn as RDS_CONN_UP, and would again permit
-	 * rds_send_xmi() threads through, so ideally we would
-	 * synchronize on RDS_CONN_UP after lock_sock(), but cannot
-	 * do that: waiting on !RDS_IN_XMIT after lock_sock() may
-	 * end up deadlocking with tcp_sendmsg(), and the RDS_IN_XMIT
-	 * would not get set. As a result, we set c_state to
-	 * RDS_CONN_RESETTTING, to ensure that rds_tcp_state_change
-	 * cannot mark rds_conn_path_up() in the window before lock_sock().
+	 * An incoming syn-ack at this point would end up marking the
+	 * conn as RDS_CONN_UP, and would again permit rds_send_xmit()
+	 * threads through, so ideally we would synchronize on
+	 * RDS_CONN_UP after lock_sock(), but cannot do that: acquiring
+	 * RDS_IN_XMIT after lock_sock() may end up deadlocking with
+	 * tcp_sendmsg(), which takes the socket lock while holding
+	 * RDS_IN_XMIT.  As a result, we set c_state to
+	 * RDS_CONN_RESETTING, to ensure that rds_tcp_state_change
+	 * cannot mark rds_conn_path_up() in the window before
+	 * lock_sock().
 	 *
 	 * Only make that transition if the path is still connecting
 	 * (or already resetting from an earlier duel).  A path in any
@@ -166,7 +170,18 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 	    !rds_conn_path_transition(cp, RDS_CONN_RESETTING,
 				      RDS_CONN_RESETTING))
 		rds_conn_path_drop(cp, 0);
-	wait_event(cp->cp_waitq, !test_bit(RDS_IN_XMIT, &cp->cp_flags));
+	wait_event(cp->cp_waitq,
+		   !test_and_set_bit_lock(RDS_IN_XMIT, &cp->cp_flags));
+
+	/* Read t_sock only while owning RDS_IN_XMIT, never before the
+	 * wait: the teardown in rds_conn_shutdown() releases the old
+	 * socket and clears t_sock, so a pointer sampled earlier can
+	 * be stale by the time we wake up.
+	 */
+	osock = tc->t_sock;
+	if (!osock)
+		goto newsock;
+
 	/* reset receive side state for rds_tcp_data_recv() for osock  */
 	cancel_delayed_work_sync(&cp->cp_send_w);
 	cancel_delayed_work_sync(&cp->cp_recv_w);
@@ -185,6 +200,9 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 	lock_sock(sock->sk);
 	rds_tcp_set_callbacks(sock, cp);
 	release_sock(sock->sk);
+
+	clear_bit_unlock(RDS_IN_XMIT, &cp->cp_flags);
+	wake_up_all(&cp->cp_waitq);
 }
 
 /* Add tc to rds_tcp_tc_list and set tc->t_sock. See comments
-- 
2.25.1

[PATCH net v3 5/5] net/rds: acquire the fastpath locks in rds_conn_shutdown()

[Allison Henderson]

From: Håkon Bugge <haakon.bugge@oracle.com>

rds_conn_shutdown() quiesces the transmit and receive-refill paths by
waiting for RDS_IN_XMIT and RDS_RECV_REFILL to be sampled clear, and
then runs the transport shutdown and rds_conn_path_reset().  Sampling
the bits clear is not the same as owning them: the moment after the
wait_event() returns, rds_send_xmit() can re-acquire RDS_IN_XMIT (or
rds_ib_recv_refill() can re-acquire RDS_RECV_REFILL) and run
concurrently with the teardown.

The sender does recheck the connection state after taking the lock,
but that recheck is a classic store-buffering pattern: teardown writes
the state and reads the bit while the sender writes the bit and reads
the state.  acquire_in_xmit() is only an acquire operation, so on
weakly ordered architectures both sides can miss each other's write,
and the transmit path then runs while the transport zeroes its rings
(e.g. rds_ib_ring_init()) and rds_send_path_reset() rewrites the
transmit state under it.

Oracle UEK fixed the same class of crashes - a 14-year tail of
BUG_ON()s in rds_ib_sub_signaled(), unexpected op-codes and NULL
dereferences in rds_ib_send_cqe_handler() during failover testing -
by making the teardown path *acquire* the fastpath bit locks instead
of testing them ("rds: Make sure transmit path and connection
tear-down does not run concurrently").  Ownership of a single word is
decided by RMW atomicity, so no cross-variable ordering is needed.

Do the same here: take both locks before calling the transport
shutdown, hold them across rds_conn_path_reset(), and release them
explicitly with a wake-up afterwards.  Both are released with
clear_bit_unlock(), so that the ring re-initialization done by the
transport shutdown and the transmit state rewritten by
rds_send_path_reset() are ordered before either bit is seen clear by
the next acquire_in_xmit() or acquire_refill().

The fastpath users of these bits - rds_send_xmit() and
rds_ib_recv_refill() - are trylock style and back off while teardown
owns the locks, so no new lock dependency is introduced for them.
rds_tcp_reset_callbacks() is different: since the previous patch it
acquires RDS_IN_XMIT as well, and it blocks doing so, so its wait now
spans the teardown instead of at most one send batch.  That waiter
runs from rds_tcp_accept_one() on the single-threaded krdsd workqueue
and holds rds_tcp_accept_lock and t_conn_path_lock while it waits, so
a duelling SYN accepted while its path is being torn down parks
accept processing for the duration of the teardown - for TCP bounded
by the (up to 5 s) drain loop in rds_tcp_conn_path_shutdown().  The
window is narrow: the accept-side state check has to pass before the
teardown moves the path to RDS_CONN_DISCONNECTING.

Because krdsd is a single global workqueue, everything else queued
there - accept processing for other connections and network
namespaces, and the flush_workqueue(rds_wq) in rds_tcp_listen_stop()
during namespace teardown - waits behind the parked accept worker for
that time.  It cannot deadlock: the teardown runs on the path's own
ordered workqueue and never waits on krdsd, so it always completes
the drain and releases the bit (on the allocation-failure fallback
where a path shares rds_wq, the two work items simply serialize).
Nor is the blocking wait itself new: rds_tcp_reset_callbacks() has
waited on RDS_IN_XMIT from the krdsd work item since
commit 335b48d980f6 ("RDS: TCP: Add/use rds_tcp_reset_callbacks to
reset tcp socket safely"); this patch stretches its worst case from
a sender's batch to the teardown's drain.  The alternative to parking
is the accept path racing the teardown, which is what these patches
close; making the teardown itself non-blocking is a separate item.

One observable side effect: the SENDING flag reported by rds-info has
always mirrored RDS_IN_XMIT, so it now also covers the window where
teardown owns the bit.

The comments that describe the old sample-based handshake or name
rds_send_xmit() as the only other holder of these bits - in
rds_send_xmit(), above rds_conn_path_reset(), in rds_ib_recv_refill()
and in rds_tcp_reset_callbacks() - are updated to match.

Fixes: 0f4b1c7e89e6 ("rds: fix rds_send_xmit() serialization")
Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
[achender: reimplement for net-next shutdown path: acquire the existing
 RDS_IN_XMIT/RDS_RECV_REFILL bit locks in rds_conn_shutdown() and release
 after teardown; update comments and commit message]
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
v3: release RDS_RECV_REFILL with clear_bit_unlock() like RDS_IN_XMIT;
    refresh the rds_conn_path_reset() header comment, the
    acquire_refill() comment in rds_ib_recv_refill() and the
    rds_tcp_reset_callbacks() comment to name the teardown as an owner
    of the bits; changelog describes the knock-on effect of the parked
    accept worker on the shared krdsd workqueue (bounded stall, not a
    deadlock) and that the blocking wait itself predates this series
v2: reordered after the rds_tcp_reset_callbacks() change; changelog
    describes the accept-side waiter parking on krdsd for the duration
    of a TCP teardown
    v1: https://lore.kernel.org/netdev/20260814013501.43760-4-achender@kernel.org/
    part 2: https://lore.kernel.org/netdev/20260806072045.1092968-4-achender@kernel.org/
 net/rds/connection.c | 40 ++++++++++++++++++++++++++++++++--------
 net/rds/ib_recv.c    |  4 +++-
 net/rds/send.c       |  5 +++--
 net/rds/tcp.c        | 10 +++++++---
 4 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index 46ac72088f84..fbbac55a0e81 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -106,10 +106,12 @@ static struct rds_connection *rds_conn_lookup(struct net *net,
 }
 
 /*
- * This is called by transports as they're bringing down a connection.
- * It clears partial message state so that the transport can start sending
- * and receiving over this connection again in the future.  It is up to
- * the transport to have serialized this call with its send and recv.
+ * This is called by rds_conn_shutdown() once the transport has brought
+ * a path down.  It clears partial message state so that the transport
+ * can start sending and receiving over this path again in the future.
+ * The caller owns RDS_IN_XMIT and RDS_RECV_REFILL across this call,
+ * which is what serializes it against the send and receive-refill
+ * paths.
  */
 static void rds_conn_path_reset(struct rds_conn_path *cp)
 {
@@ -124,8 +126,9 @@ static void rds_conn_path_reset(struct rds_conn_path *cp)
 	/* Clear the bits the reset is responsible for individually: a
 	 * blanket cp_flags = 0 is a plain store that can clobber a
 	 * concurrent atomic read-modify-write on the same word.
-	 * RDS_IN_XMIT and RDS_RECV_REFILL belong to the caller,
-	 * rds_conn_shutdown(), and are left alone here.
+	 * RDS_IN_XMIT and RDS_RECV_REFILL are held as locks by the
+	 * caller, rds_conn_shutdown(), which releases them once the
+	 * teardown is complete.
 	 */
 	clear_bit(RDS_LL_SEND_FULL, &cp->cp_flags);
 	clear_bit(RDS_RECONNECT_PENDING, &cp->cp_flags);
@@ -414,14 +417,35 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
 		}
 		mutex_unlock(&cp->cp_cm_lock);
 
+		/* Quiesce the transmit and receive-refill paths by
+		 * acquiring their bit locks, not merely waiting for
+		 * them to be released: with a plain wait, either path
+		 * can re-take its lock the instant after we sample it
+		 * clear and then run concurrently with the transport
+		 * shutdown and the path reset below.  Holding both
+		 * locks across the teardown makes that structurally
+		 * impossible.
+		 */
 		wait_event(cp->cp_waitq,
-			   !test_bit(RDS_IN_XMIT, &cp->cp_flags));
+			   !test_and_set_bit_lock(RDS_IN_XMIT, &cp->cp_flags));
 		wait_event(cp->cp_waitq,
-			   !test_bit(RDS_RECV_REFILL, &cp->cp_flags));
+			   !test_and_set_bit(RDS_RECV_REFILL, &cp->cp_flags));
 
 		conn->c_trans->conn_path_shutdown(cp);
 		rds_conn_path_reset(cp);
 
+		/* Release the two locks and wake any waiter (e.g.
+		 * rds_tcp_reset_callbacks()) that blocked on them while
+		 * we held them.  The unlock orders the transport's ring
+		 * re-initialization and the path reset above before
+		 * either bit is seen clear.  rds_conn_path_reset() leaves
+		 * both bits alone: ownership ends here, not inside the
+		 * reset.
+		 */
+		clear_bit_unlock(RDS_IN_XMIT, &cp->cp_flags);
+		clear_bit_unlock(RDS_RECV_REFILL, &cp->cp_flags);
+		wake_up_all(&cp->cp_waitq);
+
 		if (!rds_conn_path_transition(cp, RDS_CONN_DISCONNECTING,
 					      RDS_CONN_DOWN) &&
 		    !rds_conn_path_transition(cp, RDS_CONN_ERROR,
diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index 357128d34a54..406c309b0920 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -392,7 +392,9 @@ void rds_ib_recv_refill(struct rds_connection *conn, int prefill, gfp_t gfp)
 
 	/* the goal here is to just make sure that someone, somewhere
 	 * is posting buffers.  If we can't get the refill lock,
-	 * let them do their thing
+	 * let them do their thing.  The holder may also be
+	 * rds_conn_shutdown() tearing the path down, in which case
+	 * there is nothing to post.
 	 */
 	if (!acquire_refill(conn))
 		return;
diff --git a/net/rds/send.c b/net/rds/send.c
index 8aad185e4b1a..b90e0586f818 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -244,8 +244,9 @@ int rds_send_xmit(struct rds_conn_path *cp)
 	WRITE_ONCE(cp->cp_send_gen, send_gen);
 
 	/*
-	 * rds_conn_shutdown() sets the conn state and then tests RDS_IN_XMIT,
-	 * we do the opposite to avoid races.
+	 * rds_conn_shutdown() sets the conn state and then acquires
+	 * RDS_IN_XMIT; we take the lock first and then check the state,
+	 * so one of us is guaranteed to see the other's update.
 	 */
 	if (!rds_conn_path_up(cp)) {
 		release_in_xmit(cp);
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 771fc56d6c26..e09ac071066e 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -142,8 +142,10 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 	 * so we must quiesce any send threads before resetting
 	 * cp_transport_data.  Setting cp_state to something other
 	 * than RDS_CONN_UP stops new senders, and owning RDS_IN_XMIT
-	 * excludes any thread already inside rds_send_xmit() for the
-	 * whole socket swap and the rds_send_path_reset() below.
+	 * excludes any thread already inside rds_send_xmit() - or a
+	 * teardown in rds_conn_shutdown(), which holds the same lock
+	 * for the duration of the transport shutdown - for the whole
+	 * socket swap and the rds_send_path_reset() below.
 	 *
 	 * An incoming syn-ack at this point would end up marking the
 	 * conn as RDS_CONN_UP, and would again permit rds_send_xmit()
@@ -176,7 +178,9 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 	/* Read t_sock only while owning RDS_IN_XMIT, never before the
 	 * wait: the teardown in rds_conn_shutdown() releases the old
 	 * socket and clears t_sock, so a pointer sampled earlier can
-	 * be stale by the time we wake up.
+	 * be stale by the time we wake up.  The teardown holds the
+	 * same lock while it does so, so what we read here cannot
+	 * change under us until we release it.
 	 */
 	osock = tc->t_sock;
 	if (!osock)
-- 
2.25.1

Re: [PATCH net v3 0/5] net/rds: own the fastpath locks across connection teardown

[Allison Henderson]

On Fri, 2026-08-21 at 22:24 -0700, Allison Henderson wrote:
> Hi all,
> 
> This is v3 of the follow-up set to "net/rds: Bug fix ports, part 2"
> [1] (v1 of this set is at [2], v2 at [3]).  During review of part 2,
> the later half of that series needed more work than a respin, so it
> was split off into this set together with the companion fixes
> identified along the way.  As discussed on the v2 thread, it is now
> targeted at net.
> 
> RDS connection teardown quiesces the transmit and receive-refill fast
> paths by waiting for the RDS_IN_XMIT/RDS_RECV_REFILL bits to be
> sampled clear.  Sampling a bit clear is not owning it: the fast path
> can re-take its bit right after the wait returns and then run
> concurrently with the transport shutdown and the send-state reset.
> Oracle UEK closed this by making teardown acquire the bits as locks
> ("rds: Make sure transmit path and connection tear-down does not run
> concurrently"); patches 4 and 5 do the same for the two
> rds_send_path_reset() call sites upstream.  These two are effectively
> v3 of patches 4 and 3 of "net/rds: Bug fix ports, part 2" [1].
> 
> Making teardown block on the bits as locks promotes three latent
> ordering bugs from rare to load-bearing, so they are fixed first:
> 
>   Patch 1: release_in_xmit() checks waitqueue_active() after
>   clear_bit_unlock(), which does not order that read; the wake-up of
>   the (now uninterruptible, untimed) teardown wait can be lost.  Use
>   wq_has_sleeper().
> 
>   Patch 2: rds_conn_path_reset() wipes the whole cp_flags word with a
>   plain store.  Once teardown owns bits in that word across the
>   reset, a blanket store would end lock ownership early - and it
>   already races atomic RMWs on the same word today.  Clear the bits
>   the reset is responsible for individually, as Oracle UEK also does.
> 
>   Patch 3: rds_tcp_reset_callbacks() stores RDS_CONN_RESETTING
>   unconditionally, which can overwrite the RDS_CONN_ERROR or
>   RDS_CONN_DISCONNECTING of a shutdown already in progress on the
>   same path and send that shutdown through an extra drop cycle.  Once
>   the accept path can park for the duration of a teardown (patch 5)
>   that window widens, so make the transition conditional first, as
>   Oracle UEK does.
> 
> With those in place, patch 4 converts rds_tcp_reset_callbacks() from
> waiting on RDS_IN_XMIT to acquiring it, holding it across the socket
> swap and rds_send_path_reset(), and patch 5 has rds_conn_shutdown()
> hold both bit locks across the transport shutdown and path reset.
> The order matters: with the accept path owning the lock first, no
> intermediate commit leaves it resuming on a socket pointer that a
> lock-holding teardown has already released.
> 
> [PATCH net 1/5] net/rds: use wq_has_sleeper() in release_in_xmit()
>   Restore full barrier before wake-up checks in release_in_xmit()
> 
> [PATCH net 2/5] net/rds: clear cp_flags bits individually in rds_conn_path_reset()
>   Partial port of commit d04896037223 ("net/rds: Preserve essential connection state flags")
>   https://github.com/oracle/linux-uek/commit/d04896037223
> 
> [PATCH net 3/5] net/rds: tcp: don't force RDS_CONN_RESETTING over a concurrent shutdown
>   Port of commit 72c176a1d9ac ("net/rds: Don't force state RDS_CONN_RESETTING")
>   https://github.com/oracle/linux-uek/commit/72c176a1d9ac
> 
> [PATCH net 4/5] net/rds: acquire RDS_IN_XMIT in rds_tcp_reset_callbacks()
>   Extend the port in patch 5 to the second rds_send_path_reset() call site
> 
> [PATCH net 5/5] net/rds: acquire the fastpath locks in rds_conn_shutdown()
>   Port of commit 2b8aaa4f163b ("rds: Make sure transmit path and connection tear-down does not run concurrently")
>   https://github.com/oracle/linux-uek/commit/2b8aaa4f163b
> 
> Changes since v2 [3]:
>   - Retargeted at net; rebased onto net/main.
>   - No functional changes apart from patch 5 now releasing
>     RDS_RECV_REFILL with clear_bit_unlock(), matching the RDS_IN_XMIT
>     release beside it, so the teardown's ring and send-state writes
>     are ordered before the bit is seen clear.
>   - Comment and changelog corrections from the v2 review pass:
>     patch 2 no longer claims a quiescence guarantee that only patch 5
>     delivers; patch 3 spells out that the fallback drop replaces the
>     shutdown's state with RDS_CONN_ERROR (which rds_conn_shutdown()
>     tolerates) and queues one more down-work pass; patch 4's block
>     comment names all three t_sock writers and what serializes each;
>     patch 5 refreshes the rds_conn_path_reset() header, the
>     acquire_refill() comment in rds_ib_recv_refill() and the
>     rds_tcp_reset_callbacks() comment to name the teardown as an
>     owner of the bits, and its changelog describes the knock-on
>     effect of the parked accept worker on the shared krdsd workqueue
>     and why that is a bounded stall rather than a deadlock.
> 
> The cong.c wq_has_sleeper() conversion mentioned on the v2 thread is
> a pre-existing issue independent of this set and is sent separately.
> 
> Questions and comments appreciated!
> 
> Thanks,
> Allison

Sashiko noticed a socket leak exposed by this set, so i've resent a v4:
https://lore.kernel.org/all/20260824003759.127353-1-achender@kernel.org/

Thanks!
Allison

> 
> [1] https://lore.kernel.org/netdev/20260806072045.1092968-1-achender@kernel.org/
> [2] https://lore.kernel.org/netdev/20260814013501.43760-1-achender@kernel.org/
> [3] https://lore.kernel.org/netdev/20260816001510.73645-1-achender@kernel.org/
> 
> Allison Henderson (3):
>   net/rds: use wq_has_sleeper() in release_in_xmit()
>   net/rds: clear cp_flags bits individually in rds_conn_path_reset()
>   net/rds: acquire RDS_IN_XMIT in rds_tcp_reset_callbacks()
> 
> Gerd Rausch (1):
>   net/rds: tcp: don't force RDS_CONN_RESETTING over a concurrent
>     shutdown
> 
> Håkon Bugge (1):
>   net/rds: acquire the fastpath locks in rds_conn_shutdown()
> 
>  net/rds/connection.c | 46 +++++++++++++++++++----
>  net/rds/ib_recv.c    |  4 +-
>  net/rds/send.c       | 12 ++++--
>  net/rds/tcp.c        | 87 +++++++++++++++++++++++++++++++-------------
>  4 files changed, 112 insertions(+), 37 deletions(-)
> 
> 
> base-commit: 4e15e89faac9f308baeb01f46c13a051814d2449