github-mirror/linux
1
0
Fork 0
mirror of https://github.com/torvalds/linux.git synced 2026-09-11 20:13:02 +02:00
Code Issues Actions Packages Projects Releases Wiki Activity Graph

Merge branch 'net-rds-own-the-fastpath-locks-across-connection-teardown'

Browse Source 
Allison Henderson says:

====================
net/rds: own the fastpath locks across connection teardown

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
e97656d03c 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).
====================

Link: https://patch.msgid.link/20260828223921.202913-1-achender@kernel.org
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
This commit is contained in:
Jakub Kicinski 2026-09-02 19:42:25 -07:00
commit 2f38e26a57
5 changed files with 166 additions and 53 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

89
net/rds/connection.c
View File

@ -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)
{
@ -120,7 +122,16 @@ 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 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);
/* Do not clear next_rx_seq here, else we cannot distinguish
* retransmitted packets from new packets, and will hand all
@ -406,28 +417,70 @@ 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,
RDS_CONN_DOWN)) {
/* This can happen - eg when we're in the middle of tearing
* down the connection, and someone unloads the rds module.
* Quite reproducible with loopback connections.
* Mostly harmless.
/* The path was dropped again while we tore it
* down: by a socket state-change callback in
* irq context on receipt of a FIN, or by an
* accept that claimed the path just before a
* drop put it back to RDS_CONN_ERROR and then
* installed a fresh socket on it. Unless a
* pending destroy suppressed it, the drop also
* queued another shutdown pass, and that pass
* must run, because it is what tears down
* whatever attached to the path after the
* transport shutdown above sampled its state.
* Consuming the RDS_CONN_ERROR here would turn
* that pass into a no-op: leave the state
* alone, and let the pass finish the job.
*
* Note that this also happens with rds-tcp because
* we could have triggered rds_conn_path_drop in irq
* mode from rds_tcp_state change on the receipt of
* a FIN, thus we need to recheck for RDS_CONN_ERROR
* here.
* Quiesce the reconnect timer before bailing
* out, though. When a pending destroy did
* suppress the queue, no later pass runs, and
* rds_conn_path_destroy() is about to flush
* cp_down_w and free the path: it must not
* find cp_conn_w still armed. A successor
* pass, when there is one, re-arms the
* reconnect from its own tail.
*/
cancel_delayed_work_sync(&cp->cp_conn_w);
clear_bit(RDS_RECONNECT_PENDING, &cp->cp_flags);
if (rds_conn_path_state(cp) == RDS_CONN_ERROR)
return;
/* No current cp_state writer leaves a
* DISCONNECTING path in any state but
* RDS_CONN_ERROR; report loudly if one ever
* does.
*/
rds_conn_path_error(cp, "%s: failed to transition "
"to state DOWN, current state "

9
net/rds/ib_recv.c
View File

@ -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);
}
@ -392,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;

14
net/rds/send.c
View File

@ -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);
}
@ -239,8 +244,11 @@ int rds_send_xmit(struct rds_conn_path *cp)
WRITE_ONCE(cp->cp_send_gen, send_gen);
/*
* rds_conn_shutdown() sets the conn state and then tests RDS_IN_XMIT,
* we do the opposite to avoid races.
* rds_conn_shutdown() sets the conn state and then acquires
* RDS_IN_XMIT; we take the lock first and then check the state.
* Ownership is decided by the atomic RMW on the cp_flags word:
* if the teardown won the bit we back off here, and if we won
* it the teardown waits until we release it.
*/
if (!rds_conn_path_up(cp)) {
release_in_xmit(cp);

101
net/rds/tcp.c
View File

@ -115,46 +115,90 @@ 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() - 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.
*
* 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
* 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, 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) &&
!rds_conn_path_transition(cp, RDS_CONN_RESETTING,
RDS_CONN_RESETTING))
rds_conn_path_drop(cp, 0);
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. The teardown holds the
* same lock while it does so, so what we read here cannot
* change under us until we release it.
*/
osock = tc->t_sock;
if (!osock)
goto newsock;
/* reset receive side state for rds_tcp_data_recv() for osock.
*
* The sync cancels while owning RDS_IN_XMIT rely on cp_wq
* being ordered: a teardown blocked on the bit occupies
* cp_wq's only execution slot, so cp_send_w and cp_recv_w are
* pending at most and the cancels never flush. Nothing here
* may flush or wait on cp_wq itself.
*/
atomic_set(&cp->cp_state, RDS_CONN_RESETTING);
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);
cancel_delayed_work_sync(&cp->cp_recv_w);
lock_sock(osock->sk);
@ -172,6 +216,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

6
net/rds/tcp_listen.c
View File

@ -295,7 +295,11 @@ int rds_tcp_accept_one(struct rds_tcp_net *rtn)
if (rs_tcp->t_sock) {
/* Duelling SYN has been handled in rds_tcp_accept_one() */
rds_tcp_reset_callbacks(new_sock, cp);
/* rds_connect_path_complete() marks RDS_CONN_UP */
/* rds_connect_path_complete() marks RDS_CONN_UP, or,
* if a concurrent shutdown won the duel, drops the
* path again and the pass that drop queues reaps the
* socket installed above.
*/
rds_connect_path_complete(cp, RDS_CONN_RESETTING);
} else {
rds_tcp_set_callbacks(new_sock, cp);