Re: [PATCH mptcp-next 0/3] mptcp: blackhole: sysctl SYN retrans + fix conditions

[Mat Martineau <martineau@kernel.org>]

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

> Recently, I found a network having weird behaviours with MPTCP packets:
>
> - The first connection to a server had a successful 3WHS, then MPTCP
>  options got stripped off.
>
> - The next one had the first SYN (with or without MPTCP) and 5
>  retransmissions dropped, before being apparently intercepted and
>  proxied to the end server.
>
> - (The next ones were sometimes intercepted, sometimes not, or dropped
>   at the beginning. I'm trying to find out which kind of "optimiser" is
>   causing this.)
>
> The result of this was a blackhole being "wrongly" detected, and no ways
> to force connections with quite a few SYN drops to finally use MPTCP at
> the end.
>
> In this series, we have:
>
> - A small fix for the doc.
>
> - A new sysctl to change the number of SYN retransmitted with MPTCP
>  options before falling back to TCP. The modification looks simple
>  enough to still be sent to netdev before the closure I think.
>

Hi Matthieu -

Patches 1 and 2 LGTM:

Reviewed-by: Mat Martineau <martineau@kernel.org>

> - A fix to only turn on the blackhole protection only when the first SYN
>  retransmitted without MPTCP option is accepted, instead of any after.
>  The blackhole feature was supposed to do that from the beginning, but
>  a check was wrongly placed. I think we should consider this as a fix,
>  even if there are also risks of not detecting a blackhole if the first
>  SYN retransmitted without MPTCP is dropped by accident. But that seems
>  more unlikely for an "MPTCP firewall blackhole", and I guess not all
>  future MPTCP connections will behave exactly like that. It sounds then
>  safer to reduce the possibilities of enabling the blackhole protection
>  by accident, and apply this patch.

I have one comment on patch 3, see my reply there.

- Mat

>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
> ---
> Matthieu Baerts (NGI0) (3):
>      doc: mptcp: sysctl: blackhole_timeout is per-netns
>      mptcp: sysctl: add syn_retrans_before_tcp_fallback
>      mptcp: blackhole only if 1st SYN retrans w/o MPC is accepted
>
> Documentation/networking/mptcp-sysctl.rst | 18 +++++++++++++++++-
> net/mptcp/ctrl.c                          | 25 +++++++++++++++++++------
> 2 files changed, 36 insertions(+), 7 deletions(-)
> ---
> base-commit: 9336324d1aec351496e048ec5b6bbda07944ad16
> change-id: 20250114-mpc-no-blackhole-526a61ea0334
>
> Best regards,
> -- 
> Matthieu Baerts (NGI0) <matttbe@kernel.org>
>
>
>