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,

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.