| Message ID | 20240801-upstream-net-next-20240801-tcp-limit-wake-up-x-syn-v1-1-3a87f977ad5f@kernel.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] tcp: limit wake-up for crossed SYN cases with SYN-ACK | expand |
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 :)
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
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
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;
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(-) --- base-commit: 743ff02152bc46bb4a2f2a49ec891c87eba6ab5b change-id: 20240801-upstream-net-next-20240801-tcp-limit-wake-up-x-syn-62e971a2accc Best regards,