| Message ID | 20240718-upstream-net-next-20240716-tcp-3rd-ack-consume-sk_socket-v2-2-d653f85639f6@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | tcp: restrict crossed SYN specific actions to SYN-ACK | expand |
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
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;
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(-)