Re: [PATCH net] tcp: Fix error ts_recent time during three-way handshake

[Wang Hai <wanghai38@huawei.com>]

On 2025/2/18 20:02, Jason Xing wrote:
> Hi Wang,
> 
> On Tue, Feb 18, 2025 at 7:02 PM Wang Hai <wanghai38@huawei.com> wrote:
>>
>> If two ack packets from a connection enter tcp_check_req at the same time
>> through different cpu, it may happen that req->ts_recent is updated with
>> with a more recent time and the skb with an older time creates a new sock,
>> which will cause the tcp_validate_incoming check to fail.
>>
>> cpu1                                cpu2
>> tcp_check_req
>>                                      tcp_check_req
>> req->ts_recent = tmp_opt.rcv_tsval = t1
>>                                      req->ts_recent = tmp_opt.rcv_tsval = t2
>>
>> newsk->ts_recent = req->ts_recent = t2 // t1 < t2
>> tcp_child_process
>> tcp_rcv_state_process
>> tcp_validate_incoming
>> tcp_paws_check
>> if ((s32)(rx_opt->ts_recent - rx_opt->rcv_tsval) <= paws_win) // failed
>>
>> In tcp_check_req, restore ts_recent to this skb's to fix this bug.
> 
> It's known that it's possible to receive two packets in two different
> cpus like this case nearly at the same time. I'm curious if the socket
> was running on the loopback?
> 
Hi Jason,

Yeah, it's running in loopback.
> Even if the above check that you commented in tcp_paws_check() fails,
> how about the rest of the checks in tcp_validate_incoming()? 

The skb from cpu1 is a valid skb, so it should have passed the
tcp_validate_incoming check, but the current concurrency issue
prevented it from passing the check.

5951 static bool tcp_validate_incoming(struct sock *sk, struct sk_buff *skb,
5952                                   const struct tcphdr *th, int 
syn_inerr)
5953 {
5954         struct tcp_sock *tp = tcp_sk(sk);
5955         SKB_DR(reason);
5956
5957         /* RFC1323: H1. Apply PAWS check first. */
5958         if (!tcp_fast_parse_options(sock_net(sk), skb, th, tp) ||
5959             !tp->rx_opt.saw_tstamp ||
5960             tcp_paws_check(&tp->rx_opt, TCP_PAWS_WINDOW)) // failed
5961                 goto step1;
5962
5963         reason = tcp_disordered_ack_check(sk, skb);
5964         if (!reason) // failed
5965                 goto step1;
5966         /* Reset is accepted even if it did not pass PAWS. */
5967         if (th->rst) // failed
5968                 goto step1;
5969         if (unlikely(th->syn)) // failed
5970                 goto syn_challenge;
5971
5972         /* Old ACK are common, increment PAWS_OLD_ACK
5973          * and do not send a dupack.
5974          */
5975         if (reason == SKB_DROP_REASON_TCP_RFC7323_PAWS_ACK) {
5976                 NET_INC_STATS(sock_net(sk), LINUX_MIB_PAWS_OLD_ACK);
5977                 goto discard;
5978         }
5979         NET_INC_STATS(sock_net(sk), LINUX_MIB_PAWSESTABREJECTED);
5980         if (!tcp_oow_rate_limited(sock_net(sk), skb,
5981                                   LINUX_MIB_TCPACKSKIPPEDPAWS,
5982                                   &tp->last_oow_ack_time))
5983                 tcp_send_dupack(sk, skb);
5984         goto discard; // Drop the skb from here.
> test, I doubt if really that check failed which finally caused the skb
> from CPU2 to be discarded. Let me put it this way, is the paws_win
> check the root cause? What is the drop reason for
> tcp_validate_incoming()?
> 
The values of ts_recent and rcv_tsval are compared in tcp_paws_check,
where ts_recent comes from req->ts_recent = t2 and rcv_tsval is the
current cpu1 skb time. ts_recent - rcv_tsval may be greater than 1
and here will fail here. Tcp_paws_check fails causing 
tcp_validate_incoming to fail too.

> Thanks,
> Jason
> 
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>> Signed-off-by: Wang Hai <wanghai38@huawei.com>
>> ---
>>   net/ipv4/tcp_minisocks.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/net/ipv4/tcp_minisocks.c b/net/ipv4/tcp_minisocks.c
>> index b089b08e9617..0208455f9eb8 100644
>> --- a/net/ipv4/tcp_minisocks.c
>> +++ b/net/ipv4/tcp_minisocks.c
>> @@ -878,6 +878,10 @@ struct sock *tcp_check_req(struct sock *sk, struct sk_buff *skb,
>>          sock_rps_save_rxhash(child, skb);
>>          tcp_synack_rtt_meas(child, req);
>>          *req_stolen = !own_req;
>> +       if (own_req && tcp_sk(child)->rx_opt.tstamp_ok &&
>> +           unlikely(tcp_sk(child)->rx_opt.ts_recent != tmp_opt.rcv_tsval))
>> +               tcp_sk(child)->rx_opt.ts_recent = tmp_opt.rcv_tsval;
>> +
>>          return inet_csk_complete_hashdance(sk, child, req, own_req);
>>
>>   listen_overflow:
>> --
>> 2.17.1
>>
>>