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

[Allison Henderson <achender@kernel.org>]

Hi all,

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

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

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

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

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

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

With those in place, patch 5 converts rds_tcp_reset_callbacks() from
waiting on RDS_IN_XMIT to acquiring it, holding it across the socket
swap and rds_send_path_reset(), and patch 6 has rds_conn_shutdown()
hold both bit locks across the transport shutdown and path reset.

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

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

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

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

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

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

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

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

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

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

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

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

Questions and comments appreciated!

Thanks,
Allison

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

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

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

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

 net/rds/connection.c |  89 ++++++++++++++++++++++++++++++--------
 net/rds/ib_recv.c    |   9 ++--
 net/rds/send.c       |  14 ++++--
 net/rds/tcp.c        | 101 +++++++++++++++++++++++++++++++------------
 net/rds/tcp_listen.c |   6 ++-
 5 files changed, 166 insertions(+), 53 deletions(-)


base-commit: 4e15e89faac9f308baeb01f46c13a051814d2449
-- 
2.25.1