Re: [PATCH mptcp-next 3/3] mptcp: blackhole only if 1st SYN retrans w/o MPC is accepted

[Mat Martineau <martineau@kernel.org>]

On Tue, 14 Jan 2025, Matthieu Baerts (NGI0) wrote:

> The Fixes commit mentioned this:
>
>> An MPTCP firewall blackhole can be detected if the following SYN
>> retransmission after a fallback to "plain" TCP is accepted.
>
> But in fact, this blackhole was detected if any following SYN
> retransmissions after a fallback to TCP was accepted.
>
> That's because 'mptcp_subflow_early_fallback()' will set 'request_mptcp'
> to 0, and 'mpc_drop' will never be reset to 0 after.
>
> This is an issue, because some not so unusual situations might cause the
> kernel to detect a false-positive blackhole, e.g. a client trying to
> connect to a server while the network is not ready yet, causing a few
> SYN retransmissions, before reaching the end server.
>
> Fixes: 27069e7cb3d1 ("mptcp: disable active MPTCP in case of blackhole")
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> net/mptcp/ctrl.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c
> index 3999e0ba2c35b50c36ce32277e0b8bfb24197946..2dd81e6c26bdb5220abed68e26d70d2dc3ab14fb 100644
> --- a/net/mptcp/ctrl.c
> +++ b/net/mptcp/ctrl.c

Some more context before the diff hunk:

> 	if (subflow->request_mptcp && ssk->sk_state == TCP_SYN_SENT) {
> 		struct net *net = sock_net(ssk);
> 		u8 timeouts, to_max;
>
> 		timeouts = inet_csk(ssk)->icsk_retransmits;
> 		to_max = mptcp_get_pernet(net)->syn_retrans_before_tcp_fallback;
>
> 		if (timeouts == to_max || (timeouts < to_max && expired)) {

I think it would help to change the above code to:

> 	if (ssk->sk_state == TCP_SYN_SENT) {
> 		struct net *net = sock_net(ssk);
> 		u8 timeouts, to_max;
>
>		if (!subflow->request_mptcp) {
> 			subflow->mptcp_drop = 0;
> 			return;
> 		}
>
> 		timeouts = inet_csk(ssk)->icsk_retransmits;
> 		to_max = mptcp_get_pernet(net)->syn_retrans_before_tcp_fallback;
>
> 		if (timeouts == to_max || (timeouts < to_max && expired)) {

(end of added hunk)

> @@ -418,9 +418,9 @@ void mptcp_active_detect_blackhole(struct sock *ssk, bool expired)
> 			MPTCP_INC_STATS(net, MPTCP_MIB_MPCAPABLEACTIVEDROP);
> 			subflow->mpc_drop = 1;
> 			mptcp_subflow_early_fallback(mptcp_sk(subflow->conn), subflow);
> -		} else {
> -			subflow->mpc_drop = 0;
> 		}

And drop this from the patch:

> +	} else if (ssk->sk_state == TCP_SYN_SENT) {
> +		subflow->mpc_drop = 0;

That way ssk->sk_state is only checked once.

- Mat