[PATCH net v5 0/7] net/rds: own the fastpath locks across connection teardown

[PATCH net v5 0/7] net/rds: own the fastpath locks across connection teardown

[Allison Henderson]

Hi all,

This is v5 of the follow-up set to "net/rds: Bug fix ports, part 2"
[1] (v1 at [2], v2 at [3], v3 at [4], v4 at [5]).  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 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 5 and 6 do the same for the two
rds_send_path_reset() call sites upstream.

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

  Patches 1 and 2 fix the release side of the two bit locks.
  release_in_xmit() and release_refill() both clear their bit and then
  test for waiters, but the barrier is on the wrong side of the clear
  to order the critical section's stores before the release, and the
  waiter check does not order against the clear.  Once teardown blocks
  on these bits as locks (uninterruptible and untimed), a lost wake-up
  or a store observed out of order stops mattering only in theory.
  Use clear_bit_unlock() and wq_has_sleeper(), the pattern already
  half-present in release_in_xmit().

  Patch 3: 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 4: 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 6) that
  window widens, so make the transition conditional first, as Oracle
  UEK does.

With those in place, patch 5 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 6 has rds_conn_shutdown()
hold both bit locks across the transport shutdown and path reset.

Patch 7 fixes a pre-existing teardown-state hole that this series
makes easier to hit but did not introduce.  Since commit
e97656d03ca0 the final transition in rds_conn_shutdown() accepts
RDS_CONN_ERROR as well as RDS_CONN_DISCONNECTING, so that a FIN
processed during the teardown does not derail the shutdown.  But
consuming that RDS_CONN_ERROR also consumes the shutdown pass that a
concurrent rds_conn_path_drop() queued along with it.  For a FIN that
is harmless; for rds_tcp_accept_one() it is not.  A drop can race the
accept's DOWN -> CONNECTING path claim, the accept then installs the
freshly accepted socket while the drop's teardown - which sampled
tc->t_sock before that socket existed - is still running,
rds_connect_path_complete() fails and drops the path again, and if the
in-flight shutdown's final transition then swallows that
RDS_CONN_ERROR, the pass that should reap the just-installed socket
finds the path already RDS_CONN_DOWN and does nothing.  The socket is
leaked with its callbacks armed and its rds_tcp_connection still on
rds_tcp_tc_list, the peer sees an established connection that nothing
reads, and the path wedges in RDS_CONN_DOWN.  Make the final
transition DISCONNECTING -> DOWN only and leave a racing drop's
RDS_CONN_ERROR alone, so the pass it queued runs and tears down
whatever attached to the path; the branch quiesces the reconnect
timer itself, since a pending destroy can suppress that pass (see the
changes below).

This surfaced while re-reviewing v3: whether the
release-then-transition ordering in patch 6 could let a woken waiter
install a socket that the teardown then strands.  Chasing that down,
the reachable form of the leak turned out to be the accept-vs-drop
race above rather than the parked-waiter path (a path mid-teardown is
never handed to rds_tcp_reset_callbacks(): rds_tcp_accept_one_path()
only claims a path it can move DOWN -> CONNECTING), and it predates
this series.  It reproduces on an instrumented kernel - a test-only
drop injected into the accept window plus a widened teardown-to-tail
window - as an ESTABLISHED socket with an ever-growing receive queue
on a path stuck down; the same kernel runs clean with patch 7.

The set was built per-commit, run through the rds selftests (tcp and
rdma/rxe), and exercised with a connection/netns churn load and
module load/unload cycles; the patch 7 destroy-window fix was
additionally verified against an instrumented kernel that reproduces
the timer-left-armed WARN deterministically (fires on every destroyed
path unfixed, silent with the fix).

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

[PATCH net 2/7] net/rds: use clear_bit_unlock() in release_refill()
  The matching release-side fix for RDS_RECV_REFILL

[PATCH net 3/7] 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 4/7] 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 5/7] net/rds: acquire RDS_IN_XMIT in rds_tcp_reset_callbacks()
  Extend the port in patch 6 to the second rds_send_path_reset() call site

[PATCH net 6/7] 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

