Re: [PATCH mptcp v2] mptcp: restore full join state in syncookie MP_JOIN reconstruction
Paolo Abeni <[email protected]>
| Newsgroups | org.kernel.vger.netdev,dev.linux.lists.mptcp,org.kernel.vger.stable |
|---|---|
| Message-ID | <[email protected]> |
On 8/12/26 12:23 AM, Harshit Varu wrote:
> mptcp_token_join_cookie_init_state() rebuilds the request socket for a
> MP_JOIN 4th-ACK that was handled under SYN cookies, but it only restores
> remote_nonce, local_nonce, backup, join_id, token and msk from the saved
> cookie entry. local_id, request_bkup and thmac are never restored, even
> though the SYN path saves local_id and computes the other two.
>
> subflow_ulp_clone() then reads those three fields and copies them into the
> joined subflow context (local_id, request_bkup, thmac). Because the
> request-sock slab is SLAB_TYPESAFE_BY_RCU and not zeroed on allocation, the
> values are stale bytes of previously freed request sockets, which an
> off-path peer can influence by sending concurrent MP_JOIN SYNs. A corrupted
> local_id breaks id-based path-manager bookkeeping, and a corrupted
> request_bkup misclassifies the subflow in the packet scheduler's
> backup/active selection.
>
> Save and restore request_bkup and thmac as well, completing the state
> restore.
>
> Fixes: 9466a1ccebbe ("mptcp: enable JOIN requests even if cookies are in use")
> Cc: [email protected]
> Assisted-by: opencode:deepseek-v4-flash
> Signed-off-by: Harshit Varu <[email protected]>
> ---
> net/mptcp/syncookies.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/net/mptcp/syncookies.c b/net/mptcp/syncookies.c
> index 7f2252634..3c25ff627 100644
> --- a/net/mptcp/syncookies.c
> +++ b/net/mptcp/syncookies.c
> @@ -27,7 +27,9 @@ struct join_entry {
> u8 join_id;
> u8 local_id;
> u8 backup;
> + u8 request_bkup;
> u8 valid;
> + u64 thmac;
Is thmac used for passive flows after
mptcp_token_join_cookie_init_state()? I think it's not. If so just init
to 0 in mptcp_token_join_cookie_init_state (with a comment) and remove
remove the new field from here.
Same for request_bkup, AFAICS both are used in syn_ack only.
The bottom line is that we don't want to increase `struct join_entry`
size without good reasons.
/P