Re: [PATCH mptcp-next v19 3/5] mptcp: remove CB offset field

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] mptcp: stream corruption in mptcp_init_skb() on non-zero subflow offset
- [High] mptcp: unsynchronized copied_seq when MPTCP_SYNC_SEQ is deferred
- [Medium] mptcp: unlocked test_and_clear_bit() on cb_flags in mptcp_move_skbs()
- [Medium] mptcp: missing reset of tfo_iasn in mptcp_disconnect() and __mptcp_init_sock()
--

commit 373a78c8c2985557d0ce492ed496a374c5080029
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: remove CB offset field

This commit replaces the per-skb MPTCP_SKB_CB(skb)->offset field with an
msk-level copied_seq counter to track bytes consumed from the receive queue.
It also updates fastopen sequence synchronization, receive queue coalescing,
and mptcp_inq_hint() to compute offsets and hints from copied_seq.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index e855aaa59da29..3f967c4dc825f 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -168,7 +168,6 @@ static bool __mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
>  	int limit = READ_ONCE(sk->sk_rcvbuf);
>  
>  	if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq ||
> -	    MPTCP_SKB_CB(from)->offset ||
>  	    ((to->len + from->len) > (limit >> 3)) ||
>  	    !skb_try_coalesce(to, from, fragstolen, delta))
>  		return false;
> @@ -415,8 +414,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
>  	skb_set_owner_r(skb, sk);
>  }
>  
> -static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
> -			   int copy_len)
> +static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
>  {
>  	struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk);
>  	bool has_rxtstamp = TCP_SKB_CB(skb)->has_rxtstamp;
> @@ -425,9 +423,9 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset,
>  	 * mptcp_subflow_get_mapped_dsn() is based on the current tp->copied_seq
>  	 * value
>  	 */
> -	MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow);
> -	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + copy_len;
> -	MPTCP_SKB_CB(skb)->offset = offset;
> +	MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow) -
> +				     offset;
> +	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;
>  	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;

[Severity: High]
As reported by Sashiko on v18 [1], can subtracting offset from
mptcp_subflow_get_mapped_dsn(subflow) in mptcp_init_skb() while removing the
MPTCP_SKB_CB(from)->offset check in __mptcp_try_coalesce() corrupt the
MPTCP receive stream when offset > 0?

In __mptcp_move_skbs_from_subflow(), the skb at the head of
ssk->sk_receive_queue can have a non-zero offset (tp->copied_seq -
TCP_SKB_CB(skb)->seq > 0) while starting a new DSS mapping M2 at
tp->copied_seq with DSN mapped_dsn = mptcp_subflow_get_mapped_dsn(subflow).

For example, in subflow_check_data_avail() -> get_mapping_status(), a single
TCP skb can carry the tail of a previous DSS mapping M1 in bytes
0 .. offset - 1 and a new DSS mapping M2 in its SKB_EXT_MPTCP extension
starting at byte offset. When M1's remaining bytes were already received on
another subflow (before64(ack_seq, old_ack)), get_mapping_status() sees
!skb_is_fully_mapped(ssk, skb) and keeps SKB_EXT_MPTCP on the skb,
mptcp_subflow_discard_data() advances tp->copied_seq by offset without
eating the skb and resets subflow->map_valid = 0, and the next iteration of
subflow_check_data_avail() installs M2 starting at tp->copied_seq. A
partially overlapping TCP segment queued by tcp_data_queue() or
tcp_ofo_queue() at a mapping boundary can also produce offset > 0.

Because MPTCP stripes data across multiple subflows, the discarded prefix
bytes in skb->data[0 .. offset) do not belong to DSN range
[mapped_dsn - offset, mapped_dsn). Setting map_seq = mapped_dsn - offset and
end_seq = map_seq + skb->len appears to cause three issues:

1. In __mptcp_add_backlog(), __mptcp_move_skb(), mptcp_data_queue_ofo(), and
__mptcp_ofo_queue(), whenever mapped_dsn - offset equals to->end_seq,
__mptcp_try_coalesce() calls skb_try_coalesce(to, from, ...), which appends
all from->len bytes starting from byte 0 (including the discarded prefix
from->data[0 .. offset)) onto to.

