Re: [PATCH mptcp-next 2/6] mptcp: prevent dummy DATA_FIN on RST in fallback mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: subflow_is_done() checks sk_err after sock_error() clears it
--

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

mptcp: prevent dummy DATA_FIN on RST in fallback mode

This commit checks sk->sk_err in subflow_is_done() to prevent creating a
dummy DATA_FIN when a fallback subflow is aborted with a TCP RST.

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 78e886b202585..a53c83644a40b 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1327,6 +1327,9 @@ static void mptcp_subflow_discard_data(struct sock *ssk, struct sk_buff *skb,
>  
>  static bool subflow_is_done(const struct sock *sk)
>  {
> +	if (READ_ONCE(sk->sk_err))
> +		return false;

[Severity: Medium]
Can sk->sk_err already be cleared to 0 before subflow_sched_work_if_closed()
checks subflow_is_done()?

When a fallback subflow receives a TCP RST in softirq, tcp_reset() ->
tcp_done_with_error() sets sk->sk_err and invokes subflow_state_change():

net/mptcp/subflow.c:subflow_state_change() {
	if (mptcp_subflow_data_available(sk))
		mptcp_data_ready(parent, sk);
	else if (unlikely(READ_ONCE(sk->sk_err)))
		subflow_error_report(sk);

	subflow_sched_work_if_closed(mptcp_sk(parent), sk);
}

When the parent MPTCP socket is not owned by userspace
(!sock_owned_by_user(parent)), both subflow_error_report() (via
__mptcp_error_report()) and mptcp_data_ready() (via move_skbs_to_msk() after
draining the subflow receive queue) immediately call
__mptcp_subflow_error_report(parent, ssk):

net/mptcp/protocol.c:__mptcp_subflow_error_report() {
	if (sk->sk_state != TCP_SYN_SENT && !__mptcp_check_fallback(mptcp_sk(sk)))
		goto out;

	err = sock_error(ssk);
	if (!err)
		goto out;
}

Because sock_error(ssk) executes xchg(&ssk->sk_err, 0), ssk->sk_err is
atomically reset to 0 while propagating the error to parent->sk_err.

When subflow_state_change() subsequently calls
subflow_sched_work_if_closed(mptcp_sk(parent), sk), won't subflow_is_done(ssk)
observe READ_ONCE(sk->sk_err) == 0, return true because RCV_SHUTDOWN and
TCP_CLOSE are set, and still generate the dummy DATA_FIN via
mptcp_update_rcv_data_fin()?

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