[PATCH net 7/7] net/rds: don't let rds_conn_shutdown() consume a concurrent drop
  Fix the accept-vs-drop socket leak found while reviewing patch 6

Changes since v4 [5]:
  - Patch 7: fixed a hole the v4 review found in the new quiet return:
    it left rds_conn_shutdown() before the tail that cancels
    cp_conn_w, and when a pending destroy had kept the racing drop
    from queueing a successor pass, nothing ever cancelled it -
    rds_conn_path_destroy() would then trip its
    WARN_ON(delayed_work_pending(&cp->cp_conn_w)) and free the path
    with the reconnect timer still armed.  The failure branch now
    quiesces cp_conn_w and clears RDS_RECONNECT_PENDING itself before
    returning, so every exit of a shutdown pass leaves the timer
    idle no matter which pass completes the transition.  Reproduced
    on an instrumented kernel (deterministic destroy-window drop) as
    the destroy-time WARN_ON firing with the timer armed; the same
    kernel runs clean with the fix.  The comment in the branch also
    no longer claims a module unload takes the noisy path (an
    unload's drop stores RDS_CONN_ERROR and takes the quiet return
    like everything else), and notes the remaining noisy branch is
    defensive.
  - Patch 6: changelog corrections from the review - the backport
    paragraph now names all four prerequisites (patches 2, 3, 5 and
    the follow-up patch 7); the krdsd paragraph no longer claims the
    teardown "never waits on krdsd" and instead explains why the
    mutual waits cannot cycle (the holder's sync cancels target
    pending-at-most items on the ordered cp_wq); the "up to 5 s"
    bound is scoped to TCP, with a note that IB's uncapped drain has
    no blocking waiter to park.  Two comments updated to match: the
    send.c ordering comment now attributes the guarantee to the
    atomic RMW on the cp_flags word rather than cross-variable
    ordering, and the sync cancels in rds_tcp_reset_callbacks() now
    document the ordered-cp_wq reliance.
  - Patch 4: the call-site comment in rds_tcp_accept_one() no longer
    promises rds_connect_path_complete() will mark the path
    RDS_CONN_UP; it names the drop-and-reap outcome too.
    Investigated possible patch re-ordering with patches 5/6, but
    retaining this ordering to avoid widening the pre-existing race
    window betwen accept and shutdown before the conditional transition
    is in place.
  - Patch 2: changelog now names the companion and dependent patches
    by subject instead of series position, and describes the
    teardown's acquire side as of the end of the series.
  - Patches 1, 3 and 5 are unchanged; patches 1-6 are code-identical
    to v4 apart from the two comments noted above.

The cong.c wq_has_sleeper() conversion mentioned on the v2 thread was
sent separately and has already been reviewed.

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/
[4] https://lore.kernel.org/netdev/20260822052459.88017-1-achender@kernel.org/
[5] https://lore.kernel.org/netdev/20260824003759.127353-1-achender@kernel.org/

Allison Henderson (5):
  net/rds: use wq_has_sleeper() in release_in_xmit()
  net/rds: use clear_bit_unlock() in release_refill()
  net/rds: clear cp_flags bits individually in rds_conn_path_reset()
  net/rds: acquire RDS_IN_XMIT in rds_tcp_reset_callbacks()
  net/rds: don't let rds_conn_shutdown() consume a concurrent drop

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 |  89 ++++++++++++++++++++++++++++++--------
 net/rds/ib_recv.c    |   9 ++--
 net/rds/send.c       |  14 ++++--
 net/rds/tcp.c        | 101 +++++++++++++++++++++++++++++++------------
 net/rds/tcp_listen.c |   6 ++-
 5 files changed, 166 insertions(+), 53 deletions(-)


base-commit: 4e15e89faac9f308baeb01f46c13a051814d2449
-- 
2.25.1

[PATCH net v5 1/7] 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>
---
    v5: no change since v4.
 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 v5 2/7] net/rds: use clear_bit_unlock() in release_refill()

[Allison Henderson]

