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

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

[Matthieu Baerts (NGI0)]

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>

[PATCH mptcp-next 1/3] doc: mptcp: sysctl: blackhole_timeout is per-netns

[Matthieu Baerts (NGI0)]

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 95598c21fc8e8782533bbc6a36de63bb465c297c..4ce31a2ac85be2daeb84c5447f06f69aabd18ef7 100644
--- a/Documentation/networking/mptcp-sysctl.rst
+++ b/Documentation/networking/mptcp-sysctl.rst
@@ -41,7 +41,7 @@ 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

[PATCH mptcp-next 2/3] mptcp: sysctl: add syn_retrans_before_tcp_fallback

[Matthieu Baerts (NGI0)]

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 4ce31a2ac85be2daeb84c5447f06f69aabd18ef7..03e1d3610333e29423b0f40591c9e914dc2d0366 100644
--- a/Documentation/networking/mptcp-sysctl.rst
+++ b/Documentation/networking/mptcp-sysctl.rst
@@ -108,3 +108,19 @@ 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 b0dd008e2114bce65ee3906bbdc19a5a4316cefa..3999e0ba2c35b50c36ce32277e0b8bfb24197946 100644
--- a/net/mptcp/ctrl.c
+++ b/net/mptcp/ctrl.c
@@ -32,6 +32,7 @@ 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;
@@ -92,6 +93,7 @@ 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;
@@ -245,6 +247,12 @@ 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)
@@ -269,6 +277,7 @@ 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));
@@ -392,17 +401,21 @@ 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

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

[Matthieu Baerts (NGI0)]

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.1

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

[Mat Martineau]

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

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

[Matthieu Baerts]

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.

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

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 4231 bytes --]

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

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

[Matthieu Baerts]

Hi Mat,

Thank you for your reply!

On 21/01/2025 23:36, Mat Martineau wrote:
> 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?

That's what I would have normally done, but because it was just before
the v6.13 release, it would have blocked patch 2 which is now in net-next.

>> 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 :)

Sure, I will post a v2. So because patch 1 & 2 have already been
applied, this v2 will contain this patch (fix, easy to backport) and the
refactoring. We can then decide if we keep the patches separated, or if
we split them.

If we prefer to squash the two of them, that's OK. We will have
conflicts with the <= v6.13, but not complex to resolve. (By doing that,
Sasha might even suggest to backport the 2nd patch of this v1 series --
the new sysctl entry -- as well, which is fine by me.)

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

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

[MPTCP CI]

Hi Matthieu,

Thank you for your modifications, that's great!

Our CI did some validations and here is its report:

- KVM Validation: normal: Success! ✅
- KVM Validation: debug: Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Unstable: 1 failed test(s): bpftest_test_progs_mptcp 🔴
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/12773481101

Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/f4011cb563e8
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=925349


If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:

    $ cd [kernel source code]
    $ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
        --pull always mptcp/mptcp-upstream-virtme-docker:latest \
        auto-normal

For more details:

    https://github.com/multipath-tcp/mptcp-upstream-virtme-docker


Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)

Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)

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

[Mat Martineau]

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>
>
>
>

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

[Matthieu Baerts]

H Mat,

On 17/01/2025 01:13, Mat Martineau wrote:
> 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>

Thank you for the review!

I already applied these two patches:

New patches for t/upstream-net and t/upstream:
- 34dc1c307f30: doc: mptcp: sysctl: blackhole_timeout is per-netns
- Results: 1b76b0bd2522..551c1dba2a68 (export-net)
- Results: 2a6dfa001e66..2d16bea74b90 (export)

New patches for t/upstream:
- 5fb87b001602: mptcp: sysctl: add syn_retrans_before_tcp_fallback
- Results: 2d16bea74b90..b7be33140cc3 (export)

Tests are now in progress:

- export-net:
https://github.com/multipath-tcp/mptcp_net-next/commit/70769bbce0f766c3583cb1caa791722685884f19/checks
- export:
https://github.com/multipath-tcp/mptcp_net-next/commit/caa6490e85f954f97ad955a2496c1b1297efee89/checks

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.