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

[Matthieu Baerts <matttbe@kernel.org>]

Hi Jiayuan,

On 11/09/2026 04:25, Jiayuan Chen wrote:
> 
> 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.

Yes, better indeed.

But then I guess we should not send a TFO cookie in the SYNACK for this
case, right?

Setting foc->len = -1 for MPJ in subflow_prep_synack()?

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

I don't think the MPTCP one mentions this case.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.