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

[Mat Martineau <martineau@kernel.org>]

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