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

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

[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
-- 
2.25.1