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, it is then
required to also limit the check to packets containing the SYN-ACK
flags.
While at it, also update the attached comment: 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").
Fixes: 168a8f58059a ("tcp: TCP Fast Open Server - main code path")
Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com>
Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org>
---
Notes:
- The above 'Fixes' tag should correspond to the commit introducing the
possibility to have sk->sk_socket being set there in other cases than
the crossed SYN one. But I might have missed other cases. Maybe
1da177e4c3f4 ("Linux-2.6.12-rc2") might be safer? On the other hand,
I don't think this wake-up was causing any visible issue, apart from
not being needed.
---
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 bfe1bc69dc3e..5cebb389bf71 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;
--
2.45.2
On Thu, Jul 18, 2024 at 12:34 PM Matthieu Baerts (NGI0) <matttbe@kernel.org> wrote: > > 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, it is then > required to also limit the check to packets containing the SYN-ACK > flags. > > While at it, also update the attached comment: 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"). > > Fixes: 168a8f58059a ("tcp: TCP Fast Open Server - main code path") > Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com> > Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> > --- > Notes: > - The above 'Fixes' tag should correspond to the commit introducing the > possibility to have sk->sk_socket being set there in other cases than > the crossed SYN one. But I might have missed other cases. Maybe > 1da177e4c3f4 ("Linux-2.6.12-rc2") might be safer? On the other hand, > I don't think this wake-up was causing any visible issue, apart from > not being needed. This seems a net-next candidate to me ?
Hi Eric, Thank you for the review! On 23/07/2024 16:32, Eric Dumazet wrote: > On Thu, Jul 18, 2024 at 12:34 PM Matthieu Baerts (NGI0) > <matttbe@kernel.org> wrote: >> >> 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, it is then >> required to also limit the check to packets containing the SYN-ACK >> flags. >> >> While at it, also update the attached comment: 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"). >> >> Fixes: 168a8f58059a ("tcp: TCP Fast Open Server - main code path") >> Suggested-by: Kuniyuki Iwashima <kuniyu@amazon.com> >> Signed-off-by: Matthieu Baerts (NGI0) <matttbe@kernel.org> >> --- >> Notes: >> - The above 'Fixes' tag should correspond to the commit introducing the >> possibility to have sk->sk_socket being set there in other cases than >> the crossed SYN one. But I might have missed other cases. Maybe >> 1da177e4c3f4 ("Linux-2.6.12-rc2") might be safer? On the other hand, >> I don't think this wake-up was causing any visible issue, apart from >> not being needed. > > This seems a net-next candidate to me ? Fine by me! I modified this line mainly because Kuniyuki mentioned that it was the same check as the new one, modified in patch 1/2. I didn't find any visible issue with the wakeup, so I guess it can go to net-next. Cheers, Matt -- Sponsored by the NGI0 Core fund.
© 2016 - 2024 Red Hat, Inc.