2. When M2 is out of order at the MPTCP level (mapped_dsn > msk->ack_seq)
because [msk->ack_seq, mapped_dsn) is in flight on another subflow, and
mapped_dsn - offset <= msk->ack_seq < mapped_dsn, __mptcp_move_skb() sees
MPTCP_SKB_CB(skb)->map_seq <= msk->ack_seq and treats the out-of-order skb
as in-sequence or a partial packet starting at msk->ack_seq. It advances
msk->ack_seq and msk->bytes_received across the unreceived gap
[msk->ack_seq, mapped_dsn), queues or coalesces the skb into
sk->sk_receive_queue, delivers the stale bytes from skb->data[0 .. offset)
to user space in __mptcp_recvmsg_mskq() and mptcp_recv_skb(), and causes the
genuine data for [msk->ack_seq, mapped_dsn) arriving on the other subflow to
be dropped as MPTCP_MIB_DUPDATA.

3. When mapped_dsn - offset > msk->ack_seq, mptcp_data_queue_ofo() inserts
the skb into msk->out_of_order_queue at mapped_dsn - offset, where it can
coalesce with or replace legitimate out-of-order segments covering
[mapped_dsn - offset, mapped_dsn) and be dequeued early by
__mptcp_ofo_queue() once msk->ack_seq reaches mapped_dsn - offset.

[ ... ]

> @@ -2396,8 +2376,10 @@ static bool mptcp_move_skbs(struct sock *sk)
>  	bool enqueued = false;
>  	u32 moved = 0;
>  
> -	if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags))
> +	if (test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)) {
> +		msk->copied_seq += msk->tfo_iasn;
>  		__mptcp_sync_rcv_sequence(sk);
> +	}
>  
>  	mptcp_data_lock(sk);
>  	while (mptcp_can_spool_backlog(sk, &skbs)) {

[Severity: Medium]
As reported by Sashiko on v15 [2], is it safe to call
test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags) and
__mptcp_sync_rcv_sequence(sk) in mptcp_move_skbs() before acquiring
mptcp_data_lock(sk)?

All other flags in msk->cb_flags share the same unsigned long word and are
updated when sock_owned_by_user(sk) is true under mptcp_data_lock(sk) using
non-atomic __set_bit() or |=, such as in mptcp_propagate_state() right after
subflow_set_remote_key():

    mptcp_propagate_state()
        subflow_set_remote_key()
            set_bit(MPTCP_SYNC_SEQ, &msk->cb_flags);
        ...
        __set_bit(MPTCP_SYNC_STATE, &msk->cb_flags);

as well as in __mptcp_data_acked(), __mptcp_check_push(),
mptcp_retransmit_timer(), mptcp_subflow_process_delegated(),
mptcp_finish_join(), subflow_error_report(), and
mptcp_check_and_set_pending().

If a BH or timer handler on another CPU holds mptcp_data_lock(sk) and
performs a non-atomic read-modify-write on msk->cb_flags while
MPTCP_SYNC_SEQ is set, and mptcp_move_skbs() clears MPTCP_SYNC_SEQ without
holding mptcp_data_lock(sk) between that load and store, can the store
restore MPTCP_SYNC_SEQ to 1 and cause mptcp_release_cb() or a later
mptcp_move_skbs() call to add msk->tfo_iasn to msk->copied_seq a second
time?

> @@ -2418,22 +2400,17 @@ static bool mptcp_move_skbs(struct sock *sk)
>  static unsigned int mptcp_inq_hint(const struct sock *sk)
>  {
>  	const struct mptcp_sock *msk = mptcp_sk(sk);
> -	const struct sk_buff *skb;
> -
> -	skb = skb_peek(&sk->sk_receive_queue);
> -	if (skb) {
> -		u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
> +	u64 hint_val;
>  
> -		if (hint_val >= INT_MAX)
> -			return INT_MAX;
> +	hint_val = READ_ONCE(msk->ack_seq) - READ_ONCE(msk->copied_seq);
> +	if (hint_val >= INT_MAX)
> +		return INT_MAX;
>  
> -		return (unsigned int)hint_val;
> -	}
> -
> -	if (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN))
> +	if (!hint_val &&
> +	    (sk->sk_state == TCP_CLOSE || (sk->sk_shutdown & RCV_SHUTDOWN)))
>  		return 1;
>  
> -	return 0;
> +	return (unsigned int)hint_val;
>  }

[Severity: High]
As reported by Sashiko on v15 [2], can deferring the msk->copied_seq update
via MPTCP_SYNC_SEQ in subflow_set_remote_key() leave msk->copied_seq out of
sync with sk->sk_receive_queue and msk->ack_seq?

When subflow_set_remote_key() runs in BH context while sock_owned_by_user(sk)
is true, it immediately updates WRITE_ONCE(msk->ack_seq, subflow->iasn) and
msk->tfo_iasn = subflow->iasn under mptcp_data_lock(sk), while deferring
msk->copied_seq += subflow->iasn and __mptcp_sync_rcv_sequence(sk) via
MPTCP_SYNC_SEQ. This appears to leave two windows where sequence numbers are
mismatched:

