Re: [PATCH v2] tcp: fastopen: check rsk_drop_req() in tcp_fastopen_create_child()

[Jiayuan Chen <jiayuan.chen@linux.dev>]

On 9/11/26 12:12 AM, Matthieu Baerts wrote:
> Hi Jiayuan,
>
> On 10/09/2026 14:15, Jiayuan Chen wrote:
>> On 9/10/26 6:13 PM, Matthieu Baerts wrote:
>>> Hi Paolo,
>>>
>>> Thank you for having checked!
>>>
>>> On 10/09/2026 10:12, Paolo Abeni wrote:
>>>> On 9/3/26 11:40 AM, Yilin Zhang wrote:
>>>>> subflow_syn_recv_sock() sets drop_req when an MP_JOIN SYN takes the
>>>>> fatal
>>>>> fallback and destroys the cloned child. tcp_fastopen_create_child()
>>>>> ignored the flag and could queue the destroyed child.
>>>>>
>>>>> With an MPTCP listener using server-side Fast Open, a valid-cookie
>>>>> MP_JOIN SYN could then expose the freed child through accept().
>>>>>
>>>>> Release the locked child and drop the request before tcp_conn_request()
>>>>> sends a SYN-ACK. Initialize drop_req when allocating the request so the
>>>>> check cannot observe stale state after request-socket reuse.
>>>>>
>>>>> Changes in v2:
>>>>> - unlock the child before dropping its reference
>>>>> - drop the request instead of sending a SYN-ACK after the MPTCP reset,
>>>>>     as suggested by Jiayuan Chen
>>>>>
>>>>> Fixes: 90bf45134d55 ("mptcp: add new sock flag to deal with join
>>>>> subflows")
>>>>> Reported-by: Kimi Security Team <bug-report@moonshot.ai>
>>>>> Suggested-by: Jiayuan Chen <jiayuan.chen@linux.dev>
>>>>> Signed-off-by: Yilin Zhang <yilinzhang@moonshot.ai>
>>>> This looks like the wrong fix. IIRC fastopen is not compatible with MPJ
>>>> - as the latter must accept data only after the 4way handshake
>>>> completion.
>>>>
>>>> @Mat(s): could you please double check the above ^^^ statement???
>>>>
>>>> If so mptcp should reject entirely MPJ + fastopen and no addtional code
>>>> required on the TCP side.
>>> I didn't check in details, but when I try with this packetdrill repro...
>>>
>>>    https://lore.kernel.org/20260903070649.3965366-1-yilinzhang@moonshot.ai
>>>
>>> ... it looks like the kernel replies with an MP_RST, but also a SYN+ACK,
>>> and a WARN:
>>>
>>>     refcount_t: underflow; use-after-free.
>>>     (...)
>>>     inet_csk_listen_stop (net/ipv4/inet_connection_sock.c:1533)
>>>     ? mptcp_subflow_queue_clean (net/mptcp/subflow.c:1925)
>>>     mptcp_check_listen_stop.part.0 (include/net/sock.h:1486
>>>                                     net/mptcp/protocol.c:3482)
>>>     __mptcp_close (net/mptcp/protocol.c:3534)
>>>     mptcp_close (net/mptcp/protocol.c:3572)
>>>     inet_release (net/ipv4/af_inet.c:425)
>>>
>>> With this v2, I don't see the SYN+ACK, only the RST.
>>>
>>>
>>> If I'm not mistaken, with TFO, syn_recv_sock will be called first
>>> (tcp_conn_request -> tcp_fastopen_create_child -> subflow_syn_recv_sock)
>>> then MPTCP will only check TFO when the SYN+ACK is being sent
>>> (tcp_conn_request -> subflow_v(46)_send_synack).
>>>
>>> In subflow_syn_recv_sock(), I don't think we have a strong indicator
>>
>> You inspired me. Maybe we can use such code:
>>
>>      if (subflow_req->mp_join &&
>>          (TCP_SKB_CB(skb)->tcp_flags & TCPHDR_SYN))
>>              return NULL;
> Good idea! Indeed, having a SYN here only happens with TFO.
>
> It would be good to add a comment there then.
>
>> We now skip RST and SYN+ACK also can be sent by tcp_conn_request().
> Yes, I guess returning NULL is not enough, a reset should probably be
> sent as well (prohibit), a MIB counter incremented (MPJ rejected?), and
> the request dropped.


Should we accept this subflow ?

It's just a SYN with MPJ + valid fastopen cookie, replying SYNACK 
and fallback to 3-way handshake may be easier.

(I'm not sure whether RFC define it or not.)


>
> Just to avoid a deadlock: @Yilin: will you send a v3 with this suggestion?


If we want to reject such SYN, we need modify tcp_conn_request() to 
avoid the SYNACK being sent

and explicit‌ly send RST before returning NULL.

>
> Cheers,
> Matt