Re: [PATCH net v4 4/7] net/rds: tcp: don't force RDS_CONN_RESETTING over a concurrent shutdown

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net/rds: tcp: don't force RDS_CONN_RESETTING over a concurrent shutdown

This replaces the unconditional atomic_set(&cp->cp_state,
RDS_CONN_RESETTING) in rds_tcp_reset_callbacks() with two conditional
rds_conn_path_transition() attempts (CONNECTING -> RESETTING, and
RESETTING -> RESETTING), and drops the path with rds_conn_path_drop() if
neither succeeds.  The intent is to avoid stomping on a shutdown that is
already in flight.

> diff --git a/net/rds/tcp.c b/net/rds/tcp.c
> index b263634ac750d..ad14217867a4d 100644
> --- a/net/rds/tcp.c
> +++ b/net/rds/tcp.c
> @@ -150,9 +150,22 @@ void rds_tcp_reset_callbacks(struct socket *sock,
>  	 * 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()
> +	 * 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 queues one more shutdown pass, but rds_conn_shutdown()
> +	 * accepts RDS_CONN_ERROR in its final transition to
> +	 * RDS_CONN_DOWN, so the shutdown in flight completes normally.
>  	 */
> -	atomic_set(&cp->cp_state, RDS_CONN_RESETTING);
> +	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);

[Severity: Medium]
Does this break the post-condition that the only caller of
rds_tcp_reset_callbacks() encodes?

Previously the unconditional atomic_set() guaranteed the path was left in
RDS_CONN_RESETTING.  Now, when neither transition succeeds,
rds_conn_path_drop() leaves cp_state at RDS_CONN_ERROR, and
rds_tcp_reset_callbacks() returns with the path in that state.

In rds_tcp_accept_one() the state check under t_conn_path_lock explicitly
admits RDS_CONN_ERROR:

net/rds/tcp_listen.c:rds_tcp_accept_one() {
	conn_state = rds_conn_path_state(cp);
	WARN_ON(conn_state == RDS_CONN_UP);
	if (conn_state != RDS_CONN_CONNECTING && conn_state != RDS_CONN_ERROR) {
		rds_conn_path_drop(cp, 0);
		goto rst_nsk;
	}
	...
	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(cp, RDS_CONN_RESETTING);
	}

So for a path found in RDS_CONN_ERROR with osock != NULL, both cmpxchg
attempts fail, the drop re-stamps RDS_CONN_ERROR, and the following
rds_connect_path_complete(cp, RDS_CONN_RESETTING) can never make its
RESETTING -> UP transition.

That lands in the failure path:

net/rds/threads.c:rds_connect_path_complete() {
	if (!rds_conn_path_transition(cp, curr, RDS_CONN_UP)) {
		printk(KERN_WARNING "%s: Cannot transition to state UP, "
				"current state is %d\n", __func__,
				atomic_read(&cp->cp_state));
		rds_conn_path_drop(cp, false);
		return;
	}

which is an unconditional, non-ratelimited KERN_WARNING plus another drop,
rather than bringing the freshly installed socket up.  The same mismatch is
reachable when a peer FIN/RST turns the state into RDS_CONN_ERROR via
rds_tcp_state_change() -> rds_conn_path_drop() between the accept-side check
and these two cmpxchgs, since nothing serializes cp_state against
t_conn_path_lock.

Should the hard-coded RDS_CONN_RESETTING argument in rds_tcp_accept_one() and
the "rds_connect_path_complete() marks RDS_CONN_UP" comment be updated, or
should rds_tcp_reset_callbacks() report which branch it took so the caller
can skip the complete step?

[Severity: High]
This isn't a bug introduced by this patch, but the newly added
rds_conn_path_drop(cp, 0) here queues one more cp_down_w pass on cp->cp_wq
while the accept worker is still inside rds_tcp_reset_callbacks() and about
to release osock.  Can that widen the existing double-teardown window on the
same struct socket?

rds_tcp_reset_callbacks() samples the socket at entry:

net/rds/tcp.c:rds_tcp_reset_callbacks() {
	struct socket *osock = tc->t_sock;

	if (!osock)
		goto newsock;

and later, after the drop, unconditionally tears it down:

	rds_tcp_restore_callbacks(osock, tc);
	release_sock(osock->sk);
	sock_release(osock);

The shutdown worker latches the same pointer and does the same teardown:

net/rds/tcp_connect.c:rds_tcp_conn_path_shutdown() {
	struct socket *sock = tc->t_sock;
	...
	lock_sock()/rds_tcp_restore_callbacks()/sock_release()

with a drain loop of up to ~5 s in between.  Neither path holds a common
lock: rds_conn_path_drop() and rds_conn_shutdown() do not take
t_conn_path_lock, and the transport shutdown runs outside cp_cm_lock, so both
can sock_release() the same socket.

The pre-existing part is the unsynchronized sampling of tc->t_sock at the top
of rds_tcp_reset_callbacks().  Checking the states in which the new drop
actually fires with osock != NULL, each already had a shutdown queued, in
flight, or completed (with tc->t_sock already cleared), so I could not
identify a state where this patch alone creates a teardown race that did not
exist before.

Also worth noting for the series ordering: later commits in this series close
the window structurally, "net/rds: acquire RDS_IN_XMIT in
rds_tcp_reset_callbacks()" reads tc->t_sock only while owning RDS_IN_XMIT,
and "net/rds: acquire the fastpath locks in rds_conn_shutdown()" holds
RDS_IN_XMIT across conn_path_shutdown().  At this commit the window is still
open, so a bisect landing here still carries it.  Would it make sense to
order those two commits before this one?
-- 
pw-bot: cr