netdev - Re: [PATCH net-next] tcp: limit wake-up for crossed SYN cases with SYN-ACK

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <79e327c5-f87f-4295-b461-81c09a699af6@kernel.org>
Date: Fri, 2 Aug 2024 10:48:26 +0200
From: Matthieu Baerts <matttbe@...nel.org>
To: Eric Dumazet <edumazet@...gle.com>
Cc: mptcp@...ts.linux.dev, "David S. Miller" <davem@...emloft.net>,
 David Ahern <dsahern@...nel.org>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org, Kuniyuki Iwashima <kuniyu@...zon.com>
Subject: Re: [PATCH net-next] tcp: limit wake-up for crossed SYN cases with
 SYN-ACK

On 01/08/2024 19:52, Eric Dumazet wrote:
> On Thu, Aug 1, 2024 at 6:39 PM Matthieu Baerts (NGI0)
> <matttbe@...nel.org> wrote:
>>
>> In TCP_SYN_RECV states, sk->sk_socket will be assigned in case of
>> marginal crossed SYN, but also in other cases, e.g.
>>
>>  - With TCP Fast Open, if the connection got accept()'ed before
>>    receiving the 3rd ACK ;
>>
>>  - With MPTCP, when accepting additional subflows to an existing MPTCP
>>    connection.
>>
>> In these cases, the switch to TCP_ESTABLISHED is done when receiving the
>> 3rd ACK, without the SYN flag then.
>>
>> To properly restrict the wake-up to crossed SYN cases as expected there,
>> it is then required to also limit the check to packets containing the
>> SYN-ACK flags.
>>
>> Without this modification, it looks like the wake-up was not causing any
>> visible issue with TFO and MPTCP, apart from not being needed. That's
>> why this patch doesn't contain a Cc to stable, and a Fixes tag.
>>
>> While at it, the attached comment has also been updated: sk->sk_sleep
>> has been removed in 2010, and replaced by sk->sk_wq in commit
>> 43815482370c ("net: sock_def_readable() and friends RCU conversion").
>>
>> Suggested-by: Kuniyuki Iwashima <kuniyu@...zon.com>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@...nel.org>
>> ---
>> Notes:
>>   - This is the same patch as the one suggested earlier in -net as part
>>     of another series, but targeting net-next (Eric), and with an
>>     updated commit message. The previous version was visible there:
>>     https://lore.kernel.org/20240718-upstream-net-next-20240716-tcp-3rd-ack-consume-sk_socket-v2-2-d653f85639f6@kernel.org/
>> ---
> 
> Note: I am not aware of any tests using FASYNC
> 
> sock_wake_async() / kill_fasync() are sending signals, not traditional wakeups.

Thank you for the review and the explanation!

> Do we really want to potentially break some applications still using
> pre-multi-thread era async io ?

They are potentially already broken if we don't test them :-D

> Not that I really care, but I wonder why you care :)

More seriously, I sent this patch, because in previous discussions about
the crossed SYN case, Kuniyuki mentioned that he used the same condition
as the one I modified here. I didn't see why it is needed to send such
signal there for TFO and MPTCP cases, so I sent this patch. On the other
hand, I suppose such old apps relying on FASYNC will not natively use
TFO or MPTCP (except if they are forced externally).

In other words, I'm not fixing a problem I saw here, I'm only
restricting the condition to crossed SYN case, as mentioned in the
linked comment, then excluding TFO and MPTCP cases that don't seem to
require this signal at that moment. But then it's fine for me to drop
this patch if you think it is not needed :)

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