release_refill() drops the RDS_RECV_REFILL bit with a plain
clear_bit().  clear_bit() has no ordering semantics, and the
smp_mb__after_atomic() that follows it sits on the wrong side for a
lock release: it orders the clear against the waitqueue_active() load
below it, but does nothing to order the refill critical section's ring
and descriptor stores before the clear itself.

That matters once connection teardown owns RDS_RECV_REFILL as a lock
across the transport shutdown and path reset, rather than sampling it
clear, which "net/rds: acquire the fastpath locks in
rds_conn_shutdown()" later in this series arranges: on a weakly
ordered architecture the teardown can win the bit and start the
shutdown and reset while some of the refill's stores are not yet
visible to it.  The same gap existed under the sample-based scheme - a
waiter that saw the bit clear had no guarantee it also observed the
refill's stores - but taking the bit as a lock makes the missing
release pairing load-bearing.

Switch to clear_bit_unlock(), which orders the critical section before
the release, and replace the open-coded barrier-plus-waitqueue_active()
with wq_has_sleeper(), whose internal full barrier keeps the
store-buffering guarantee between clearing the bit and checking for
sleepers.  This mirrors what "net/rds: use wq_has_sleeper() in
release_in_xmit()" does for RDS_IN_XMIT.

The fast-path acquire side, acquire_refill(), uses test_and_set_bit(),
a full-barrier RMW that pairs with this release.  The teardown at this
point in the series still samples the bit, so on its own this change
is release-side hardening; the shutdown-conversion patch named above
makes the teardown acquire the bit with the same RMW, completing the
pairing at the end of the series.

Fixes: 73ce4317bf98 ("RDS: make sure we post recv buffers")
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
    v5: changelog now names the companion and dependent patches by
    subject and describes the teardown's acquire side as of the end
    of the series.  No code change.
 net/rds/ib_recv.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index 357128d34a54..a6983861eec7 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -363,15 +363,14 @@ static int acquire_refill(struct rds_connection *conn)
 
 static void release_refill(struct rds_connection *conn)
 {
-	clear_bit(RDS_RECV_REFILL, &conn->c_flags);
-	smp_mb__after_atomic();
+	clear_bit_unlock(RDS_RECV_REFILL, &conn->c_flags);
 
 	/* We don't use wait_on_bit()/wake_up_bit() because our waking is in a
 	 * 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.
 	 */
-	if (waitqueue_active(&conn->c_waitq))
+	if (wq_has_sleeper(&conn->c_waitq))
 		wake_up_all(&conn->c_waitq);
 }
 
-- 
2.25.1

[PATCH net v5 3/7] 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>
---
    v5: no change since v4.
 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 v5 4/7] 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 comment at that call site, which promised that
rds_connect_path_complete() marks the path RDS_CONN_UP, is updated to
name this outcome as well.

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>
---
    v5: the rds_tcp_accept_one() call-site comment no longer promises
    an unconditional transition to RDS_CONN_UP.  No other change.
 net/rds/tcp.c        | 17 +++++++++++++++--
 net/rds/tcp_listen.c |  6 +++++-
 2 files changed, 20 insertions(+), 3 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);
diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index a3db9b057084..13fa60c1985b 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -295,7 +295,11 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
 	if (rs_tcp->t_sock) {
 		/* Duelling SYN has been handled in rds_tcp_accept_one() */
 		rds_tcp_reset_callbacks(new_sock, cp);
-		/* rds_connect_path_complete() marks RDS_CONN_UP */
+		/* rds_connect_path_complete() marks RDS_CONN_UP, or,
+		 * if a concurrent shutdown won the duel, drops the
+		 * path again and the pass that drop queues reaps the
+		 * socket installed above.
+		 */
 		rds_connect_path_complete(cp, RDS_CONN_RESETTING);
 	} else {
 		rds_tcp_set_callbacks(new_sock, cp);
-- 
2.25.1

[PATCH net v5 5/7] 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 four
writers - the connect, accept, teardown and swap paths - 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>
---
    v5: no change since v4.
 net/rds/tcp.c | 70 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 25 deletions(-)

diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index ad14217867a4..f4c83e368390 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -115,42 +115,48 @@ 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().  Four paths write it: the active
+ * connect in rds_tcp_conn_path_connect(), which sets it and clears it
+ * again on failure; the accept path in rds_tcp_accept_one(), which
+ * sets it for a path with no socket yet; the teardown in
+ * rds_tcp_conn_path_shutdown(), which clears it; and the swap done
+ * here, which does both.  The connect and accept paths are serialized
+ * against each other by t_conn_path_lock.  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 +172,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 +202,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 v5 6/7] 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().  An IB
path's drain in rds_ib_conn_path_shutdown() has no round cap, but no
blocking waiter either: rds_tcp_reset_callbacks() is the only blocking
acquirer of these bits and waits only on its own TCP path, and the
fastpaths are trylock-and-back-off on both transports, so a long IB
drain lengthens only that path's own quiesce.  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, although the waits do point at each
other: the teardown blocks until the bit's holder releases it, and
the holder may be that krdsd accept worker.  The holder finishes
without needing anything the teardown owns: the sync cancels
rds_tcp_reset_callbacks() issues target cp_send_w and cp_recv_w on
the path's ordered cp_wq, whose only execution slot is occupied by
the blocked cp_down_w itself, so they are pending at most and cancel
without flushing - a reliance on cp_wq being ordered that is now
noted next to those cancels (on the allocation-failure fallback where
a path shares rds_wq, the 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.

For anyone backporting this patch standalone: it depends on
"net/rds: clear cp_flags bits individually in rds_conn_path_reset()"
and "net/rds: acquire RDS_IN_XMIT in rds_tcp_reset_callbacks()"
earlier in this series.  Without the former, the blanket cp_flags
clear in rds_conn_path_reset() would drop both held bits in the middle
of the teardown; without the latter, rds_tcp_reset_callbacks() would
still sample t_sock without owning RDS_IN_XMIT.  "net/rds: use
clear_bit_unlock() in release_refill()" is needed for the refill
side's release to pair with the acquire added here, and the follow-up
"net/rds: don't let rds_conn_shutdown() consume a concurrent drop"
completes the teardown-state handling for the waiter this patch
parks; a backport should carry all four.

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>
---
    v5: changelog corrections (backport prerequisites, krdsd wait
    reasoning, TCP scope of the drain bound); the send.c ordering
    comment and the sync-cancel comment in rds_tcp_reset_callbacks()
    updated to match.  No functional change.
 net/rds/connection.c | 40 ++++++++++++++++++++++++++++++++--------
 net/rds/ib_recv.c    |  4 +++-
 net/rds/send.c       |  7 +++++--
 net/rds/tcp.c        | 19 +++++++++++++++----
 4 files changed, 55 insertions(+), 15 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 a6983861eec7..bd6cb3ffaa57 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -391,7 +391,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..1afa981e5c06 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -244,8 +244,11 @@ 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.
+	 * Ownership is decided by the atomic RMW on the cp_flags word:
+	 * if the teardown won the bit we back off here, and if we won
+	 * it the teardown waits until we release it.
 	 */
 	if (!rds_conn_path_up(cp)) {
 		release_in_xmit(cp);
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index f4c83e368390..69c6d3145b5a 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -144,8 +144,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()
@@ -178,13 +180,22 @@ 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)
 		goto newsock;
 
-	/* reset receive side state for rds_tcp_data_recv() for osock  */
+	/* reset receive side state for rds_tcp_data_recv() for osock.
+	 *
+	 * The sync cancels while owning RDS_IN_XMIT rely on cp_wq
+	 * being ordered: a teardown blocked on the bit occupies
+	 * cp_wq's only execution slot, so cp_send_w and cp_recv_w are
+	 * pending at most and the cancels never flush.  Nothing here
+	 * may flush or wait on cp_wq itself.
+	 */
 	cancel_delayed_work_sync(&cp->cp_send_w);
 	cancel_delayed_work_sync(&cp->cp_recv_w);
 	lock_sock(osock->sk);
-- 
2.25.1

[PATCH net v5 7/7] net/rds: don't let rds_conn_shutdown() consume a concurrent drop

[Allison Henderson]

rds_conn_shutdown() finishes by moving the path from
RDS_CONN_DISCONNECTING to RDS_CONN_DOWN, and also accepts
RDS_CONN_ERROR as the starting state of that final transition, so that
a FIN processed in softirq context during the teardown does not derail
the shutdown into a noisy error path.

But consuming that RDS_CONN_ERROR also consumes the shutdown pass that
came with it: rds_conn_path_drop() sets RDS_CONN_ERROR and then queues
cp_down_w, and a pass that starts on a path already in RDS_CONN_DOWN
is a no-op.  For the FIN case that is harmless - the socket the FIN
arrived on is the very socket the teardown just released.  It is not
harmless for a dropper that attached something to the path first.

rds_tcp_accept_one() is such a dropper.  Its path claim in
rds_tcp_accept_one_path() transitions RDS_CONN_DOWN ->
RDS_CONN_CONNECTING, and a concurrent drop - a FIN on a previous
socket in softirq context, an administrative reset - can put the path
into RDS_CONN_ERROR between that claim and the state check that
follows, which accepts RDS_CONN_ERROR.  The accept then installs the
freshly accepted socket with rds_tcp_set_callbacks() while the queued
teardown - which sampled tc->t_sock before this socket existed - is
still running.  rds_connect_path_complete() fails its transition to
RDS_CONN_UP and drops the path again, queueing the pass that should
reap the socket it just installed.  If the in-flight shutdown's final
transition consumes that drop's RDS_CONN_ERROR, the queued pass finds
the path in RDS_CONN_DOWN and does nothing.  The installed socket is
never torn down: it sits established with its callbacks armed and its
rds_tcp_connection on rds_tcp_tc_list, the peer sees a connection that
nothing ever reads, and the path is wedged in RDS_CONN_DOWN until some
later event drops it again.  Reproduced with widened race windows as
an ever-growing receive queue on a socket owned by a path stuck in
RDS_CONN_DOWN, with the peer's send path wedged behind it.

Make the final transition only DISCONNECTING -> DOWN.  If it fails
because the path is in RDS_CONN_ERROR, a drop raced the teardown:
cancel the reconnect timer and clear RDS_RECONNECT_PENDING - the one
piece of the skipped tail that must not be left behind - and return,
letting the pass the drop queued finish the job: it tears down
whatever attached to the path in the meantime, completes the
transition to RDS_CONN_DOWN, and re-arms the reconnect from its own
tail.

The timer quiesce in that branch matters because the racing drop does
not always queue that pass: rds_conn_path_drop() returns without
queueing when a destroy is pending - exactly the situation during a
netns teardown or module unload, when a FIN on the dying socket is
processed while rds_conn_path_destroy() flushes cp_down_w.  If the
flushed pass is the one that takes this return, no later pass exists,
and rds_conn_path_destroy() would find cp_conn_w still armed
(WARN_ON) and then free a path whose reconnect timer can still fire.
With the cancel in the branch, every exit of a shutdown pass leaves
the timer quiesced no matter which pass completes the transition.

The FIN case keeps making progress, one pass later and still without
noisy logging.  Any other state keeps today's rds_conn_path_error()
handling; no current cp_state writer can leave a DISCONNECTING path
in anything but RDS_CONN_ERROR (every other writer is a cmpxchg from
a non-DISCONNECTING state), so that branch is defensive.

On kernels without the preceding patches the same hazard exists with
the sample-based quiesce; the fix applies there equally.

Fixes: e97656d03ca0 ("rds: tcp: allow progress of rds_conn_shutdown if the rds_connection is marked ERROR by an intervening FIN")
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
    v5: the failure branch now cancels cp_conn_w and clears
    RDS_RECONNECT_PENDING before returning, closing the
    destroy-pending hole found in the v4 review; comments updated to
    match (the module-unload example was wrong - an unload's drop
    takes the quiet return too).
 net/rds/connection.c | 43 ++++++++++++++++++++++++++++++++-----------
 net/rds/tcp.c        |  9 ++++++---
 2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index fbbac55a0e81..b6c4beb50eaf 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -447,19 +447,40 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
 		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,
 					      RDS_CONN_DOWN)) {
-			/* This can happen - eg when we're in the middle of tearing
-			 * down the connection, and someone unloads the rds module.
-			 * Quite reproducible with loopback connections.
-			 * Mostly harmless.
+			/* The path was dropped again while we tore it
+			 * down: by a socket state-change callback in
+			 * irq context on receipt of a FIN, or by an
+			 * accept that claimed the path just before a
+			 * drop put it back to RDS_CONN_ERROR and then
+			 * installed a fresh socket on it.  Unless a
+			 * pending destroy suppressed it, the drop also
+			 * queued another shutdown pass, and that pass
+			 * must run, because it is what tears down
+			 * whatever attached to the path after the
+			 * transport shutdown above sampled its state.
+			 * Consuming the RDS_CONN_ERROR here would turn
+			 * that pass into a no-op: leave the state
+			 * alone, and let the pass finish the job.
 			 *
-			 * Note that this also happens with rds-tcp because
-			 * we could have triggered rds_conn_path_drop in irq
-			 * mode from rds_tcp_state change on the receipt of
-			 * a FIN, thus we need to recheck for RDS_CONN_ERROR
-			 * here.
+			 * 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
+			 * pass, when there is one, re-arms the
+			 * reconnect from its own tail.
+			 */
+			cancel_delayed_work_sync(&cp->cp_conn_w);
+			clear_bit(RDS_RECONNECT_PENDING, &cp->cp_flags);
+
+			if (rds_conn_path_state(cp) == RDS_CONN_ERROR)
+				return;
+			/* No current cp_state writer leaves a
+			 * DISCONNECTING path in any state but
+			 * RDS_CONN_ERROR; report loudly if one ever
+			 * does.
 			 */
 			rds_conn_path_error(cp, "%s: failed to transition "
 					    "to state DOWN, current state "
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 69c6d3145b5a..774a71f88d37 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -165,9 +165,12 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 	 * 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.
+	 * and, unless a pending destroy is about to reap the whole
+	 * connection anyway, queues one more shutdown pass.  A shutdown
+	 * already in flight leaves that RDS_CONN_ERROR alone when it
+	 * finishes; the queued pass then completes the transition to
+	 * RDS_CONN_DOWN and tears down anything that attached to the
+	 * path in the meantime.
 	 */
 	if (!rds_conn_path_transition(cp, RDS_CONN_CONNECTING,
 				      RDS_CONN_RESETTING) &&
-- 
2.25.1

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

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 28 Aug 2026 15:39:14 -0700 you wrote:
> Hi all,
> 
> This is v5 of the follow-up set to "net/rds: Bug fix ports, part 2"
> [1] (v1 at [2], v2 at [3], v3 at [4], v4 at [5]).  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 targeted at net.
> 
> [...]

Here is the summary with links:
  - [net,v5,1/7] net/rds: use wq_has_sleeper() in release_in_xmit()
    https://git.kernel.org/netdev/net/c/6d0c8b707391
  - [net,v5,2/7] net/rds: use clear_bit_unlock() in release_refill()
    https://git.kernel.org/netdev/net/c/17c4476dbb9c
  - [net,v5,3/7] net/rds: clear cp_flags bits individually in rds_conn_path_reset()
    https://git.kernel.org/netdev/net/c/103c4b13c4f5
  - [net,v5,4/7] net/rds: tcp: don't force RDS_CONN_RESETTING over a concurrent shutdown
    https://git.kernel.org/netdev/net/c/e8e60d74fec4
  - [net,v5,5/7] net/rds: acquire RDS_IN_XMIT in rds_tcp_reset_callbacks()
    https://git.kernel.org/netdev/net/c/02c5f9dc2efd
  - [net,v5,6/7] net/rds: acquire the fastpath locks in rds_conn_shutdown()
    https://git.kernel.org/netdev/net/c/813f3582ac7a
  - [net,v5,7/7] net/rds: don't let rds_conn_shutdown() consume a concurrent drop
    https://git.kernel.org/netdev/net/c/260c6308fe2e

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html