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

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

Sashiko noticed a socket leak exposed by this set, so i've resent a v4:
https://lore.kernel.org/all/[email protected]/

Thanks!
Allison

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