:p
atchew
Login
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. - 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. 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>
All other sysctl entries mention it, and it is a per-namespace sysctl. So mention it as well. Fixes: 27069e7cb3d1 ("mptcp: disable active MPTCP in case of blackhole") Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Documentation/networking/mptcp-sysctl.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/networking/mptcp-sysctl.rst b/Documentation/networking/mptcp-sysctl.rst index XXXXXXX..XXXXXXX 100644 --- a/Documentation/networking/mptcp-sysctl.rst +++ b/Documentation/networking/mptcp-sysctl.rst @@ -XXX,XX +XXX,XX @@ blackhole_timeout - INTEGER (seconds) MPTCP is re-enabled and will reset to the initial value when the blackhole issue goes away. - 0 to disable the blackhole detection. + 0 to disable the blackhole detection. This is a per-namespace sysctl. Default: 3600 -- 2.47.1
The number of SYN + MPC retransmissions before falling back to TCP was fixed to 2. This is certainly a good default value, but having a fixed number can be problem in some environments. The current behaviour means that if all packets are dropped, there will be: - The initial SYN + MPC - 2 retransmissions with MPC - The next ones will be without MPTCP. So typically ~3 seconds before falling back to TCP. In some networks where some temporally blackholes are unfortunately frequent, or when a client tries to initiate connections while the network is not ready yet, this can cause new connections not to have MPTCP connections. In such environments, it is now possible to increase the number of SYN retransmissions with MPTCP options to make sure MPTCP is used. Interesting values are: - 0: the first retransmission will be done without MPTCP options: quite aggressive, but also a higher risk of detecting false-positive MPTCP blackholes. - >= 128: all SYN retransmissions will keep the MPTCP options: back to the < 6.12 behaviour. The default behaviour is not changed here. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Documentation/networking/mptcp-sysctl.rst | 16 ++++++++++++++++ net/mptcp/ctrl.c | 21 +++++++++++++++++---- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/Documentation/networking/mptcp-sysctl.rst b/Documentation/networking/mptcp-sysctl.rst index XXXXXXX..XXXXXXX 100644 --- a/Documentation/networking/mptcp-sysctl.rst +++ b/Documentation/networking/mptcp-sysctl.rst @@ -XXX,XX +XXX,XX @@ stale_loss_cnt - INTEGER This is a per-namespace sysctl. Default: 4 + +syn_retrans_before_tcp_fallback - INTEGER + The number of SYN + MP_CAPABLE retransmissions before falling back to + TCP, i.e. dropping the MPTCP options. In other words, if all the packets + are dropped on the way, there will be: + + * The initial SYN with MPTCP support + * This number of SYN retransmitted with MPTCP support + * The next SYN retransmissions will be without MPTCP support + + 0 means the first retransmission will be done without MPTCP options. + >= 128 means that all SYN retransmissions will keep the MPTCP options. A + lower number might increase false-positive MPTCP blackholes detections. + This is a per-namespace sysctl. + + Default: 2 diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/ctrl.c +++ b/net/mptcp/ctrl.c @@ -XXX,XX +XXX,XX @@ struct mptcp_pernet { unsigned int close_timeout; unsigned int stale_loss_cnt; atomic_t active_disable_times; + u8 syn_retrans_before_tcp_fallback; unsigned long active_disable_stamp; u8 mptcp_enabled; u8 checksum_enabled; @@ -XXX,XX +XXX,XX @@ static void mptcp_pernet_set_defaults(struct mptcp_pernet *pernet) pernet->mptcp_enabled = 1; pernet->add_addr_timeout = TCP_RTO_MAX; pernet->blackhole_timeout = 3600; + pernet->syn_retrans_before_tcp_fallback = 2; atomic_set(&pernet->active_disable_times, 0); pernet->close_timeout = TCP_TIMEWAIT_LEN; pernet->checksum_enabled = 0; @@ -XXX,XX +XXX,XX @@ static struct ctl_table mptcp_sysctl_table[] = { .proc_handler = proc_blackhole_detect_timeout, .extra1 = SYSCTL_ZERO, }, + { + .procname = "syn_retrans_before_tcp_fallback", + .maxlen = sizeof(u8), + .mode = 0644, + .proc_handler = proc_dou8vec_minmax, + }, }; static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet *pernet) @@ -XXX,XX +XXX,XX @@ static int mptcp_pernet_new_table(struct net *net, struct mptcp_pernet *pernet) /* table[7] is for available_schedulers which is read-only info */ table[8].data = &pernet->close_timeout; table[9].data = &pernet->blackhole_timeout; + table[10].data = &pernet->syn_retrans_before_tcp_fallback; hdr = register_net_sysctl_sz(net, MPTCP_SYSCTL_PATH, table, ARRAY_SIZE(mptcp_sysctl_table)); @@ -XXX,XX +XXX,XX @@ void mptcp_active_enable(struct sock *sk) void mptcp_active_detect_blackhole(struct sock *ssk, bool expired) { struct mptcp_subflow_context *subflow; - u32 timeouts; if (!sk_is_mptcp(ssk)) return; - timeouts = inet_csk(ssk)->icsk_retransmits; subflow = mptcp_subflow_ctx(ssk); if (subflow->request_mptcp && ssk->sk_state == TCP_SYN_SENT) { - if (timeouts == 2 || (timeouts < 2 && expired)) { - MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_MPCAPABLEACTIVEDROP); + 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)) { + MPTCP_INC_STATS(net, MPTCP_MIB_MPCAPABLEACTIVEDROP); subflow->mpc_drop = 1; mptcp_subflow_early_fallback(mptcp_sk(subflow->conn), subflow); } else { -- 2.47.1
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 XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/ctrl.c +++ b/net/mptcp/ctrl.c @@ -XXX,XX +XXX,XX @@ 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; } + } else if (ssk->sk_state == TCP_SYN_SENT) { + subflow->mpc_drop = 0; } } -- 2.47.1
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. (applied) - 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. (applied) - 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. - A small cleanup to exit early. Please note that patches 1 and 2 have already been applied in our tree. So this version only has patches 3 and 4. Patch 3 is a fix, maybe not patch 4, but it might be easier to squash patch 3 and 4 and send it as a fix to -net. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Changes in v2: - Previous patches 1 & 2 have been applied. - Patch 1 (ex 3) has not been modified. - Patch 2 has been added, it can be squashed to the previous one. - Link to v1: https://lore.kernel.org/r/20250114-mpc-no-blackhole-v1-0-994bd2a357fb@kernel.org --- Matthieu Baerts (NGI0) (2): mptcp: blackhole only if 1st SYN retrans w/o MPC is accepted mptcp: blackhole: avoid checking the state twice net/mptcp/ctrl.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) --- base-commit: a9a670c7f9f94ac66e6a79ecb81e4e7a6e20f001 change-id: 20250114-mpc-no-blackhole-526a61ea0334 Best regards, -- Matthieu Baerts (NGI0) <matttbe@kernel.org>
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 XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/ctrl.c +++ b/net/mptcp/ctrl.c @@ -XXX,XX +XXX,XX @@ 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; } + } else if (ssk->sk_state == TCP_SYN_SENT) { + subflow->mpc_drop = 0; } } -- 2.47.1
A small cleanup, reordering the conditions to avoid checking things twice. The code here is called in case of timeout on a TCP connection, before triggering a retransmission. But it only acts on SYN + MPC packets. So the conditions can be re-order to exit early in case of non-MPTCP SYN + MPC. This also reduce the indentation levels. Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> --- Notes: if it is easier, this patch can be squashed in the previous one, and sent as a fix to -net. There will be conflicts with the previous versions, but not complex to fix -- and the new sysctl could even be backported if that's what the stable team prefers. --- net/mptcp/ctrl.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/net/mptcp/ctrl.c b/net/mptcp/ctrl.c index XXXXXXX..XXXXXXX 100644 --- a/net/mptcp/ctrl.c +++ b/net/mptcp/ctrl.c @@ -XXX,XX +XXX,XX @@ void mptcp_active_enable(struct sock *sk) void mptcp_active_detect_blackhole(struct sock *ssk, bool expired) { struct mptcp_subflow_context *subflow; + u8 timeouts, to_max; + struct net *net; - if (!sk_is_mptcp(ssk)) + /* Only check MPTCP SYN ... */ + if (likely(!sk_is_mptcp(ssk) || ssk->sk_state != TCP_SYN_SENT)) return; subflow = mptcp_subflow_ctx(ssk); - 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)) { - MPTCP_INC_STATS(net, MPTCP_MIB_MPCAPABLEACTIVEDROP); - subflow->mpc_drop = 1; - mptcp_subflow_early_fallback(mptcp_sk(subflow->conn), subflow); - } - } else if (ssk->sk_state == TCP_SYN_SENT) { + /* ... + MP_CAPABLE */ + if (!subflow->request_mptcp) { + /* Mark as blackhole iif the 1st non-MPTCP SYN is accepted */ subflow->mpc_drop = 0; + return; + } + + net = sock_net(ssk); + 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)) { + MPTCP_INC_STATS(net, MPTCP_MIB_MPCAPABLEACTIVEDROP); + subflow->mpc_drop = 1; + mptcp_subflow_early_fallback(mptcp_sk(subflow->conn), subflow); } } -- 2.47.1