Re: [PATCH mptcp-next v20 3/6] mptcp: fix TFO map_seq sign extension in fastopen

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] mptcp: inaccurate commit message on before64()/after64() and OoO queue
--

commit c0adf4769c9ae204c304e167fe42012e5a15d3c6
Author: Geliang Tang <geliang@kernel.org>

mptcp: fix TFO map_seq sign extension in fastopen

This commit casts skb->len to u64 before negating it when initializing the
TFO skb's dummy map_seq in mptcp_fastopen_subflow_synack_set_params(), so
that it is properly sign-extended in 64-bit arithmetic.

>     mptcp: fix TFO map_seq sign extension in fastopen
>     
>     In mptcp_fastopen_subflow_synack_set_params(), the TFO skb's map_seq is
>     set to -skb->len to represent data before the IASN.
>     
>     Since skb->len is u32, the negation -skb->len is computed in u32
>     arithmetic, producing a large positive value (e.g., -1420 becomes
>     4294965876). When assigned to map_seq (u64), this value is zero-extended
>     rather than sign-extended.
>     
>     This causes incorrect behavior in sequence number comparisons throughout
>     the code. Functions like after64() and before64() may misorder TFO skbs,
>     and the out-of-order queue handling may misclassify TFO data.
>     
>     Fix by casting to u64 before negation: -(u64)skb->len. This ensures
>     proper 64-bit two's complement representation and correct semantics for
>     all sequence number operations.
>     
>     Signed-off-by: Geliang Tang <tanggeliang@kylinos.cn>

[Severity: Medium]
Is the commit message description of the impact on after64(), before64(), and
the out-of-order queue accurate?

In mptcp_fastopen_subflow_synack_set_params() in net/mptcp/fastopen.c, the
TFO skb is queued directly to the tail of the in-order receive queue:

	MPTCP_SKB_CB(skb)->map_seq = -(u64)skb->len;
	MPTCP_SKB_CB(skb)->end_seq = 0;
	MPTCP_SKB_CB(skb)->offset = 0;
	...
	__skb_queue_tail(&sk->sk_receive_queue, skb);

Because it goes directly onto sk->sk_receive_queue, it never passes through
__mptcp_move_skb(), mptcp_data_queue_ofo(), or msk->out_of_order_queue, and
its dummy map_seq is not compared with before64() or after64(). When
subsequent skbs arrive in __mptcp_move_skb(), __mptcp_sync_rcv_sequence()
updates the TFO skb's dummy map_seq and end_seq using msk->tfo_iasn before
any comparison or coalescing takes place, and __mptcp_try_coalesce() checks
MPTCP_SKB_CB(to)->end_seq rather than map_seq.

Instead, the missing sign extension affects mptcp_inq_hint() in
net/mptcp/protocol.c before the 3rd ACK arrives (while msk->ack_seq is 0):

	skb = skb_peek(&sk->sk_receive_queue);
	if (skb) {
		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;

		if (hint_val >= INT_MAX)
			return INT_MAX;

		return (unsigned int)hint_val;
	}

Without the cast to u64, 0ULL - MPTCP_SKB_CB(skb)->map_seq underflows and
clamps to INT_MAX for SIOCINQ, TCP_CM_INQ, and mptcp_rcv_space_adjust(). It
is also needed by the subsequent commit 698eb6e05455 ("mptcp: remove CB
offset field"), which initializes msk->copied_seq from
MPTCP_SKB_CB(skb)->map_seq and relies on 64-bit two's complement wrapping
when adding msk->tfo_iasn.

Could the commit message be updated to describe the actual callers affected
by the unextended map_seq value?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793695518.git.tanggeliang@kylinos.cn?part=3