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

[Matthieu Baerts <matttbe@kernel.org>]

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.