1. In mptcp_move_skbs(), test_and_clear_bit(MPTCP_SYNC_SEQ, &msk->cb_flags)
is only checked once before acquiring mptcp_data_lock(sk) and outside the
while (mptcp_can_spool_backlog(sk, &skbs)) loop (which drops
mptcp_data_lock(sk) on each iteration). If the 3rd ACK carrying MP_CAPABLE
and data arrives in BH after that initial check, subflow_set_remote_key()
sets msk->tfo_iasn and MPTCP_SYNC_SEQ, and mptcp_data_ready() queues the new
skb onto msk->backlog_list.

mptcp_move_skbs() then splices msk->backlog_list and calls
__mptcp_move_skbs() -> __mptcp_move_skb(), which sees msk->rcvd_dummy_seq
and calls __mptcp_sync_rcv_sequence(sk) directly:

    __mptcp_move_skb()
        if (unlikely(msk->rcvd_dummy_seq))
            __mptcp_sync_rcv_sequence(sk);

This clears msk->rcvd_dummy_seq = false and rebases the TFO skb in
sk->sk_receive_queue to map_seq = msk->tfo_iasn - skb->len, without adding
msk->tfo_iasn to msk->copied_seq. When mptcp_move_skbs() returns to
mptcp_recv_skb() or mptcp_recvmsg(), sk->sk_receive_queue is in the
post-IASN sequence space while msk->copied_seq (and peek_seq) is still in
the pre-IASN sequence space. In mptcp_recv_skb(),
offset = msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq underflows by
msk->tfo_iasn, causing mptcp_eat_recv_skb() to drop unread skbs (or return a
corrupted offset if (u32)(-msk->tfo_iasn) < skb->len), and
__mptcp_recvmsg_mskq() skips unread skbs as already-peeked data.

2. While MPTCP_SYNC_SEQ is still pending, msk->ack_seq holds subflow->iasn
while msk->copied_seq is still pre-IASN. Calling mptcp_inq_hint(sk) before
MPTCP_SYNC_SEQ is processed (such as in __mptcp_recvmsg_mskq() ->
mptcp_rcv_space_adjust(), in mptcp_recvmsg() for TCP_CM_INQ when
msk->backlog_list is empty after a pure 3rd ACK without payload, in
__mptcp_read_sock() -> mptcp_rcv_space_adjust(), or in mptcp_ioctl(SIOCINQ)
if the remote key arrives after mptcp_move_skbs()) subtracts the pre-IASN
msk->copied_seq from the post-IASN msk->ack_seq, returning INT_MAX (or an
IASN-inflated value) and underflowing copied -= mptcp_inq_hint(sk) in
mptcp_rcv_space_adjust().

[ ... ]

> @@ -3699,6 +3689,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
>  	mptcp_pm_data_reset(msk);
>  	mptcp_ca_reset(sk);
>  	msk->bytes_consumed = 0;
> +	msk->copied_seq = 0;
>  	msk->bytes_acked = 0;
>  	msk->bytes_received = 0;
>  	msk->bytes_sent = 0;

[Severity: Medium]
Should mptcp_disconnect() (and __mptcp_init_sock()) also reset
msk->tfo_iasn = 0?

If an MPTCP socket connects and receives a remote key,
subflow_set_remote_key() sets msk->tfo_iasn = subflow->iasn. If the socket
is later disconnected via mptcp_disconnect() and reused as a listener, child
sockets cloned in mptcp_sk_clone_init() via sk_clone_lock() will inherit the
stale non-zero msk->tfo_iasn.

If a passive TFO connection on that listener queues a SYN payload in
mptcp_fastopen_subflow_synack_set_params() (setting map_seq and
msk->copied_seq to -(u64)skb->len and msk->rcvd_dummy_seq = true) and then
falls back to TCP on the 3rd ACK without receiving MP_CAPABLE,
subflow_set_remote_key() is never called on the child. When the next
fallback data segment arrives in __mptcp_move_skb(),
__mptcp_sync_rcv_sequence() will rebase the TFO skb's map_seq to
msk->tfo_iasn - skb->len using the stale msk->tfo_iasn while
msk->copied_seq remains -(u64)skb->len.

[1] https://lore.kernel.org/r/3cc892c20ed48f8f31f40e7a9ff7363d4b7b0806.1793349369.git.geliang@kernel.org
[2] https://lore.kernel.org/r/25f42bae1af43649a5c89ca10cfe0d962941c6f7.1792718041.git.tanggeliang@kylinos.cn

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