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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@xxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxx/
> > > ---
> >
> > 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