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

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

[Matthieu Baerts (NGI0)]

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@amazon.com>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.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/
---
 net/ipv4/tcp_input.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 454362e359da..b2d2c843ecd2 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6797,9 +6797,9 @@ tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
 
 		/* Note, that this wakeup is only for marginal crossed SYN case.
 		 * Passively open sockets are not waked up, because
-		 * sk->sk_sleep == NULL and sk->sk_socket == NULL.
+		 * sk->sk_wq == NULL and sk->sk_socket == NULL.
 		 */
-		if (sk->sk_socket)
+		if (sk->sk_socket && th->syn)
 			sk_wake_async(sk, SOCK_WAKE_IO, POLL_OUT);
 
 		tp->snd_una = TCP_SKB_CB(skb)->ack_seq;

---
base-commit: 743ff02152bc46bb4a2f2a49ec891c87eba6ab5b
change-id: 20240801-upstream-net-next-20240801-tcp-limit-wake-up-x-syn-62e971a2accc

Best regards,
-- 
Matthieu Baerts (NGI0) <matttbe@kernel.org>

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

[Eric Dumazet]

On Thu, Aug 1, 2024 at 6:39 PM Matthieu Baerts (NGI0)
<matttbe@kernel.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@amazon.com>
> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.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.

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

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

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

[Matthieu Baerts]

On 01/08/2024 19:52, Eric Dumazet wrote:
> On Thu, Aug 1, 2024 at 6:39 PM Matthieu Baerts (NGI0)
> <matttbe@kernel.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@amazon.com>
>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.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.

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

[Paolo Abeni]

On 8/2/24 10:48, Matthieu Baerts wrote:
> On 01/08/2024 19:52, Eric Dumazet wrote:
>> On Thu, Aug 1, 2024 at 6:39 PM Matthieu Baerts (NGI0)
>> <matttbe@kernel.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@amazon.com>
>>> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.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 :)

AFAICS this patch makes the code consistent with the associated comment, 
but does not address any specific/real issue, right?

If I read correctly, with the patch applied the stack will skip  wakeup 
signals for some TFO and MPTCP edge cases. Such signals are not 
duplicates, and existing (legacy) application could rely on them for 
correct functionality.

I think that risk of a the potential regression offsets the (unclear to 
me) benefit. I'm dropping this patch.

Thanks,

Paolo