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

[Matthieu Baerts <matttbe@kernel.org>]

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.

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

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