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

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

[Allison Henderson]

Hi all,

This is v4 of the follow-up set to "net/rds: Bug fix ports, part 2"
[1] (v1 at [2], v2 at [3], v3 at [4]).  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 is new in v4 and 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.

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) with a clean dmesg, and exercised with a connection/netns
churn load and module load/unload cycles.

[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 v3 [4]:
  - New patch 2 (release_refill clear_bit_unlock/wq_has_sleeper),
    completing the release-side pairing for the second bit lock that
    patch 6 now takes; noticed while re-reviewing patch 6.
  - New patch 7 (don't consume a concurrent drop), fixing the
    accept-vs-drop socket leak described above; found while
    re-reviewing patch 6.
  - Patch 5's block comment now names all four t_sock writers,
    including the accept-path setter.
  - Patch 6's changelog gains a note for stable backporters about its
    dependency on patches 3 and 5.
  - Patches 1, 3, 4, 5 and 6 are otherwise code-identical to v3.

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/

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 | 75 +++++++++++++++++++++++++++---------
 net/rds/ib_recv.c    |  9 +++--
 net/rds/send.c       | 12 ++++--
 net/rds/tcp.c        | 92 +++++++++++++++++++++++++++++++-------------
 4 files changed, 137 insertions(+), 51 deletions(-)


base-commit: 4e15e89faac9f308baeb01f46c13a051814d2449
-- 
2.25.1

[PATCH net v4 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>
---
    v4: no change since v3.
 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 v4 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 a later patch 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 patch 1 does for RDS_IN_XMIT in
release_in_xmit().

The acquire side, both the fast-path acquire_refill() and the teardown,
uses test_and_set_bit(), a full-barrier RMW, so it already pairs with
the release.

Fixes: 73ce4317bf98 ("RDS: make sure we post recv buffers")
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
    New in v4.  Completes the release-side pairing for the second
    bit lock that patch 6 acquires; raised while re-reviewing patch 6.
 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 v4 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>
---
    v4: no change since v3.
 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 v4 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 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>
---
    v4: no change since v3.
 net/rds/tcp.c | 17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

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

Re: [PATCH net v4 4/7] net/rds: tcp: don't force RDS_CONN_RESETTING over a concurrent shutdown

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/rds: tcp: don't force RDS_CONN_RESETTING over a concurrent shutdown

This replaces the unconditional atomic_set(&cp->cp_state,
RDS_CONN_RESETTING) in rds_tcp_reset_callbacks() with two conditional
rds_conn_path_transition() attempts (CONNECTING -> RESETTING, and
RESETTING -> RESETTING), and drops the path with rds_conn_path_drop() if
neither succeeds.  The intent is to avoid stomping on a shutdown that is
already in flight.

> diff --git a/net/rds/tcp.c b/net/rds/tcp.c
> index b263634ac750d..ad14217867a4d 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);

[Severity: Medium]
Does this break the post-condition that the only caller of
rds_tcp_reset_callbacks() encodes?

Previously the unconditional atomic_set() guaranteed the path was left in
RDS_CONN_RESETTING.  Now, when neither transition succeeds,
rds_conn_path_drop() leaves cp_state at RDS_CONN_ERROR, and
rds_tcp_reset_callbacks() returns with the path in that state.

In rds_tcp_accept_one() the state check under t_conn_path_lock explicitly
admits RDS_CONN_ERROR:

net/rds/tcp_listen.c:rds_tcp_accept_one() {
	conn_state = rds_conn_path_state(cp);
	WARN_ON(conn_state == RDS_CONN_UP);
	if (conn_state != RDS_CONN_CONNECTING && conn_state != RDS_CONN_ERROR) {
		rds_conn_path_drop(cp, 0);
		goto rst_nsk;
	}
	...
	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(cp, RDS_CONN_RESETTING);
	}

So for a path found in RDS_CONN_ERROR with osock != NULL, both cmpxchg
attempts fail, the drop re-stamps RDS_CONN_ERROR, and the following
rds_connect_path_complete(cp, RDS_CONN_RESETTING) can never make its
RESETTING -> UP transition.

That lands in the failure path:

net/rds/threads.c:rds_connect_path_complete() {
	if (!rds_conn_path_transition(cp, curr, RDS_CONN_UP)) {
		printk(KERN_WARNING "%s: Cannot transition to state UP, "
				"current state is %d\n", __func__,
				atomic_read(&cp->cp_state));
		rds_conn_path_drop(cp, false);
		return;
	}

which is an unconditional, non-ratelimited KERN_WARNING plus another drop,
rather than bringing the freshly installed socket up.  The same mismatch is
reachable when a peer FIN/RST turns the state into RDS_CONN_ERROR via
rds_tcp_state_change() -> rds_conn_path_drop() between the accept-side check
and these two cmpxchgs, since nothing serializes cp_state against
t_conn_path_lock.

Should the hard-coded RDS_CONN_RESETTING argument in rds_tcp_accept_one() and
the "rds_connect_path_complete() marks RDS_CONN_UP" comment be updated, or
should rds_tcp_reset_callbacks() report which branch it took so the caller
can skip the complete step?

[Severity: High]
This isn't a bug introduced by this patch, but the newly added
rds_conn_path_drop(cp, 0) here queues one more cp_down_w pass on cp->cp_wq
while the accept worker is still inside rds_tcp_reset_callbacks() and about
to release osock.  Can that widen the existing double-teardown window on the
same struct socket?

rds_tcp_reset_callbacks() samples the socket at entry:

net/rds/tcp.c:rds_tcp_reset_callbacks() {
	struct socket *osock = tc->t_sock;

	if (!osock)
		goto newsock;

and later, after the drop, unconditionally tears it down:

	rds_tcp_restore_callbacks(osock, tc);
	release_sock(osock->sk);
	sock_release(osock);

The shutdown worker latches the same pointer and does the same teardown:

net/rds/tcp_connect.c:rds_tcp_conn_path_shutdown() {
	struct socket *sock = tc->t_sock;
	...
	lock_sock()/rds_tcp_restore_callbacks()/sock_release()

with a drain loop of up to ~5 s in between.  Neither path holds a common
lock: rds_conn_path_drop() and rds_conn_shutdown() do not take
t_conn_path_lock, and the transport shutdown runs outside cp_cm_lock, so both
can sock_release() the same socket.

The pre-existing part is the unsynchronized sampling of tc->t_sock at the top
of rds_tcp_reset_callbacks().  Checking the states in which the new drop
actually fires with osock != NULL, each already had a shutdown queued, in
flight, or completed (with tc->t_sock already cleared), so I could not
identify a state where this patch alone creates a teardown race that did not
exist before.

Also worth noting for the series ordering: later commits in this series close
the window structurally, "net/rds: acquire RDS_IN_XMIT in
rds_tcp_reset_callbacks()" reads tc->t_sock only while owning RDS_IN_XMIT,
and "net/rds: acquire the fastpath locks in rds_conn_shutdown()" holds
RDS_IN_XMIT across conn_path_shutdown().  At this commit the window is still
open, so a bisect landing here still carries it.  Would it make sense to
order those two commits before this one?
-- 
pw-bot: cr

[PATCH net v4 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>
---
    v4: block comment now names all four t_sock writers, including
    the accept-path setter in rds_tcp_accept_one().
 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 v4 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().  The
window is narrow: the accept-side state check has to pass before the
teardown moves the path to RDS_CONN_DISCONNECTING.

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

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

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

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.

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>
---
    v4: changelog gains a note for stable backporters about the
    dependency on patches 3 and 5.  No code change since v3.
 net/rds/connection.c | 40 ++++++++++++++++++++++++++++++++--------
 net/rds/ib_recv.c    |  4 +++-
 net/rds/send.c       |  5 +++--
 net/rds/tcp.c        | 10 +++++++---
 4 files changed, 45 insertions(+), 14 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index 46ac72088f84..fbbac55a0e81 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -106,10 +106,12 @@ static struct rds_connection *rds_conn_lookup(struct net *net,
 }
 
 /*
- * This is called by transports as they're bringing down a connection.
- * It clears partial message state so that the transport can start sending
- * and receiving over this connection again in the future.  It is up to
- * the transport to have serialized this call with its send and recv.
+ * This is called by rds_conn_shutdown() once the transport has brought
+ * a path down.  It clears partial message state so that the transport
+ * can start sending and receiving over this path again in the future.
+ * The caller owns RDS_IN_XMIT and RDS_RECV_REFILL across this call,
+ * which is what serializes it against the send and receive-refill
+ * paths.
  */
 static void rds_conn_path_reset(struct rds_conn_path *cp)
 {
@@ -124,8 +126,9 @@ static void rds_conn_path_reset(struct rds_conn_path *cp)
 	/* Clear the bits the reset is responsible for individually: a
 	 * blanket cp_flags = 0 is a plain store that can clobber a
 	 * concurrent atomic read-modify-write on the same word.
-	 * RDS_IN_XMIT and RDS_RECV_REFILL belong to the caller,
-	 * rds_conn_shutdown(), and are left alone here.
+	 * RDS_IN_XMIT and RDS_RECV_REFILL are held as locks by the
+	 * caller, rds_conn_shutdown(), which releases them once the
+	 * teardown is complete.
 	 */
 	clear_bit(RDS_LL_SEND_FULL, &cp->cp_flags);
 	clear_bit(RDS_RECONNECT_PENDING, &cp->cp_flags);
@@ -414,14 +417,35 @@ void rds_conn_shutdown(struct rds_conn_path *cp)
 		}
 		mutex_unlock(&cp->cp_cm_lock);
 
+		/* Quiesce the transmit and receive-refill paths by
+		 * acquiring their bit locks, not merely waiting for
+		 * them to be released: with a plain wait, either path
+		 * can re-take its lock the instant after we sample it
+		 * clear and then run concurrently with the transport
+		 * shutdown and the path reset below.  Holding both
+		 * locks across the teardown makes that structurally
+		 * impossible.
+		 */
 		wait_event(cp->cp_waitq,
-			   !test_bit(RDS_IN_XMIT, &cp->cp_flags));
+			   !test_and_set_bit_lock(RDS_IN_XMIT, &cp->cp_flags));
 		wait_event(cp->cp_waitq,
-			   !test_bit(RDS_RECV_REFILL, &cp->cp_flags));
+			   !test_and_set_bit(RDS_RECV_REFILL, &cp->cp_flags));
 
 		conn->c_trans->conn_path_shutdown(cp);
 		rds_conn_path_reset(cp);
 
+		/* Release the two locks and wake any waiter (e.g.
+		 * rds_tcp_reset_callbacks()) that blocked on them while
+		 * we held them.  The unlock orders the transport's ring
+		 * re-initialization and the path reset above before
+		 * either bit is seen clear.  rds_conn_path_reset() leaves
+		 * both bits alone: ownership ends here, not inside the
+		 * reset.
+		 */
+		clear_bit_unlock(RDS_IN_XMIT, &cp->cp_flags);
+		clear_bit_unlock(RDS_RECV_REFILL, &cp->cp_flags);
+		wake_up_all(&cp->cp_waitq);
+
 		if (!rds_conn_path_transition(cp, RDS_CONN_DISCONNECTING,
 					      RDS_CONN_DOWN) &&
 		    !rds_conn_path_transition(cp, RDS_CONN_ERROR,
diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index 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..b90e0586f818 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -244,8 +244,9 @@ int rds_send_xmit(struct rds_conn_path *cp)
 	WRITE_ONCE(cp->cp_send_gen, send_gen);
 
 	/*
-	 * rds_conn_shutdown() sets the conn state and then tests RDS_IN_XMIT,
-	 * we do the opposite to avoid races.
+	 * rds_conn_shutdown() sets the conn state and then acquires
+	 * RDS_IN_XMIT; we take the lock first and then check the state,
+	 * so one of us is guaranteed to see the other's update.
 	 */
 	if (!rds_conn_path_up(cp)) {
 		release_in_xmit(cp);
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index f4c83e368390..826e620b2dd1 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,7 +180,9 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 	/* Read t_sock only while owning RDS_IN_XMIT, never before the
 	 * wait: the teardown in rds_conn_shutdown() releases the old
 	 * socket and clears t_sock, so a pointer sampled earlier can
-	 * be stale by the time we wake up.
+	 * be stale by the time we wake up.  The teardown holds the
+	 * same lock while it does so, so what we read here cannot
+	 * change under us until we release it.
 	 */
 	osock = tc->t_sock;
 	if (!osock)
-- 
2.25.1

Re: [PATCH net v4 6/7] net/rds: acquire the fastpath locks in rds_conn_shutdown()

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/rds: acquire the fastpath locks in rds_conn_shutdown()

This changes rds_conn_shutdown() from waiting for RDS_IN_XMIT and
RDS_RECV_REFILL to be observed clear into acquiring both bits as locks,
holding them across the transport shutdown and rds_conn_path_reset(), and
releasing them with clear_bit_unlock() plus a wake-up.  Related comments in
rds_send_xmit(), rds_ib_recv_refill() and rds_tcp_reset_callbacks() are
updated to describe the new ownership rules.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index 46ac72088f842..fbbac55a0e818 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c

[ ... ]

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

[Severity: Medium]

How long can this ownership window be in practice?  Both bits are now held
across conn->c_trans->conn_path_shutdown(), which blocks.  For TCP that is
the drain loop in rds_tcp_conn_path_shutdown():

	} while (!wait_event_timeout(tc->t_recv_done_waitq,
				(sk->sk_state == TCP_CLOSING || ...) &&
				skb_queue_empty_lockless(&sk->sk_receive_queue),
				msecs_to_jiffies(100)) &&
			 ++rounds < 50);

which is the up to ~5 s the changelog mentions, with the exit condition
driven by the peer's TCP state and receive queue.

For IB, rds_ib_conn_path_shutdown() has no iteration cap:

	while (!wait_event_timeout(rds_ib_ring_empty_wait,
				   rds_ib_conn_path_shutdown_check_wait(cp) == 0,
				   msecs_to_jiffies(1000))) {

so the window there is unbounded, and rds_ib_flush_mrs() and tasklet_kill()
run inside it too.

The only blocking waiter for RDS_IN_XMIT is rds_tcp_reset_callbacks(), called
from rds_tcp_accept_one() on rds_wq, which rds_threads_init() creates as:

	rds_wq = create_singlethread_workqueue("krdsd");

and it parks there holding rtn->rds_tcp_accept_lock and tc->t_conn_path_lock.
While it is parked the single krdsd worker is occupied, so accept processing
for other connections and other namespaces, rds_ibdev->free_work, rds_message
unpin work, and the flush_workqueue(rds_wq) in rds_tcp_listen_stop() during
namespace teardown all wait behind it.  Before this patch that wait was
bounded by a single rds_send_xmit() batch, since teardown never owned the bit.

Is the uncapped IB retry loop meant to be covered by the "up to 5 s" bound
stated in the changelog?

>  		wait_event(cp->cp_waitq,
> -			   !test_bit(RDS_RECV_REFILL, &cp->cp_flags));
> +			   !test_and_set_bit(RDS_RECV_REFILL, &cp->cp_flags));

[Severity: Low]

RDS_RECV_REFILL is now taken as a lock here, and the release comment below
describes an ordered handoff of the bit.  The release side of that handoff is
release_refill() in net/rds/ib_recv.c, which only gained release semantics in
the earlier series patch "net/rds: use clear_bit_unlock() in
release_refill()".

The backport paragraph in the changelog names only two prerequisites:

    "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 release_refill() patch, that function reads:

	clear_bit(RDS_RECV_REFILL, &conn->c_flags);
	smp_mb__after_atomic();

with the barrier after the clear, which does not order the refill path's
preceding ring and recv stores before the bit clear.

Should the release_refill() patch be listed as a prerequisite as well?

>  
>  		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,

[Severity: Medium]

This is a pre-existing behaviour rather than something this patch introduces,
but the new ownership window makes it reachable deterministically, so it
seems worth a question.

rds_tcp_reset_callbacks() does its state check before it blocks on
RDS_IN_XMIT, so a duelling SYN accepted in the window before the teardown
moves the path to RDS_CONN_DISCONNECTING now resumes only after
conn_path_shutdown() and rds_conn_path_reset() have run.  The accept then
installs the new socket:

net/rds/tcp_listen.c:rds_tcp_accept_one() {
	...
	if (rs_tcp->t_sock) {
		rds_tcp_reset_callbacks(new_sock, cp);
		rds_connect_path_complete(cp, RDS_CONN_RESETTING);
	...
}

and rds_connect_path_complete() cannot match RDS_CONN_RESETTING from
DISCONNECTING/ERROR:

net/rds/threads.c:rds_connect_path_complete() {
	if (!rds_conn_path_transition(cp, curr, RDS_CONN_UP)) {
		printk(KERN_WARNING "%s: Cannot transition to state UP, ...");
		rds_conn_path_drop(cp, false);
	...
}

At this commit the RDS_CONN_ERROR set by that drop can be consumed by the
second arm above, so the shutdown pass the drop queued becomes a no-op and
the freshly accepted socket stays attached to a path the teardown already
reset.

The last patch of this series, "net/rds: don't let rds_conn_shutdown()
consume a concurrent drop", replaces that arm with

	if (rds_conn_path_state(cp) == RDS_CONN_ERROR)
		return;

so at the end of the series the queued pass survives and finishes the
teardown.  Since the new deterministic resume point depends on that, should
the standalone-backport paragraph name that patch too?

> diff --git a/net/rds/send.c b/net/rds/send.c
> index 8aad185e4b1aa..b90e0586f818c 100644
> --- a/net/rds/send.c
> +++ b/net/rds/send.c
> @@ -244,8 +244,9 @@ int rds_send_xmit(struct rds_conn_path *cp)
>  	WRITE_ONCE(cp->cp_send_gen, send_gen);
>  
>  	/*
> -	 * rds_conn_shutdown() sets the conn state and then tests RDS_IN_XMIT,
> -	 * we do the opposite to avoid races.
> +	 * rds_conn_shutdown() sets the conn state and then acquires
> +	 * RDS_IN_XMIT; we take the lock first and then check the state,
> +	 * so one of us is guaranteed to see the other's update.
>  	 */
>  	if (!rds_conn_path_up(cp)) {

[Severity: Low]

This isn't a bug, only a note on the wording.  Read on its own, "guaranteed
to see the other's update" looks like a cross-variable ordering claim, which
neither acquire_in_xmit() (test_and_set_bit_lock, acquire only) nor the plain
atomic_read() behind rds_conn_path_up() provides.

Checking it against the code, the claim holds for a different reason: both
sides RMW the same bit of the same cp_flags word, so the modification order
of that word serializes them.  If the sender's test_and_set_bit_lock() wins,
the teardown's returns 1 and it blocks in wait_event(); if the teardown wins,
acquire_in_xmit() fails and the sender backs off.

Would it be clearer to say that ownership is decided by the RMW on cp_flags,
as the changelog does, rather than by ordering between the two variables?

> diff --git a/net/rds/tcp.c b/net/rds/tcp.c
> index f4c83e3683905..826e620b2dd14 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.

[Severity: Low]

The changelog states:

    "It cannot deadlock: the teardown runs on the path's own ordered
    workqueue and never waits on krdsd, so it always completes the drain
    and releases the bit"

Is that accurate as written?  After this patch rds_conn_shutdown() blocks in
wait_event() until it can acquire RDS_IN_XMIT, and rds_tcp_reset_callbacks()
running from the krdsd accept worker can already own that bit, so the
teardown does wait for a krdsd work item to make progress.  The dependency
looks bidirectional.

While holding RDS_IN_XMIT, rds_tcp_reset_callbacks() does:

	cancel_delayed_work_sync(&cp->cp_send_w);
	cancel_delayed_work_sync(&cp->cp_recv_w);
	lock_sock(osock->sk);

cp_send_w and cp_recv_w live on cp->cp_wq, the same workqueue whose single
execution slot the blocked cp_down_w occupies.  That appears safe only
because __rds_conn_create() allocates it as:

		conn->c_path[i].cp_wq =
			alloc_ordered_workqueue("krds_cp_wq#%lu/%d", 0,
						rds_conn_count, i);
		if (!conn->c_path[i].cp_wq)
			conn->c_path[i].cp_wq = rds_wq;

with max_active == 1, so those items cannot be running and the sync cancels
return without flushing.

Could that invariant be stated, either in the changelog or next to the new
blocking wait, given that a non-ordered cp_wq or any added flush inside the
RDS_IN_XMIT section here would turn it into a hang?

[ ... ]

Re: [PATCH net v4 6/7] net/rds: acquire the fastpath locks in rds_conn_shutdown()

[Allison Henderson]

On Thu, 2026-08-27 at 11:02 -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net/rds: acquire the fastpath locks in rds_conn_shutdown()
> 
> This changes rds_conn_shutdown() from waiting for RDS_IN_XMIT and
> RDS_RECV_REFILL to be observed clear into acquiring both bits as locks,
> holding them across the transport shutdown and rds_conn_path_reset(), and
> releasing them with clear_bit_unlock() plus a wake-up.  Related comments in
> rds_send_xmit(), rds_ib_recv_refill() and rds_tcp_reset_callbacks() are
> updated to describe the new ownership rules.
> 
> > diff --git a/net/rds/connection.c b/net/rds/connection.c
> > index 46ac72088f842..fbbac55a0e818 100644
> > --- a/net/rds/connection.c
> > +++ b/net/rds/connection.c
> 
> [ ... ]
> 
> > @@ -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));
> 
> [Severity: Medium]
> 
> How long can this ownership window be in practice?  Both bits are now held
> across conn->c_trans->conn_path_shutdown(), which blocks.  For TCP that is
> the drain loop in rds_tcp_conn_path_shutdown():
> 
> 	} while (!wait_event_timeout(tc->t_recv_done_waitq,
> 				(sk->sk_state == TCP_CLOSING || ...) &&
> 				skb_queue_empty_lockless(&sk->sk_receive_queue),
> 				msecs_to_jiffies(100)) &&
> 			 ++rounds < 50);
> 
> which is the up to ~5 s the changelog mentions, with the exit condition
> driven by the peer's TCP state and receive queue.
> 
> For IB, rds_ib_conn_path_shutdown() has no iteration cap:
> 
> 	while (!wait_event_timeout(rds_ib_ring_empty_wait,
> 				   rds_ib_conn_path_shutdown_check_wait(cp) == 0,
> 				   msecs_to_jiffies(1000))) {
> 
> so the window there is unbounded, and rds_ib_flush_mrs() and tasklet_kill()
> run inside it too.
> 
> The only blocking waiter for RDS_IN_XMIT is rds_tcp_reset_callbacks(), called
> from rds_tcp_accept_one() on rds_wq, which rds_threads_init() creates as:
> 
> 	rds_wq = create_singlethread_workqueue("krdsd");
> 
> and it parks there holding rtn->rds_tcp_accept_lock and tc->t_conn_path_lock.
> While it is parked the single krdsd worker is occupied, so accept processing
> for other connections and other namespaces, rds_ibdev->free_work, rds_message
> unpin work, and the flush_workqueue(rds_wq) in rds_tcp_listen_stop() during
> namespace teardown all wait behind it.  Before this patch that wait was
> bounded by a single rds_send_xmit() batch, since teardown never owned the bit.
> 
> Is the uncapped IB retry loop meant to be covered by the "up to 5 s" bound
> stated in the changelog?

The reason this doesnt concern the krdsd-parking concern is because the only
*blocking* acquirer of these bits is rds_tcp_reset_callbacks().  So the wait only
happens on the TCP path, not the IB path.

Both transports' fastpaths are trylock-and-back-off.  So an uncapped IB drain
lengthens that path's quiesce as it always has, but it doesnt park anything else
(the drain behavior predates this series).

I'll add an extra bit to the commit message to clarify though

> 
> >  		wait_event(cp->cp_waitq,
> > -			   !test_bit(RDS_RECV_REFILL, &cp->cp_flags));
> > +			   !test_and_set_bit(RDS_RECV_REFILL, &cp->cp_flags));
> 
> [Severity: Low]
> 
> RDS_RECV_REFILL is now taken as a lock here, and the release comment below
> describes an ordered handoff of the bit.  The release side of that handoff is
> release_refill() in net/rds/ib_recv.c, which only gained release semantics in
> the earlier series patch "net/rds: use clear_bit_unlock() in
> release_refill()".
> 
> The backport paragraph in the changelog names only two prerequisites:
> 
>     "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 release_refill() patch, that function reads:
> 
> 	clear_bit(RDS_RECV_REFILL, &conn->c_flags);
> 	smp_mb__after_atomic();
> 
> with the barrier after the clear, which does not order the refill path's
> preceding ring and recv stores before the bit clear.
> 
> Should the release_refill() patch be listed as a prerequisite as well?

Sure, I'll update the change log to add patches 2 and 7 to the list

> 
> >  
> >  		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,
> 
> [Severity: Medium]
> 
> This is a pre-existing behaviour rather than something this patch introduces,
> but the new ownership window makes it reachable deterministically, so it
> seems worth a question.
> 
> rds_tcp_reset_callbacks() does its state check before it blocks on
> RDS_IN_XMIT, so a duelling SYN accepted in the window before the teardown
> moves the path to RDS_CONN_DISCONNECTING now resumes only after
> conn_path_shutdown() and rds_conn_path_reset() have run.  The accept then
> installs the new socket:
> 
> net/rds/tcp_listen.c:rds_tcp_accept_one() {
> 	...
> 	if (rs_tcp->t_sock) {
> 		rds_tcp_reset_callbacks(new_sock, cp);
> 		rds_connect_path_complete(cp, RDS_CONN_RESETTING);
> 	...
> }
> 
> and rds_connect_path_complete() cannot match RDS_CONN_RESETTING from
> DISCONNECTING/ERROR:
> 
> net/rds/threads.c:rds_connect_path_complete() {
> 	if (!rds_conn_path_transition(cp, curr, RDS_CONN_UP)) {
> 		printk(KERN_WARNING "%s: Cannot transition to state UP, ...");
> 		rds_conn_path_drop(cp, false);
> 	...
> }
> 
> At this commit the RDS_CONN_ERROR set by that drop can be consumed by the
> second arm above, so the shutdown pass the drop queued becomes a no-op and
> the freshly accepted socket stays attached to a path the teardown already
> reset.
> 
> The last patch of this series, "net/rds: don't let rds_conn_shutdown()
> consume a concurrent drop", replaces that arm with
> 
> 	if (rds_conn_path_state(cp) == RDS_CONN_ERROR)
> 		return;
> 
> so at the end of the series the queued pass survives and finishes the
> teardown.  Since the new deterministic resume point depends on that, should
> the standalone-backport paragraph name that patch too?
> 
> > diff --git a/net/rds/send.c b/net/rds/send.c
> > index 8aad185e4b1aa..b90e0586f818c 100644
> > --- a/net/rds/send.c
> > +++ b/net/rds/send.c
> > @@ -244,8 +244,9 @@ int rds_send_xmit(struct rds_conn_path *cp)
> >  	WRITE_ONCE(cp->cp_send_gen, send_gen);
> >  
> >  	/*
> > -	 * rds_conn_shutdown() sets the conn state and then tests RDS_IN_XMIT,
> > -	 * we do the opposite to avoid races.
> > +	 * rds_conn_shutdown() sets the conn state and then acquires
> > +	 * RDS_IN_XMIT; we take the lock first and then check the state,
> > +	 * so one of us is guaranteed to see the other's update.
> >  	 */
> >  	if (!rds_conn_path_up(cp)) {
> 
> [Severity: Low]
> 
> This isn't a bug, only a note on the wording.  Read on its own, "guaranteed
> to see the other's update" looks like a cross-variable ordering claim, which
> neither acquire_in_xmit() (test_and_set_bit_lock, acquire only) nor the plain
> atomic_read() behind rds_conn_path_up() provides.
> 
> Checking it against the code, the claim holds for a different reason: both
> sides RMW the same bit of the same cp_flags word, so the modification order
> of that word serializes them.  If the sender's test_and_set_bit_lock() wins,
> the teardown's returns 1 and it blocks in wait_event(); if the teardown wins,
> acquire_in_xmit() fails and the sender backs off.
> 
> Would it be clearer to say that ownership is decided by the RMW on cp_flags,
> as the changelog does, rather than by ordering between the two variables?

Sure, I'll rework the comment to clarify the guarantee actually comes from both
sides doing an atomic RMW on the same cp_flags.

> 
> > diff --git a/net/rds/tcp.c b/net/rds/tcp.c
> > index f4c83e3683905..826e620b2dd14 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.
> 
> [Severity: Low]
> 
> The changelog states:
> 
>     "It cannot deadlock: the teardown runs on the path's own ordered
>     workqueue and never waits on krdsd, so it always completes the drain
>     and releases the bit"
> 
> Is that accurate as written?  After this patch rds_conn_shutdown() blocks in
> wait_event() until it can acquire RDS_IN_XMIT, and rds_tcp_reset_callbacks()
> running from the krdsd accept worker can already own that bit, so the
> teardown does wait for a krdsd work item to make progress.  The dependency
> looks bidirectional.
> 
> While holding RDS_IN_XMIT, rds_tcp_reset_callbacks() does:
> 
> 	cancel_delayed_work_sync(&cp->cp_send_w);
> 	cancel_delayed_work_sync(&cp->cp_recv_w);
> 	lock_sock(osock->sk);
> 
> cp_send_w and cp_recv_w live on cp->cp_wq, the same workqueue whose single
> execution slot the blocked cp_down_w occupies.  That appears safe only
> because __rds_conn_create() allocates it as:
> 
> 		conn->c_path[i].cp_wq =
> 			alloc_ordered_workqueue("krds_cp_wq#%lu/%d", 0,
> 						rds_conn_count, i);
> 		if (!conn->c_path[i].cp_wq)
> 			conn->c_path[i].cp_wq = rds_wq;
> 
> with max_active == 1, so those items cannot be running and the sync cancels
> return without flushing.
> 
> Could that invariant be stated, either in the changelog or next to the new
> blocking wait, given that a non-ordered cp_wq or any added flush inside the
> RDS_IN_XMIT section here would turn it into a hang?

Sure, we can expand the explaining so more.  I think this is re-hitting some
of the same things explained in v2. The waits point at each other but the holder
never needs anything the teardown owns. 

To further clarify: the canceled syncs target cp_send_w/cp_recv_w on the
path's ordered cp_wq.  But since they are single threaded, the slot is
occupied by the blocked cp_down_w itself.  So anything still queued behind
it cancels with out flushing.  We wouldn't want to stop and wait for an
explicit flush because the thing at the head of the queue that everyone's
waiting on is itself.

But I'll add a comment in the change log next to those cancels stating the
ordered-cp_wq reliance. So if, for example, another patch were to add a
non-ordered cp_wq or a flush inside the held-bit section, then yes, this would
indeed turn this into a hang in that case.

Thanks!
Allison

> 
> [ ... ]

[PATCH net v4 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:
return quietly and let the pass that drop queued finish the job - it
tears down whatever attached to the path in the meantime, completes
the transition to RDS_CONN_DOWN, and handles the reconnect.  If no
pass was queued because a destroy is pending, rds_conn_path_destroy()
performs the final drop and flush itself.  The FIN case keeps making
progress, one pass later and still without noisy logging; any other
unexpected state keeps today's rds_conn_path_error() handling.

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>
---
    New in v4.  Fixes the accept-vs-drop socket leak found while
    re-reviewing patch 6.
 net/rds/connection.c | 29 ++++++++++++++++++-----------
 net/rds/tcp.c        |  9 ++++++---
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index fbbac55a0e81..73b4fa8a4b96 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -447,20 +447,27 @@ 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.  The drop
+			 * 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.
+			 * Anything else - e.g. a module unload pulling
+			 * the connection down mid-teardown - keeps the
+			 * usual noisy drop.
 			 */
+			if (rds_conn_path_state(cp) == RDS_CONN_ERROR)
+				return;
 			rds_conn_path_error(cp, "%s: failed to transition "
 					    "to state DOWN, current state "
 					    "is %d\n", __func__,
diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index 826e620b2dd1..50c2f0b3a038 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 v4 7/7] net/rds: don't let rds_conn_shutdown() consume a concurrent drop

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/rds: don't let rds_conn_shutdown() consume a concurrent drop

This restricts the final state transition in rds_conn_shutdown() to
DISCONNECTING -> DOWN, and returns quietly when the path is found in
RDS_CONN_ERROR so that the shutdown pass queued by the racing drop is the
one that finishes the teardown.  The comments in rds_conn_shutdown() and
rds_tcp_reset_callbacks() are rewritten to describe the new rule.

> diff --git a/net/rds/connection.c b/net/rds/connection.c
> index fbbac55a0e818..73b4fa8a4b963 100644
> --- a/net/rds/connection.c
> +++ b/net/rds/connection.c
> @@ -447,20 +447,27 @@ 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.  The drop
> +			 * 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.
> +			 * Anything else - e.g. a module unload pulling
> +			 * the connection down mid-teardown - keeps the
> +			 * usual noisy drop.
>  			 */
> +			if (rds_conn_path_state(cp) == RDS_CONN_ERROR)
> +				return;

[Severity: High]
The new return does not just skip the reconnect - it leaves
rds_conn_shutdown() before its whole tail, which is also where
cancel_delayed_work_sync(&cp->cp_conn_w) runs, where
RDS_RECONNECT_PENDING is cleared, and where conn_slots_available()
is called.  Who performs that work when this return fires?

For the ordinary case the changelog's answer holds: the pass that the
racing rds_conn_path_drop() queued runs later and reaches the tail.
But the drop does not always queue a pass.  rds_conn_path_drop(cp,
false) stores RDS_CONN_ERROR unconditionally and only then checks
rds_destroy_pending(cp->cp_conn), returning without queue_work() when
a destroy is in progress - which is precisely the qualification the
new rds_tcp_reset_callbacks() comment in this patch spells out, and
which the changelog answers with "rds_conn_path_destroy() performs the
final drop and flush itself".

That answer looks one pass short.  rds_conn_path_destroy() does a
single rds_conn_path_drop(cp, true) followed by
flush_work(&cp->cp_down_w).  If the flushed pass is the one that a
concurrent FIN turns into this silent return - rds_tcp_state_change()
-> rds_conn_path_drop(cp, false) during the conn_path_shutdown() and
quiesce window, destroy pending, so RDS_CONN_ERROR is stored and no
cp_down_w is queued - then no pass ever reaches the tail, and there is
nothing left to flush.  rds_conn_path_destroy() continues into

	WARN_ON(delayed_work_pending(&cp->cp_conn_w));

and then frees the transport data and the path with cp_conn_w still
armed.  The WARN_ON is itself the evidence that destroy relies on
rds_conn_shutdown() having cancelled that work; if a cp_conn_w queued
by an earlier rds_queue_reconnect() has not yet expired, its timer
fires on a freed rds_conn_path.  Before this patch the ERROR -> DOWN
leg of the final transition kept the tail reachable in exactly this
situation, so the cancel always ran.

So two questions.  Should the RDS_CONN_ERROR return be placed after
cancel_delayed_work_sync(&cp->cp_conn_w) (and after the
RDS_RECONNECT_PENDING clear), so the path's timers are quiesced no
matter which pass completes the state transition?  Or should
rds_conn_path_destroy() stop depending on the shutdown pass and cancel
cp_conn_w itself before the WARN_ONs?  Either way, the destroy-pending
case deserves a comment here, because that is the one case where the
"let the pass finish the job" argument has no pass to lean on.

[Severity: Medium]
Does the new comment in rds_conn_shutdown() match the code below it?

It says a module unload "keeps the usual noisy drop", but the unload path
goes through rds_conn_destroy() -> rds_conn_path_destroy() ->
rds_conn_path_drop(cp, true), and rds_conn_path_drop() starts with an
unconditional state store:

net/rds/connection.c:rds_conn_path_drop() {
	atomic_set(&cp->cp_state, RDS_CONN_ERROR);

	rcu_read_lock();
	if (!destroy && rds_destroy_pending(cp->cp_conn)) {
		rcu_read_unlock();
		return;
	}
	queue_work(cp->cp_wq, &cp->cp_down_w);
	rcu_read_unlock();
}

So an unload racing an in-flight shutdown leaves the path in
RDS_CONN_ERROR and takes the new silent return, not the
rds_conn_path_error() branch the comment points at.  The old comment that
documented exactly that case ("someone unloads the rds module ... Mostly
harmless") is removed in the same hunk, so is the unload scenario now
described against the wrong branch?

Second question on the same comment: it states flatly that "The drop
queued another shutdown pass".  In the snippet above, a
rds_conn_path_drop(cp, false) returns before queue_work() when
rds_destroy_pending() is true:

net/rds/rds.h:rds_destroy_pending() {
	return !check_net(rds_conn_net(conn)) ||
	       (conn->c_trans->t_unloading && conn->c_trans->t_unloading(conn));
}

The comment this patch adds to rds_tcp_reset_callbacks() carries that
qualification:

	 * and, unless a pending destroy is about to reap the whole
	 * connection anyway, queues one more shutdown pass.

Should the connection.c comment carry the same "unless a destroy is
pending" wording so the two new comments agree?

Last question: which state actually reaches rds_conn_path_error() here?
Every other writer of cp_state uses a cmpxchg from a non-DISCONNECTING
state - rds_connect_path_complete() from CONNECTING/RESETTING,
rds_tcp_reset_callbacks() from CONNECTING/RESETTING, rds_connect_worker()
from DOWN - and the only unconditional store is the RDS_CONN_ERROR in
rds_conn_path_drop().  If nothing can leave a DISCONNECTING path in a
state other than RDS_CONN_ERROR, is the noisy branch the comment
illustrates reachable at all?

> +			rds_conn_path_error(cp, "%s: failed to transition "
>  					    "to state DOWN, current state "
>  					    "is %d\n", __func__,

[ ... ]

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

[Allison Henderson]

On Thu, 2026-08-27 at 11:02 -0700, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.
> ---
> net/rds: don't let rds_conn_shutdown() consume a concurrent drop
> 
> This restricts the final state transition in rds_conn_shutdown() to
> DISCONNECTING -> DOWN, and returns quietly when the path is found in
> RDS_CONN_ERROR so that the shutdown pass queued by the racing drop is the
> one that finishes the teardown.  The comments in rds_conn_shutdown() and
> rds_tcp_reset_callbacks() are rewritten to describe the new rule.
> 
> > diff --git a/net/rds/connection.c b/net/rds/connection.c
> > index fbbac55a0e818..73b4fa8a4b963 100644
> > --- a/net/rds/connection.c
> > +++ b/net/rds/connection.c
> > @@ -447,20 +447,27 @@ 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.  The drop
> > +			 * 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.
> > +			 * Anything else - e.g. a module unload pulling
> > +			 * the connection down mid-teardown - keeps the
> > +			 * usual noisy drop.
> >  			 */
> > +			if (rds_conn_path_state(cp) == RDS_CONN_ERROR)
> > +				return;
> 
> [Severity: High]
> The new return does not just skip the reconnect - it leaves
> rds_conn_shutdown() before its whole tail, which is also where
> cancel_delayed_work_sync(&cp->cp_conn_w) runs, where
> RDS_RECONNECT_PENDING is cleared, and where conn_slots_available()
> is called.  Who performs that work when this return fires?
> 
> For the ordinary case the changelog's answer holds: the pass that the
> racing rds_conn_path_drop() queued runs later and reaches the tail.
> But the drop does not always queue a pass.  rds_conn_path_drop(cp,
> false) stores RDS_CONN_ERROR unconditionally and only then checks
> rds_destroy_pending(cp->cp_conn), returning without queue_work() when
> a destroy is in progress - which is precisely the qualification the
> new rds_tcp_reset_callbacks() comment in this patch spells out, and
> which the changelog answers with "rds_conn_path_destroy() performs the
> final drop and flush itself".
> 
> That answer looks one pass short.  rds_conn_path_destroy() does a
> single rds_conn_path_drop(cp, true) followed by
> flush_work(&cp->cp_down_w).  If the flushed pass is the one that a
> concurrent FIN turns into this silent return - rds_tcp_state_change()
> -> rds_conn_path_drop(cp, false) during the conn_path_shutdown() and
> quiesce window, destroy pending, so RDS_CONN_ERROR is stored and no
> cp_down_w is queued - then no pass ever reaches the tail, and there is
> nothing left to flush.  rds_conn_path_destroy() continues into
> 
> 	WARN_ON(delayed_work_pending(&cp->cp_conn_w));
> 
> and then frees the transport data and the path with cp_conn_w still
> armed.  The WARN_ON is itself the evidence that destroy relies on
> rds_conn_shutdown() having cancelled that work; if a cp_conn_w queued
> by an earlier rds_queue_reconnect() has not yet expired, its timer
> fires on a freed rds_conn_path.  Before this patch the ERROR -> DOWN
> leg of the final transition kept the tail reachable in exactly this
> situation, so the cancel always ran.
> 
> So two questions.  Should the RDS_CONN_ERROR return be placed after
> cancel_delayed_work_sync(&cp->cp_conn_w) (and after the
> RDS_RECONNECT_PENDING clear), so the path's timers are quiesced no
> matter which pass completes the state transition?  Or should
> rds_conn_path_destroy() stop depending on the shutdown pass and cancel
> cp_conn_w itself before the WARN_ONs?  Either way, the destroy-pending
> case deserves a comment here, because that is the one case where the
> "let the pass finish the job" argument has no pass to lean on.

This finding is real, thanks for the catch.  Reproduced on an
instrumented kernel to inject the FIN drop into the teardown window
of a dying netns with a long reconnect backoff.

I'll move the RDS_CONN_ERROR after the cancel_delayed_work_sync as
suggested, and add some more comments for the destroy pending case.


> 
> [Severity: Medium]
> Does the new comment in rds_conn_shutdown() match the code below it?
> 
> It says a module unload "keeps the usual noisy drop", but the unload path
> goes through rds_conn_destroy() -> rds_conn_path_destroy() ->
> rds_conn_path_drop(cp, true), and rds_conn_path_drop() starts with an
> unconditional state store:
> 
> net/rds/connection.c:rds_conn_path_drop() {
> 	atomic_set(&cp->cp_state, RDS_CONN_ERROR);
> 
> 	rcu_read_lock();
> 	if (!destroy && rds_destroy_pending(cp->cp_conn)) {
> 		rcu_read_unlock();
> 		return;
> 	}
> 	queue_work(cp->cp_wq, &cp->cp_down_w);
> 	rcu_read_unlock();
> }
> 
> So an unload racing an in-flight shutdown leaves the path in
> RDS_CONN_ERROR and takes the new silent return, not the
> rds_conn_path_error() branch the comment points at.  The old comment that
> documented exactly that case ("someone unloads the rds module ... Mostly
> harmless") is removed in the same hunk, so is the unload scenario now
> described against the wrong branch?
> 
Yes, the comment is stale. I will update the comment here, and mention the
module unload's drop stores RDS_CONN_ERROR like every other drop.

> Second question on the same comment: it states flatly that "The drop
> queued another shutdown pass".  In the snippet above, a
> rds_conn_path_drop(cp, false) returns before queue_work() when
> rds_destroy_pending() is true:
> 
> net/rds/rds.h:rds_destroy_pending() {
> 	return !check_net(rds_conn_net(conn)) ||
> 	       (conn->c_trans->t_unloading && conn->c_trans->t_unloading(conn));
> }
> 
> The comment this patch adds to rds_tcp_reset_callbacks() carries that
> qualification:
> 
> 	 * and, unless a pending destroy is about to reap the whole
> 	 * connection anyway, queues one more shutdown pass.
> 
> Should the connection.c comment carry the same "unless a destroy is
> pending" wording so the two new comments agree?
> 
Yes, will update the comment to carry the same wording that
rds_tcp_reset_callbacks() does.  


> Last question: which state actually reaches rds_conn_path_error() here?
> Every other writer of cp_state uses a cmpxchg from a non-DISCONNECTING
> state - rds_connect_path_complete() from CONNECTING/RESETTING,
> rds_tcp_reset_callbacks() from CONNECTING/RESETTING, rds_connect_worker()
> from DOWN - and the only unconditional store is the RDS_CONN_ERROR in
> rds_conn_path_drop().  If nothing can leave a DISCONNECTING path in a
> state other than RDS_CONN_ERROR, is the noisy branch the comment
> illustrates reachable at all?
> 
> > +			rds_conn_path_error(cp, "%s: failed to transition "
> >  					    "to state DOWN, current state "
> >  					    "is %d\n", __func__,
> 
> [ ... ]


I dont think so since 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 the print is more defensive really.  I'll update
the comment to clarify.

Thanks for the reviews!
Allison