Re: [PATCH net v4] Fix race for duplicate reqsk on identical SYN

[luoxuanqiang <luoxuanqiang@kylinos.cn>]

在 2024/6/25 17:49, Paolo Abeni 写道:
> On Fri, 2024-06-21 at 09:39 +0800, luoxuanqiang wrote:
>> When bonding is configured in BOND_MODE_BROADCAST mode, if two identical
>> SYN packets are received at the same time and processed on different CPUs,
>> it can potentially create the same sk (sock) but two different reqsk
>> (request_sock) in tcp_conn_request().
>>
>> These two different reqsk will respond with two SYNACK packets, and since
>> the generation of the seq (ISN) incorporates a timestamp, the final two
>> SYNACK packets will have different seq values.
>>
>> The consequence is that when the Client receives and replies with an ACK
>> to the earlier SYNACK packet, we will reset(RST) it.
>>
>> ========================================================================
>>
>> This behavior is consistently reproducible in my local setup,
>> which comprises:
>>
>>                    | NETA1 ------ NETB1 |
>> PC_A --- bond --- |                    | --- bond --- PC_B
>>                    | NETA2 ------ NETB2 |
>>
>> - PC_A is the Server and has two network cards, NETA1 and NETA2. I have
>>    bonded these two cards using BOND_MODE_BROADCAST mode and configured
>>    them to be handled by different CPU.
>>
>> - PC_B is the Client, also equipped with two network cards, NETB1 and
>>    NETB2, which are also bonded and configured in BOND_MODE_BROADCAST mode.
>>
>> If the client attempts a TCP connection to the server, it might encounter
>> a failure. Capturing packets from the server side reveals:
>>
>> 10.10.10.10.45182 > localhost: Flags [S], seq 320236027,
>> 10.10.10.10.45182 > localhost: Flags [S], seq 320236027,
>> localhost > 10.10.10.10.45182: Flags [S.], seq 2967855116,
>> localhost > 10.10.10.10.45182: Flags [S.], seq 2967855123, <==
>> 10.10.10.10.45182 > localhost: Flags [.], ack 4294967290,
>> 10.10.10.10.45182 > localhost: Flags [.], ack 4294967290,
>> localhost > 10.10.10.10.45182: Flags [R], seq 2967855117, <==
>> localhost > 10.10.10.10.45182: Flags [R], seq 2967855117,
>>
>> Two SYNACKs with different seq numbers are sent by localhost,
>> resulting in an anomaly.
>>
>> ========================================================================
>>
>> The attempted solution is as follows:
>> Add a return value to inet_csk_reqsk_queue_hash_add() to confirm if the
>> ehash insertion is successful (Up to now, the reason for unsuccessful
>> insertion is that a reqsk for the same connection has already been
>> inserted). If the insertion fails, release the reqsk.
>>
>> Due to the refcnt, Kuniyuki suggests also adding a return value check
>> for the DCCP module; if ehash insertion fails, indicating a successful
>> insertion of the same connection, simply release the reqsk as well.
>>
>> Simultaneously, In the reqsk_queue_hash_req(), the start of the
>> req->rsk_timer is adjusted to be after successful insertion.
>>
>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Just after applying the patch I wondered if the issue addressed here
> should be observable only after commit e994b2f0fb92 ("tcp: do not lock
> listener to process SYN packets")?
>
> In practice it should not matter as the latter commit it's older than
> the currently older LST, but I'm wondering if I read the things
> correctly?
>
> Thanks!
>
> Paolo
>
Hi, Paolo, I conducted some experiments on your concern by reverting e994b2f0fb92 on version 4.19 to observe how TCP handles this race condition.

Here are the observations:
where SYN-A is processed on CPUA and SYN-B is processed on CPUB

CPUA & CPUB

In tcp_v4_rcv(), both SYN-A and SYN-B obtained the same sk from __inet_lookup_listener(), with the sk state being TCP_LISTEN.

     CPUA

     SYN-A acquired sk_lock and was processed in tcp_v4_do_rcv(), where it created reqsk-A while in TCP_LISTEN state and sent a SYNACK packet.

     

                 CPUB

                 After SYN-A was processed and sk_lock was released, SYN-B was processed. Since it was the same sk still in TCP_LISTEN state, it created reqsk-B and sent a SYNACK packet with a different seq number.

The issue remains reproducible.

BRs!