From: Sharath Srinivasan 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 Signed-off-by: Sharath Srinivasan [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 --- 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 From: William Kucharski 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 [achender: port to net-next; update commit message] Assisted-by: Claude-Code:claude-fable-5 Signed-off-by: Allison Henderson --- 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 From: Håkon Bugge 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 [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 --- 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 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 --- 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