Re: [PATCH mptcp-next v15 06/11] mptcp: align FIN handling with TCP via SOCK_DONE

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: SOCK_DONE set on fallback TCP RST via subflow_is_done() masks sk_err
--

commit d076bfa67e731d6a4dc57a834f527b4e82a636fb
Author: Geliang Tang <geliang@kernel.org>

mptcp: align FIN handling with TCP via SOCK_DONE

This commit aligns MPTCP FIN reception handling with TCP by setting SOCK_DONE
in mptcp_check_data_fin(), checking it in mptcp_inq_hint() and mptcp_recvmsg()
before sock_error(), and resetting it in mptcp_disconnect().

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 3c3ad8991799a..7b0005d47bead 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -740,6 +740,7 @@ static void mptcp_check_data_fin(struct sock *sk)
>  
>  		WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | RCV_SHUTDOWN);
>  		smp_mb__before_atomic(); /* SHUTDOWN must be visible first */
> +		sock_set_flag(sk, SOCK_DONE);
>  
>  		switch (sk->sk_state) {
>  		case TCP_ESTABLISHED:
> @@ -2423,7 +2424,7 @@ static unsigned int mptcp_inq_hint(const struct sock *sk)
>  		return INT_MAX;
>  
>  	if (!hint_val &&
> -	    (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
> +	    sock_flag(sk, SOCK_DONE))
>  		return 1;
>  
>  	return (unsigned int)hint_val;
> @@ -2495,6 +2496,9 @@ static int mptcp_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
>  			    !timeo)
>  				break;
>  		} else {
> +			if (sock_flag(sk, SOCK_DONE))
> +				break;
> +
>  			copied = sock_error(sk);
>  			if (copied)
>  				break;

[Severity: High]
Can checking sock_flag(sk, SOCK_DONE) before sock_error(sk) in
mptcp_recvmsg() mask a connection reset error on a fallback MPTCP connection
when the peer aborts with a TCP reset?

When a peer sends a TCP reset on a fallback subflow (msk->first),
tcp_reset() -> tcp_done_with_error() -> tcp_done() sets sk_err, sk_state,
and sk_shutdown on the subflow socket without setting SOCK_DONE, and invokes
subflow_state_change():

net/mptcp/subflow.c:subflow_state_change() {
	...
	if (mptcp_subflow_data_available(sk))
		mptcp_data_ready(parent, sk);
	subflow_sched_work_if_closed(mptcp_sk(parent), sk);
	...
}

In subflow_sched_work_if_closed(), subflow_is_done() checks:

net/mptcp/subflow.c:subflow_is_done() {
	return sk->sk_shutdown & RCV_SHUTDOWN || sk->sk_state == TCP_CLOSE;
}

Because subflow_is_done(ssk) returns true on a TCP reset,
subflow_sched_work_if_closed() generates a synthetic data fin:

net/mptcp/subflow.c:subflow_sched_work_if_closed() {
	...
	if (__mptcp_check_fallback(msk) && subflow_is_done(ssk) &&
	    msk->first == ssk &&
	    mptcp_update_rcv_data_fin(msk, subflow->map_seq +
				      subflow->map_data_len, true))
		mptcp_schedule_work(sk);
	...
}

If the parent msk is owned by user context when subflow_error_report(ssk)
subsequently runs (or if mptcp_worker acquires the msk socket lock before
subflow_error_report() acquires mptcp_data_lock), error and state propagation
to the parent msk is deferred via MPTCP_ERROR_REPORT in msk->cb_flags,
leaving sk->sk_state in TCP_ESTABLISHED.

Before MPTCP_ERROR_REPORT is processed at the end of mptcp_release_cb(),
mptcp_check_data_fin() can run while sk->sk_state is still TCP_ESTABLISHED
(for example via __mptcp_move_skbs() at the start of mptcp_release_cb(),
mptcp_move_skbs() in mptcp_recvmsg() or mptcp_ioctl(), or in mptcp_worker()):

net/mptcp/protocol.c:mptcp_release_cb() {
	for (;;) {
		...
		if (spool_bl && __mptcp_move_skbs(sk, &skbs, &moved)) {
			/* notify ack seq update */
			mptcp_cleanup_rbuf(msk, 0);
			sk->sk_data_ready(sk);
		}
		...
	}
	if (flags) {
		...
		if (__test_and_clear_bit(MPTCP_ERROR_REPORT, &msk->cb_flags))
			__mptcp_error_report(sk);
	}
	...
}

When mptcp_check_data_fin() runs in that window, it increments msk->ack_seq
and sets SOCK_DONE on the parent msk. Once __mptcp_error_report() then
propagates sk->sk_err, wouldn't mptcp_recvmsg() hit the new
sock_flag(sk, SOCK_DONE) check before sock_error(sk) and return 0 instead of
the connection reset error, while mptcp_inq_hint() returns 1 instead of 0?

Should subflow_is_done() also check sock_flag(sk, SOCK_DONE) (or check for
the absence of sk->sk_err) so that an abortive reset on a fallback subflow
does not synthesize a clean data fin?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792718041.git.tanggeliang@kylinos.cn?part=6