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
@@ -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;
}
+ } else if (ssk->sk_state == TCP_SYN_SENT) {
+ subflow->mpc_drop = 0;
}
}
--
2.47.1On 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
Hi Mat,
On 17/01/2025 01:26, Mat Martineau wrote:
> 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.
Good point!
I forgot to mention that, but I duplicated the simple check to avoid
conflicts with the backports. But on the other hand, it will only
conflict with the previous patch. So if you prefer, and not to block it
the patch for net-next, I can also send this fix patch later on.
If we decide to go into that direction, I suggest moving the SYN_SENT
check at the beginning:
if (!sk_is_mptcp(ssk) || ssk->sk_state != TCP_SYN_SENT)
return;
subflow = mptcp_subflow_ctx(ssk);
if (!subflow->request_mptcp) {
subflow->mpc_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)) {
(...)
WDYT?
(Or I can also keep the fix like it is for the moment, and add another
patch containing this refactoring).
Cheers,
Matt
--
Sponsored by the NGI0 Core fund.
On Fri, 17 Jan 2025, Matthieu Baerts wrote:
> Hi Mat,
>
> On 17/01/2025 01:26, Mat Martineau wrote:
>> 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.
>
> Good point!
>
> I forgot to mention that, but I duplicated the simple check to avoid
> conflicts with the backports. But on the other hand, it will only
> conflict with the previous patch. So if you prefer, and not to block it
> the patch for net-next, I can also send this fix patch later on.
>
Hi Matthieu -
Ok, I see. How about reversing the order of patches 2 & 3 to get rid of
the dependency?
> If we decide to go into that direction, I suggest moving the SYN_SENT
> check at the beginning:
>
> if (!sk_is_mptcp(ssk) || ssk->sk_state != TCP_SYN_SENT)
> return;
>
> subflow = mptcp_subflow_ctx(ssk);
>
> if (!subflow->request_mptcp) {
> subflow->mpc_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)) {
>
> (...)
>
> WDYT?
>
Good suggestion, that's a clearer way to do it.
> (Or I can also keep the fix like it is for the moment, and add another
> patch containing this refactoring).
Not sure if you're referring to my suggestion or not. Probably the
clearest option is to post a v2 based on this discussion :)
- Mat
© 2016 - 2025 Red Hat, Inc.