[PATCH net-next 0/4] net/rds: Bug fix ports, part 2

[PATCH net-next 0/4] net/rds: Bug fix ports, part 2

[Allison Henderson]

Hi all,

This is the next batch of net/rds fixes ported from the Oracle UEK
kernel, following up on the first set now in net-next [1].  As
discussed on the previous series, these target net-next since the
bugs are pretty old and could probably use the longer cycle.

Patches 3 and 4 close a teardown race that UEK fixed after a long
tail of failover crashes: quiescing the transmit path by *sampling*
the RDS_IN_XMIT/RDS_RECV_REFILL bits clear instead of acquiring them
is a store-buffering pattern.  So on weakly ordered arches,
rds_send_xmit() can run concurrently with connection teardown
resetting the transmit state under it. Patch 3 makes
rds_conn_shutdown() take the bit locks.  Patch 4 does the same for
the only other rds_send_path_reset() call site in
rds_tcp_reset_callbacks().

Patches 1 and 2 are small initialization hardening ports from the
same UEK series. re-initialize rds_send_xmit()'s to_be_dropped
list on its restart path, and initialize i_conn_path in
rds_inc_init() so both inc init helpers leave an equivalent,
fully-initialized structure.  Neither has a known user-visible bug
upstream.

[PATCH net 1/4] net/rds: reinitialize to_be_dropped on rds_send_xmit() restart
  Port commit 7f52b9968d79 ("net/rds: rds_send_xmit should INIT_LIST_HEAD (&to_be_dropped) on restart")
  https://github.com/oracle/linux-uek/commit/7f52b9968d79 

[PATCH net 2/4] net/rds: initialize i_conn_path in rds_inc_init()
  Port commit 0ec6a520da4f ("rds: rds_inc_init() should initialize the inc->i_conn_path field")
  https://github.com/oracle/linux-uek/commit/0ec6a520da4f
 
[PATCH net 3/4] net/rds: acquire the fastpath locks in rds_conn_shutdown()
  Port 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 4/4] net/rds: acquire RDS_IN_XMIT in rds_tcp_reset_callbacks()
  Extend the last port to cover extra call sites in rds_tcp_reset_callbacks()

Questions and comments appreciated!

Thanks,
Allison

[1] https://lore.kernel.org/netdev/20260730041629.3512480-1-achender@kernel.org/

Allison Henderson (1):
  net/rds: acquire RDS_IN_XMIT in rds_tcp_reset_callbacks()

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

Sharath Srinivasan (1):
  net/rds: reinitialize to_be_dropped on rds_send_xmit() restart

William Kucharski (1):
  net/rds: initialize i_conn_path in rds_inc_init()

 net/rds/connection.c | 22 ++++++++++++++++++++--
 net/rds/recv.c       |  1 +
 net/rds/send.c       |  6 ++++++
 net/rds/tcp.c        | 15 ++++++++++++++-
 4 files changed, 41 insertions(+), 3 deletions(-)


base-commit: b0057c68df711bf6a62033c072ac61c4f9d3cbc1
-- 
2.25.1

[PATCH net-next 1/4] net/rds: reinitialize to_be_dropped on rds_send_xmit() restart

[Allison Henderson]

From: Sharath Srinivasan <sharath.srinivasan@oracle.com>

The to_be_dropped list is declared once at the top of rds_send_xmit()
but the function can loop via "goto restart" after each batch.  The
code currently relies on rds_send_remove_from_sock() having emptied
the list entry by entry (via list_del_init()) at the end of the
previous batch; nothing in rds_send_xmit() itself guarantees the list
head is empty when a new batch starts.

Re-initialize the list on every restart so a change in the callee's
behavior cannot turn into list corruption and double-puts.  This is
hardening: no user-visible bug is known in the current code.

This mirrors Oracle UEK commit "net/rds: rds_send_xmit should
INIT_LIST_HEAD(&to_be_dropped) on restart".

Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
Signed-off-by: Sharath Srinivasan <sharath.srinivasan@oracle.com>
[achender: port to net-next (keep the existing LIST_HEAD declaration and
 add only the restart re-init); update commit message]
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/send.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/rds/send.c b/net/rds/send.c
index 309021e0cc9bc..c28dc9f820af0 100644
--- a/net/rds/send.c
+++ b/net/rds/send.c
@@ -200,6 +200,12 @@ int rds_send_xmit(struct rds_conn_path *cp)
 restart:
 	batch_count = 0;
 
