Skip to contentSkip to conversation

net/rds: own the fastpath locks across connection teardown

PATCH Applied Allison Henderson 2 participants 9 messages Aug 28 – Sep 3

Subsystems (4)
lore
Share & tools
lore

Allison Henderson

author maintainer
#0 lore raw

v5 cover net/rds: own the fastpath locks across connection teardown

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/[email protected]/
[2] https://lore.kernel.org/netdev/[email protected]/
[3] https://lore.kernel.org/netdev/[email protected]/
[4] https://lore.kernel.org/netdev/[email protected]/
[5] https://lore.kernel.org/netdev/[email protected]/

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

Signature · 1 line
2.25.1
author maintainer
#1 lore raw

PATCH v5 1/7 net/rds: use wq_has_sleeper() in release_in_xmit()

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

---
v5: no change since v4.

 net/rds/send.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
net/rds/send.c +6 -1
index 15a1b97f13e7..8aad185e4b1a 100644
@@ -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);
 }
 
Signature · 1 line
2.25.1
author maintainer
#2 lore raw

PATCH v5 2/7 net/rds: use clear_bit_unlock() in release_refill()

In reply to 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

---
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(-)
net/rds/ib_recv.c +2 -3
index 357128d34a54..a6983861eec7 100644
@@ -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);
 }
 
Signature · 1 line
2.25.1
author maintainer
#3 lore raw

PATCH v5 3/7 net/rds: clear cp_flags bits individually in rds_conn_path_reset()

In reply to 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

---
v5: no change since v4.

 net/rds/connection.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
net/rds/connection.c +9 -1
index 7c8ab8e973e1..46ac72088f84 100644
@@ -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
Signature · 1 line
2.25.1
author maintainer
#4 lore raw

PATCH v5 4/7 net/rds: tcp: don't force RDS_CONN_RESETTING over a concurrent shutdown

In reply to Allison Henderson

From: Gerd Rausch <[email protected]>

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

[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

---
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(-)
net/rds/tcp.c +15 -2
index b263634ac750..ad14217867a4 100644
@@ -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);
net/rds/tcp_listen.c +5 -1
index a3db9b057084..13fa60c1985b 100644
@@ -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);
Signature · 1 line
2.25.1
author maintainer
#5 lore raw

PATCH v5 5/7 net/rds: acquire RDS_IN_XMIT in rds_tcp_reset_callbacks()

In reply to 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

---
v5: no change since v4.

 net/rds/tcp.c | 70 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 45 insertions(+), 25 deletions(-)
net/rds/tcp.c +45 -25
index ad14217867a4..f4c83e368390 100644
@@ -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
Signature · 1 line
2.25.1
author maintainer
#6 lore raw

PATCH v5 6/7 net/rds: acquire the fastpath locks in rds_conn_shutdown()

In reply to Allison Henderson

From: Håkon Bugge <[email protected]>

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

[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

---
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(-)
net/rds/connection.c +32 -8
index 46ac72088f84..fbbac55a0e81 100644
@@ -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,
net/rds/ib_recv.c +3 -1
index a6983861eec7..bd6cb3ffaa57 100644
@@ -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;
net/rds/send.c +5 -2
index 8aad185e4b1a..1afa981e5c06 100644
@@ -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);
net/rds/tcp.c +15 -4
index f4c83e368390..69c6d3145b5a 100644
@@ -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);
Signature · 1 line
2.25.1
author maintainer
#7 lore raw

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

In reply to 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

---
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(-)
net/rds/connection.c +32 -11
index fbbac55a0e81..b6c4beb50eaf 100644
@@ -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 "
net/rds/tcp.c +6 -3
index 69c6d3145b5a..774a71f88d37 100644
@@ -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) &&
Signature · 1 line
2.25.1
#8 lore raw
In reply to Allison Henderson
This series was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

Quoted 9 lines · 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!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html