+	/* The drop processing after over_batch relies on the callees
+	 * emptying to_be_dropped entry by entry; re-initialize it here
+	 * rather than depending on that implicit behavior.
+	 */
+	INIT_LIST_HEAD(&to_be_dropped);
+
 	/*
 	 * sendmsg calls here after having queued its message on the send
 	 * queue.  We only have one task feeding the connection at a time.  If
-- 
2.25.1

[PATCH net-next 2/4] net/rds: initialize i_conn_path in rds_inc_init()

[Allison Henderson]

From: William Kucharski <william.kucharski@oracle.com>

rds_inc_init() initializes every field of the embedded rds_incoming
except i_conn_path, and incomings are not zero-allocated (IB carves
them out of a slab cache).  The field therefore holds stale garbage
for incs created by the non-multipath transports (rds_ib, loopback).

Initialize it to NULL so that any future reader trips over a clean
NULL pointer instead of a stale one, and so the two init helpers
(rds_inc_init/rds_inc_path_init) leave the structure in an equivalent,
fully-initialized state.  Hardening only; no user-visible bug in the
current code.

This mirrors Oracle UEK commit "rds: rds_inc_init() should initialize
the inc->i_conn_path field".

Signed-off-by: William Kucharski <william.kucharski@oracle.com>
[achender: port to net-next; update commit message]
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/recv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/rds/recv.c b/net/rds/recv.c
index cf3884d879319..f1513dfb27165 100644
--- a/net/rds/recv.c
+++ b/net/rds/recv.c
@@ -47,6 +47,7 @@ void rds_inc_init(struct rds_incoming *inc, struct rds_connection *conn,
 	refcount_set(&inc->i_refcount, 1);
 	INIT_LIST_HEAD(&inc->i_item);
 	inc->i_conn = conn;
+	inc->i_conn_path = NULL;
 	inc->i_saddr = *saddr;
 	inc->i_usercopy.rdma_cookie = 0;
 	inc->i_usercopy.rx_tstamp = ktime_set(0, 0);
-- 
2.25.1

[PATCH net-next 3/4] 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 afterwards.  All other users of these bits are trylock
style (rds_send_xmit(), the IB send tasklet, rds_ib_recv_refill()),
so they back off while teardown owns the locks and no new lock
dependency is introduced.  The explicit release with a wake-up also
covers waiters such as rds_tcp_reset_callbacks() that would otherwise
miss the silent clear inside rds_conn_path_reset().

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 commit message]
Assisted-by: Claude-Code:claude-fable-5
Signed-off-by: Allison Henderson <achender@kernel.org>
---
 net/rds/connection.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/net/rds/connection.c b/net/rds/connection.c
index 7c8ab8e973e1b..406a071efdd3b 100644
--- a/net/rds/connection.c
+++ b/net/rds/connection.c
@@ -406,14 +406,32 @@ 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);
 
+		/* rds_conn_path_reset() already cleared cp_flags, but
+		 * release the two locks explicitly and wake any waiter
+		 * (e.g. rds_tcp_reset_callbacks()) that sampled the
+		 * locks while we held them.
+		 */
+		clear_bit_unlock(RDS_IN_XMIT, &cp->cp_flags);
+		clear_bit(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,
-- 
2.25.1

[PATCH net-next 4/4] 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().

As in rds_conn_shutdown(), 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 (teardown 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.

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.  The !osock
early path is unchanged: it does not quiesce today and the connection
has never been RDS_CONN_UP at that point, so there is no sender to
serialize against.

This extends the previous change ("net/rds: acquire
the fastpath locks in rds_conn_shutdown()") to the only other
rds_send_path_reset() call site, mirroring Oracle UEK's "rds: Make sure
transmit path and connection tear-down does not run concurrently".

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>
---
 net/rds/tcp.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/net/rds/tcp.c b/net/rds/tcp.c
index b263634ac750d..042d3fdbdf7fe 100644
--- a/net/rds/tcp.c
+++ b/net/rds/tcp.c
@@ -128,6 +128,7 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 {
 	struct rds_tcp_connection *tc = cp->cp_transport_data;
 	struct socket *osock = tc->t_sock;
+	bool in_xmit_held = false;
 
 	if (!osock)
 		goto newsock;
@@ -153,7 +154,14 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 	 * cannot mark rds_conn_path_up() in the window before lock_sock()
 	 */
 	atomic_set(&cp->cp_state, RDS_CONN_RESETTING);
-	wait_event(cp->cp_waitq, !test_bit(RDS_IN_XMIT, &cp->cp_flags));
+	/* Acquire the send-path lock rather than waiting for it to be
+	 * released: a mere wait is racy, since rds_send_xmit() may take
+	 * the lock again right after we sample it clear and then run
+	 * concurrently with rds_send_path_reset() below.
+	 */
+	wait_event(cp->cp_waitq,
+		   !test_and_set_bit_lock(RDS_IN_XMIT, &cp->cp_flags));
+	in_xmit_held = true;
 	/* 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);
@@ -172,6 +180,11 @@ void rds_tcp_reset_callbacks(struct socket *sock,
 	lock_sock(sock->sk);
 	rds_tcp_set_callbacks(sock, cp);
 	release_sock(sock->sk);
+
+	if (in_xmit_held) {
+		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