* [PATCH mptcp-next v17 1/4] mptcp: drop the mptcp_ooo_try_coalesce() helper
2026-10-30 2:30 [PATCH mptcp-next v17 0/4] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
@ 2026-10-30 2:30 ` Geliang Tang
2026-10-30 2:30 ` [PATCH mptcp-next v17 2/4] mptcp: remove CB offset field Geliang Tang
` (3 subsequent siblings)
4 siblings, 0 replies; 8+ messages in thread
From: Geliang Tang @ 2026-10-30 2:30 UTC (permalink / raw)
To: mptcp; +Cc: Paolo Abeni, Geliang Tang
From: Paolo Abeni <pabeni@redhat.com>
It's used to save an additional comparison for in-order skbs, but is
also a barrier to remove CB offset. Remove the helper, let
__mptcp_try_coalesce() always perform the sequence check and remove
duplicate checks from the callers.
Co-developed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/protocol.c | 21 ++++++---------------
1 file changed, 6 insertions(+), 15 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 02a4f479d9d8..528a72da646c 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -167,7 +167,8 @@ static bool __mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
{
int limit = READ_ONCE(sk->sk_rcvbuf);
- if (unlikely(MPTCP_SKB_CB(to)->cant_coalesce) ||
+ if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq ||
+ unlikely(MPTCP_SKB_CB(to)->cant_coalesce) ||
MPTCP_SKB_CB(from)->offset ||
((to->len + from->len) > (limit >> 3)) ||
!skb_try_coalesce(to, from, fragstolen, delta))
@@ -200,15 +201,6 @@ static bool mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
return true;
}
-static bool mptcp_ooo_try_coalesce(struct mptcp_sock *msk, struct sk_buff *to,
- struct sk_buff *from)
-{
- if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq)
- return false;
-
- return mptcp_try_coalesce((struct sock *)msk, to, from);
-}
-
/* "inspired" by tcp_rcvbuf_grow(), main difference:
* - mptcp does not maintain a msk-level window clamp
* - returns true when the receive buffer is actually updated
@@ -348,7 +340,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
/* with 2 subflows, adding at end of ooo queue is quite likely
* Use of ooo_last_skb avoids the O(Log(N)) rbtree lookup.
*/
- if (mptcp_ooo_try_coalesce(msk, msk->ooo_last_skb, skb)) {
+ if (mptcp_try_coalesce(sk, msk->ooo_last_skb, skb)) {
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOMERGE);
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOQUEUETAIL);
return;
@@ -394,7 +386,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
goto merge_right;
}
- } else if (mptcp_ooo_try_coalesce(msk, skb1, skb)) {
+ } else if (mptcp_try_coalesce(sk, skb1, skb)) {
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOMERGE);
return;
}
@@ -777,8 +769,7 @@ static void __mptcp_add_backlog(struct sock *sk,
if (!list_empty(&msk->backlog_list))
tail = list_last_entry(&msk->backlog_list, struct sk_buff, list);
- if (tail && MPTCP_SKB_CB(skb)->map_seq == MPTCP_SKB_CB(tail)->end_seq &&
- ssk == tail->sk &&
+ if (tail && ssk == tail->sk &&
__mptcp_try_coalesce(sk, tail, skb, &fragstolen, &delta)) {
skb->truesize -= delta;
kfree_skb_partial(skb, fragstolen);
@@ -902,7 +893,7 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
end_seq = MPTCP_SKB_CB(skb)->end_seq;
tail = skb_peek_tail(&sk->sk_receive_queue);
- if (!tail || !mptcp_ooo_try_coalesce(msk, tail, skb)) {
+ if (!tail || !mptcp_try_coalesce(sk, tail, skb)) {
int delta = msk->ack_seq - MPTCP_SKB_CB(skb)->map_seq;
/* skip overlapping data, if any */
--
2.55.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* [PATCH mptcp-next v17 2/4] mptcp: remove CB offset field
2026-10-30 2:30 [PATCH mptcp-next v17 0/4] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
2026-10-30 2:30 ` [PATCH mptcp-next v17 1/4] mptcp: drop the mptcp_ooo_try_coalesce() helper Geliang Tang
@ 2026-10-30 2:30 ` Geliang Tang
2026-10-30 2:56 ` sashiko-bot
2026-10-30 2:30 ` [PATCH mptcp-next v17 3/4] mptcp: sync mptcp skb cb layout with tcp one Geliang Tang
` (2 subsequent siblings)
4 siblings, 1 reply; 8+ messages in thread
From: Geliang Tang @ 2026-10-30 2:30 UTC (permalink / raw)
To: mptcp; +Cc: Paolo Abeni, Geliang Tang
From: Paolo Abeni <pabeni@redhat.com>
Instead, use a new msk-level copied_seq field to track the bytes already
consumed from the MPTCP receive queue, carrying the amount of bytes already
copied to user-space, alike what TCP is already doing.
The newly introduced copied_seq field (u64) is always accessed under the
msk socket lock. For TFO synchronization, subflow_set_remote_key() checks
if copied_seq still equals the initial TFO value (-(u64)skb->len) before
updating it. If user-space has already partially read the TFO data, the
progress is preserved. WRITE_ONCE/READ_ONCE ensure proper memory ordering
between BH context (3rd ACK processing) and user-space readers.
Since both map_seq and copied_seq are u64, the offset calculation
(copied_seq - map_seq) is straightforward. We check copied_seq >= map_seq
before the subtraction to prevent underflow, which is critical for
MSG_PEEK operations that traverse multiple skbs without advancing
copied_seq.
Prior to this patch, the TFO skb dummy mapping (map_seq = -(u32)skb->len,
zero-extended to u64) caused mptcp_inq_hint() to compute a wrapped
ack_seq - map_seq, so SIOCINQ and TCP_CM_INQ returned INT_MAX before the
MPC ACK. This patch fixes that behaviour through both the -(u64)skb->len
sign fix and the new ack_seq - copied_seq hint.
Overall this simplifies __mptcp_recvmsg_mskq(), __mptcp_move_skb(), and
__mptcp_read_sock() by removing per-skb offset tracking and will also
make possible the next patch.
Co-developed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/fastopen.c | 12 ++++++----
net/mptcp/protocol.c | 53 ++++++++++++++++++++++----------------------
net/mptcp/protocol.h | 2 +-
net/mptcp/subflow.c | 22 +++++++++++++++++-
4 files changed, 56 insertions(+), 33 deletions(-)
diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index f717750906ff..04b1038b5200 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -9,6 +9,7 @@
void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subflow,
struct request_sock *req)
{
+ struct mptcp_sock *msk;
struct sock *sk, *ssk;
struct sk_buff *skb;
struct tcp_sock *tp;
@@ -44,20 +45,23 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
subflow->ssn_offset += skb->len;
has_rxtstamp = TCP_SKB_CB(skb)->has_rxtstamp;
- /* Only the sequence delta is relevant */
- MPTCP_SKB_CB(skb)->map_seq = -skb->len;
+ /* The TFO segment data sits before the IASN; before receiving
+ * the remote key, IASN is assumed being 0.
+ */
+ MPTCP_SKB_CB(skb)->map_seq = -(u64)skb->len;
MPTCP_SKB_CB(skb)->end_seq = 0;
- MPTCP_SKB_CB(skb)->offset = 0;
MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
MPTCP_SKB_CB(skb)->cant_coalesce = 1;
mptcp_data_lock(sk);
DEBUG_NET_WARN_ON_ONCE(sock_owned_by_user_nocheck(sk));
+ msk = mptcp_sk(sk);
+ msk->copied_seq = MPTCP_SKB_CB(skb)->map_seq;
mptcp_borrow_fwdmem(sk, skb);
skb_set_owner_r(skb, sk);
__skb_queue_tail(&sk->sk_receive_queue, skb);
- mptcp_sk(sk)->bytes_received += skb->len;
+ msk->bytes_received += skb->len;
sk->sk_data_ready(sk);
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 528a72da646c..f3b1b6b282a2 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -169,7 +169,6 @@ static bool __mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq ||
unlikely(MPTCP_SKB_CB(to)->cant_coalesce) ||
- MPTCP_SKB_CB(from)->offset ||
((to->len + from->len) > (limit >> 3)) ||
!skb_try_coalesce(to, from, fragstolen, delta))
return false;
@@ -416,8 +415,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;
@@ -426,9 +424,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;
MPTCP_SKB_CB(skb)->cant_coalesce = 0;
@@ -472,10 +470,6 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
/* Partial packet */
if (after64(MPTCP_SKB_CB(skb)->end_seq, msk->ack_seq)) {
copy_len = MPTCP_SKB_CB(skb)->end_seq - msk->ack_seq;
- MPTCP_SKB_CB(skb)->offset += msk->ack_seq -
- MPTCP_SKB_CB(skb)->map_seq;
- MPTCP_SKB_CB(skb)->map_seq += msk->ack_seq -
- MPTCP_SKB_CB(skb)->map_seq;
goto insert;
}
@@ -833,7 +827,7 @@ static bool __mptcp_move_skbs_from_subflow(struct mptcp_sock *msk,
if (offset < skb->len) {
size_t len = skb->len - offset;
- mptcp_init_skb(ssk, skb, offset, len);
+ mptcp_init_skb(ssk, skb, offset);
if (own_msk) {
mptcp_subflow_lend_fwdmem(subflow, skb);
@@ -896,12 +890,9 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
if (!tail || !mptcp_try_coalesce(sk, tail, skb)) {
int delta = msk->ack_seq - MPTCP_SKB_CB(skb)->map_seq;
- /* skip overlapping data, if any */
pr_debug("uncoalesced seq=%llx ack seq=%llx delta=%d\n",
MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq,
delta);
- MPTCP_SKB_CB(skb)->offset += delta;
- MPTCP_SKB_CB(skb)->map_seq += delta;
__skb_queue_tail(&sk->sk_receive_queue, skb);
}
msk->bytes_received += end_seq - msk->ack_seq;
@@ -2195,11 +2186,22 @@ static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
int copied = 0;
skb_queue_walk_safe(&sk->sk_receive_queue, skb, tmp) {
- u32 delta, offset = MPTCP_SKB_CB(skb)->offset;
- u32 data_len = skb->len - offset;
+ u32 delta, offset = 0;
+ u32 data_len;
u32 count;
int err;
+ if (msk->copied_seq >= MPTCP_SKB_CB(skb)->map_seq)
+ offset = msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
+
+ /* Skip skbs that are entirely before copied_seq */
+ if (offset >= skb->len) {
+ *last = skb;
+ continue;
+ }
+
+ data_len = skb->len - offset;
+
if (flags & MSG_PEEK) {
/* skip already peeked skbs */
if (total_data_len + data_len <= copied_total) {
@@ -2233,11 +2235,9 @@ static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
if (!(flags & MSG_PEEK)) {
msk->bytes_consumed += count;
- if (count < data_len) {
- MPTCP_SKB_CB(skb)->offset += count;
- MPTCP_SKB_CB(skb)->map_seq += count;
+ msk->copied_seq += count;
+ if (count < data_len)
break;
- }
mptcp_eat_recv_skb(sk, skb);
} else {
@@ -2394,11 +2394,11 @@ static unsigned int mptcp_inq_hint(const struct sock *sk)
{
const struct mptcp_sock *msk = mptcp_sk(sk);
const struct sk_buff *skb;
+ u64 hint_val;
skb = skb_peek(&sk->sk_receive_queue);
if (skb) {
- u64 hint_val = READ_ONCE(msk->ack_seq) - MPTCP_SKB_CB(skb)->map_seq;
-
+ hint_val = READ_ONCE(msk->ack_seq) - READ_ONCE(msk->copied_seq);
if (hint_val >= INT_MAX)
return INT_MAX;
@@ -3674,6 +3674,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;
@@ -4601,7 +4602,7 @@ static struct sk_buff *mptcp_recv_skb(struct sock *sk, u32 *off)
mptcp_move_skbs(sk);
while ((skb = skb_peek(&sk->sk_receive_queue)) != NULL) {
- offset = MPTCP_SKB_CB(skb)->offset;
+ offset = msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
if (offset < skb->len) {
*off = offset;
return skb;
@@ -4643,11 +4644,9 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
copied += count;
msk->bytes_consumed += count;
- if (count < data_len) {
- MPTCP_SKB_CB(skb)->offset += count;
- MPTCP_SKB_CB(skb)->map_seq += count;
+ msk->copied_seq += count;
+ if (count < data_len)
break;
- }
mptcp_eat_recv_skb(sk, skb);
if (!desc->count)
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 3be60da12924..5c1fa117e5d5 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -130,7 +130,6 @@
struct mptcp_skb_cb {
u64 map_seq;
u64 end_seq;
- u32 offset;
u8 has_rxtstamp;
u8 cant_coalesce;
};
@@ -311,6 +310,7 @@ struct mptcp_sock {
u64 rcv_data_fin_seq;
u64 bytes_retrans;
u64 bytes_consumed;
+ u64 copied_seq;
int snd_burst;
int old_wspace;
u64 recovery_snd_nxt; /* in recovery mode accept up to this seq;
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 78e886b20258..6a66841ca24a 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -533,6 +533,26 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
WRITE_ONCE(msk->ack_seq, subflow->iasn);
WRITE_ONCE(msk->can_ack, true);
atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
+
+ if (subflow->is_mptfo) {
+ struct sock *sk = (struct sock *)msk;
+ struct sk_buff *skb;
+
+ skb = skb_peek(&sk->sk_receive_queue);
+ if (skb) {
+ u64 tfo_map_seq = subflow->iasn - skb->len;
+
+ WRITE_ONCE(MPTCP_SKB_CB(skb)->map_seq, tfo_map_seq);
+ WRITE_ONCE(MPTCP_SKB_CB(skb)->end_seq, subflow->iasn);
+
+ if (READ_ONCE(msk->copied_seq) != -(u64)skb->len)
+ return;
+
+ WRITE_ONCE(msk->copied_seq, tfo_map_seq);
+ return;
+ }
+ }
+ WRITE_ONCE(msk->copied_seq, subflow->iasn);
}
static void mptcp_propagate_state(struct sock *sk, struct sock *ssk,
@@ -1485,7 +1505,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
skb = skb_peek(&ssk->sk_receive_queue);
subflow->map_valid = 1;
subflow->map_data_len = skb->len;
- subflow->map_subflow_seq = tcp_sk(ssk)->copied_seq - subflow->ssn_offset;
+ subflow->map_subflow_seq = TCP_SKB_CB(skb)->seq - subflow->ssn_offset;
subflow->map_seq = __mptcp_expand_seq(subflow->map_seq,
subflow->iasn +
TCP_SKB_CB(skb)->seq -
--
2.55.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH mptcp-next v17 2/4] mptcp: remove CB offset field
2026-10-30 2:30 ` [PATCH mptcp-next v17 2/4] mptcp: remove CB offset field Geliang Tang
@ 2026-10-30 2:56 ` sashiko-bot
0 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-10-30 2:56 UTC (permalink / raw)
To: Geliang Tang, mptcp
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] mptcp: race and UAF in subflow_set_remote_key() with concurrent readers
- [High] mptcp: fix partial TFO read sequence tracking in subflow_set_remote_key()
- [High] mptcp: fix MSG_PEEK overlap and wraparound in __mptcp_recvmsg_mskq()
- [High] mptcp: mptcp_init_skb() corrupts stream when skb has positive subflow offset
--
commit a818e739b6c749d445de127d77fbde3c037d69af
Author: Paolo Abeni <pabeni@redhat.com>
mptcp: remove CB offset field
This patch replaces the per-skb offset field in mptcp_skb_cb with a
socket-level copied_seq field in struct mptcp_sock to track consumed bytes in
the MPTCP receive queue. It updates TFO sequence initialization, skb mapping
setup, receive queue consumption, and SIOCINQ hint calculation to compute
offsets relative to msk->copied_seq.
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 528a72da646cc..f3b1b6b282a2e 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -169,7 +169,6 @@ static bool __mptcp_try_coalesce(struct sock *sk, struct sk_buff *to,
>
> if (MPTCP_SKB_CB(from)->map_seq != MPTCP_SKB_CB(to)->end_seq ||
> unlikely(MPTCP_SKB_CB(to)->cant_coalesce) ||
> - MPTCP_SKB_CB(from)->offset ||
> ((to->len + from->len) > (limit >> 3)) ||
> !skb_try_coalesce(to, from, fragstolen, delta))
> return false;
> @@ -416,8 +415,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;
> @@ -426,9 +424,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;
[Severity: High]
Can subtracting offset from mptcp_subflow_get_mapped_dsn(subflow) in
mptcp_init_skb(), combined with removing the MPTCP_SKB_CB(from)->offset check
in __mptcp_try_coalesce(), corrupt the receive stream when an skb on
ssk->sk_receive_queue has a positive subflow offset?
An skb in __mptcp_move_skbs_from_subflow() has offset = tp->copied_seq -
TCP_SKB_CB(skb)->seq > 0 when its leading bytes were already consumed or
discarded (for example, via subflow_check_data_avail() ->
mptcp_subflow_discard_data() when an skb spans two DSS mappings and the first
mapping was already received on another subflow, or when a partially
overlapping TCP segment crosses a DSS boundary). In that situation, only
skb->data[offset..skb->len) belongs to the current mapping starting at
mapped_dsn = mptcp_subflow_get_mapped_dsn(subflow), while skb->data[0..offset)
does not belong to [mapped_dsn - offset, mapped_dsn).
By setting MPTCP_SKB_CB(skb)->map_seq = mapped_dsn - offset, when the new
mapping is out of order (mapped_dsn > msk->ack_seq):
1. If mapped_dsn - offset <= msk->ack_seq < mapped_dsn, __mptcp_move_skb()
treats the out-of-order skb as in-sequence or partially overlapping
msk->ack_seq, advances msk->ack_seq across the unreceived hole
[msk->ack_seq, mapped_dsn), and if mapped_dsn - offset == tail->end_seq,
__mptcp_try_coalesce() -> skb_try_coalesce() appends skb->data starting at
byte 0 onto the receive queue tail. The discarded prefix bytes in
skb->data[0..offset) are then delivered to user-space in place of the missing
DSN range, and the genuine data for [msk->ack_seq, mapped_dsn) is later
dropped as MPTCP_MIB_DUPDATA.
2. If mapped_dsn - offset > msk->ack_seq, mptcp_data_queue_ofo() inserts or
coalesces the skb in msk->out_of_order_queue under the fabricated range
[mapped_dsn - offset, end_seq), where it can overwrite or cause drops of
legitimate segments covering [mapped_dsn - offset, mapped_dsn) and later
deliver skb->data[0..offset) to user-space once msk->ack_seq reaches
mapped_dsn - offset.
[ ... ]
> @@ -472,10 +470,6 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
> /* Partial packet */
> if (after64(MPTCP_SKB_CB(skb)->end_seq, msk->ack_seq)) {
> copy_len = MPTCP_SKB_CB(skb)->end_seq - msk->ack_seq;
> - MPTCP_SKB_CB(skb)->offset += msk->ack_seq -
> - MPTCP_SKB_CB(skb)->map_seq;
> - MPTCP_SKB_CB(skb)->map_seq += msk->ack_seq -
> - MPTCP_SKB_CB(skb)->map_seq;
> goto insert;
> }
[ ... ]
> @@ -896,12 +890,9 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
> if (!tail || !mptcp_try_coalesce(sk, tail, skb)) {
> int delta = msk->ack_seq - MPTCP_SKB_CB(skb)->map_seq;
>
> - /* skip overlapping data, if any */
> pr_debug("uncoalesced seq=%llx ack seq=%llx delta=%d\n",
> MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq,
> delta);
> - MPTCP_SKB_CB(skb)->offset += delta;
> - MPTCP_SKB_CB(skb)->map_seq += delta;
> __skb_queue_tail(&sk->sk_receive_queue, skb);
> }
> msk->bytes_received += end_seq - msk->ack_seq;
> @@ -2195,11 +2186,22 @@ static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
> int copied = 0;
>
> skb_queue_walk_safe(&sk->sk_receive_queue, skb, tmp) {
> - u32 delta, offset = MPTCP_SKB_CB(skb)->offset;
> - u32 data_len = skb->len - offset;
> + u32 delta, offset = 0;
> + u32 data_len;
> u32 count;
> int err;
>
> + if (msk->copied_seq >= MPTCP_SKB_CB(skb)->map_seq)
> + offset = msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
> +
> + /* Skip skbs that are entirely before copied_seq */
> + if (offset >= skb->len) {
> + *last = skb;
> + continue;
> + }
> +
> + data_len = skb->len - offset;
[Severity: High]
Does __mptcp_recvmsg_mskq() return duplicate or skipped data during MSG_PEEK
when sk->sk_receive_queue contains partially overlapping skbs or when sequence
numbers wrap across zero?
Because __mptcp_move_skb() and __mptcp_ofo_queue() no longer advance
MPTCP_SKB_CB(skb)->map_seq to msk->ack_seq when queuing a partially
overlapping skb (and mptcp_init_skb() subtracts the subflow offset from
map_seq), a second skb (skb2) in sk->sk_receive_queue can have
MPTCP_SKB_CB(skb2)->map_seq < MPTCP_SKB_CB(skb1)->end_seq.
During a normal read, consuming skb1 advances msk->copied_seq to
MPTCP_SKB_CB(skb1)->end_seq, so the next iteration computes offset =
msk->copied_seq - MPTCP_SKB_CB(skb2)->map_seq and skips the overlapping prefix
of skb2. Under MSG_PEEK, however, msk->copied_seq is not advanced as skb1 is
peeked. When the loop moves to skb2, it still compares against the unadvanced
msk->copied_seq rather than the sequence position reached after skb1, leaving
offset at 0 (or smaller than skb1->end_seq - skb2->map_seq) and copying the
overlapping prefix of skb2 into the user buffer a second time.
In addition, comparing msk->copied_seq >= MPTCP_SKB_CB(skb)->map_seq with raw
unsigned u64 arithmetic instead of !before64(msk->copied_seq,
MPTCP_SKB_CB(skb)->map_seq) breaks when sequence numbers cross 0 (for example,
on a passive TFO socket that falls back before the MPC ACK, where the TFO skb
has map_seq = -(u64)skb1->len and msk->copied_seq = -(u64)skb1->len while the
next fallback skb has map_seq = 0, causing msk->copied_seq >= map_seq to
evaluate true and skip skb2 with a wrapped offset).
[ ... ]
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 78e886b202585..6a66841ca24ae 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -533,6 +533,26 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
> WRITE_ONCE(msk->ack_seq, subflow->iasn);
> WRITE_ONCE(msk->can_ack, true);
> atomic64_set(&msk->rcv_wnd_sent, subflow->iasn);
> +
> + if (subflow->is_mptfo) {
> + struct sock *sk = (struct sock *)msk;
> + struct sk_buff *skb;
> +
> + skb = skb_peek(&sk->sk_receive_queue);
> + if (skb) {
> + u64 tfo_map_seq = subflow->iasn - skb->len;
> +
> + WRITE_ONCE(MPTCP_SKB_CB(skb)->map_seq, tfo_map_seq);
> + WRITE_ONCE(MPTCP_SKB_CB(skb)->end_seq, subflow->iasn);
> +
> + if (READ_ONCE(msk->copied_seq) != -(u64)skb->len)
> + return;
> +
> + WRITE_ONCE(msk->copied_seq, tfo_map_seq);
> + return;
> + }
> + }
> + WRITE_ONCE(msk->copied_seq, subflow->iasn);
> }
[Severity: High]
What happens in subflow_set_remote_key() if user-space partially reads the TFO
skb before the 3rd ACK arrives?
In mptcp_fastopen_subflow_synack_set_params(), both MPTCP_SKB_CB(skb)->map_seq
and msk->copied_seq are initialized to -(u64)skb->len. If user-space accepts
the passive TFO socket and reads count bytes (0 < count < skb->len) via
__mptcp_recvmsg_mskq() or __mptcp_read_sock() before the peer's MP_CAPABLE ACK
is processed, msk->copied_seq is advanced to -(u64)skb->len + count while the
partially consumed skb remains at the head of sk->sk_receive_queue.
When the 3rd ACK arrives, subflow_set_remote_key() updates
MPTCP_SKB_CB(skb)->map_seq to tfo_map_seq (subflow->iasn - skb->len) and
MPTCP_SKB_CB(skb)->end_seq to subflow->iasn, but then sees
READ_ONCE(msk->copied_seq) != -(u64)skb->len and returns early without
shifting msk->copied_seq by subflow->iasn (to tfo_map_seq + count).
Doesn't leaving msk->copied_seq at -(u64)skb->len + count while map_seq and
msk->ack_seq are in the subflow->iasn sequence space corrupt subsequent offset
calculations in __mptcp_recvmsg_mskq(), cause mptcp_recv_skb() to eat and drop
unread skbs via mptcp_eat_recv_skb(), and cause mptcp_inq_hint() to return
INT_MAX?
[Severity: Critical]
Additionally, can subflow_set_remote_key() race with concurrent user-space
readers in __mptcp_recvmsg_mskq() and __mptcp_read_sock() -> mptcp_recv_skb()
and trigger a use-after-free on skb or clobber msk->copied_seq?
When the 3rd ACK arrives in BH context, check_fully_established() calls
__mptcp_subflow_fully_established() -> subflow_set_remote_key() while holding
only mptcp_data_lock(sk) (sk->sk_lock.slock), without checking
sock_owned_by_user(sk):
net/mptcp/options.c:check_fully_established() {
...
mptcp_data_lock((struct sock *)msk);
__mptcp_subflow_fully_established(msk, subflow, mp_opt);
mptcp_data_unlock((struct sock *)msk);
}
Meanwhile, __mptcp_recvmsg_mskq() and mptcp_recv_skb() / __mptcp_read_sock()
run under lock_sock(sk) without holding mptcp_data_lock(sk) while peeking
sk->sk_receive_queue, reading MPTCP_SKB_CB(skb)->map_seq and msk->copied_seq,
updating msk->copied_seq += count, and unlinking and freeing the skb in
mptcp_eat_recv_skb(sk, skb):
1. If mptcp_eat_recv_skb() unlinks and frees the TFO skb right after
subflow_set_remote_key() executes skb = skb_peek(&sk->sk_receive_queue),
subflow_set_remote_key() will read skb->len and write
MPTCP_SKB_CB(skb)->map_seq and MPTCP_SKB_CB(skb)->end_seq on a freed sk_buff.
2. Both __mptcp_recvmsg_mskq() and __mptcp_read_sock() update
msk->copied_seq += count before calling mptcp_eat_recv_skb(sk, skb). If a full
read of the TFO skb advances msk->copied_seq to 0 and subflow_set_remote_key()
runs before mptcp_eat_recv_skb() unlinks skb, skb_peek(&sk->sk_receive_queue)
still returns skb, and READ_ONCE(msk->copied_seq) != -(u64)skb->len sees 0 and
returns early without setting msk->copied_seq = subflow->iasn.
3. Because subflow_set_remote_key() writes MPTCP_SKB_CB(skb)->map_seq before
updating msk->copied_seq, a concurrent reader in __mptcp_recvmsg_mskq() or
mptcp_recv_skb() can observe the updated map_seq (subflow->iasn - skb->len)
together with the old msk->copied_seq (-(u64)skb->len), or race its non-atomic
msk->copied_seq += count update against WRITE_ONCE(msk->copied_seq, ...).
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793326853.git.tanggeliang@kylinos.cn?part=2
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH mptcp-next v17 3/4] mptcp: sync mptcp skb cb layout with tcp one
2026-10-30 2:30 [PATCH mptcp-next v17 0/4] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
2026-10-30 2:30 ` [PATCH mptcp-next v17 1/4] mptcp: drop the mptcp_ooo_try_coalesce() helper Geliang Tang
2026-10-30 2:30 ` [PATCH mptcp-next v17 2/4] mptcp: remove CB offset field Geliang Tang
@ 2026-10-30 2:30 ` Geliang Tang
2026-10-30 2:49 ` sashiko-bot
2026-10-30 2:30 ` [PATCH mptcp-next v17 4/4] mptcp: defer read_sock cleanup to mptcp_worker Geliang Tang
2026-10-30 4:24 ` [PATCH mptcp-next v17 0/4] Reduce the differences between TCP and MPTCP for TLS usage MPTCP CI
4 siblings, 1 reply; 8+ messages in thread
From: Geliang Tang @ 2026-10-30 2:30 UTC (permalink / raw)
To: mptcp; +Cc: Paolo Abeni, Geliang Tang
From: Paolo Abeni <pabeni@redhat.com>
The MPTCP protocol uses a significantly different CB layout WRT TCP, as it
includes different information and use 64 bits for the sequence numbers.
As the msk-level rcvbuf buffer size is limited by the core socket code the
INT_MAX; after validating the incoming skb vs the current receive window,
we can safely use 32 bits for MPTCP-level sequence number. This allow
updating the MPTCP CB layout so that fields with a corresponding TCP-level
data use the same area inside the CB itself.
The 32-bit sequence numbers are safe because all SKBs in the receive queue
and OFO queue have been validated against the receive window (max INT_MAX),
so their sequence numbers are guaranteed to be within 2^31 of the current
ack_seq or copied_seq. This ensures that 32-bit arithmetic (comparisons
and subtractions) produces correct results even when 64-bit values cross
the 2^32 boundary, as the relative distance remains small.
Add build time check to ensure the latter invariant.
Co-developed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/fastopen.c | 6 ++-
net/mptcp/protocol.c | 90 +++++++++++++++++++++++++++-----------------
net/mptcp/protocol.h | 7 +++-
net/mptcp/subflow.c | 3 +-
4 files changed, 66 insertions(+), 40 deletions(-)
diff --git a/net/mptcp/fastopen.c b/net/mptcp/fastopen.c
index 04b1038b5200..08f0402324c2 100644
--- a/net/mptcp/fastopen.c
+++ b/net/mptcp/fastopen.c
@@ -48,8 +48,10 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
/* The TFO segment data sits before the IASN; before receiving
* the remote key, IASN is assumed being 0.
*/
- MPTCP_SKB_CB(skb)->map_seq = -(u64)skb->len;
+ MPTCP_SKB_CB(skb)->map_seq64 = -(u64)skb->len;
+ MPTCP_SKB_CB(skb)->map_seq = MPTCP_SKB_CB(skb)->map_seq64;
MPTCP_SKB_CB(skb)->end_seq = 0;
+ MPTCP_SKB_CB(skb)->flags = 0;
MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
MPTCP_SKB_CB(skb)->cant_coalesce = 1;
@@ -57,7 +59,7 @@ void mptcp_fastopen_subflow_synack_set_params(struct mptcp_subflow_context *subf
DEBUG_NET_WARN_ON_ONCE(sock_owned_by_user_nocheck(sk));
msk = mptcp_sk(sk);
- msk->copied_seq = MPTCP_SKB_CB(skb)->map_seq;
+ msk->copied_seq = MPTCP_SKB_CB(skb)->map_seq64;
mptcp_borrow_fwdmem(sk, skb);
skb_set_owner_r(skb, sk);
__skb_queue_tail(&sk->sk_receive_queue, skb);
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index f3b1b6b282a2..8447efa9dee6 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -168,12 +168,14 @@ 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)->map_seq64 != MPTCP_SKB_CB(to)->map_seq64 +
+ to->len ||
unlikely(MPTCP_SKB_CB(to)->cant_coalesce) ||
((to->len + from->len) > (limit >> 3)) ||
!skb_try_coalesce(to, from, fragstolen, delta))
return false;
- pr_debug("colesced seq %llx into %llx new len %d new end seq %llx\n",
+ pr_debug("colesced seq %x into %x new len %d new end seq %x\n",
MPTCP_SKB_CB(from)->map_seq, MPTCP_SKB_CB(to)->map_seq,
to->len, MPTCP_SKB_CB(from)->end_seq);
MPTCP_SKB_CB(to)->end_seq = MPTCP_SKB_CB(from)->end_seq;
@@ -255,8 +257,8 @@ static void mptcp_prune_ofo_queue(struct sock *sk,
struct sk_buff *skb = rb_to_skb(node);
/* Stop pruning if the incoming skb would land in OoO tail. */
- if (after64(MPTCP_SKB_CB(in_skb)->map_seq,
- MPTCP_SKB_CB(skb)->map_seq))
+ if (after(MPTCP_SKB_CB(in_skb)->map_seq,
+ MPTCP_SKB_CB(skb)->map_seq))
break;
pruned = true;
@@ -302,16 +304,21 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
{
struct sock *sk = (struct sock *)msk;
struct rb_node **p, *parent;
- u64 seq, end_seq, max_seq;
+ u64 end_seq, max_seq;
struct sk_buff *skb1;
+ u32 seq;
seq = MPTCP_SKB_CB(skb)->map_seq;
- end_seq = MPTCP_SKB_CB(skb)->end_seq;
+ end_seq = MPTCP_SKB_CB(skb)->map_seq64 + skb->len;
max_seq = atomic64_read(&msk->rcv_wnd_sent);
- pr_debug("msk=%p seq=%llx limit=%llx empty=%d\n", msk, seq, max_seq,
+ pr_debug("msk=%p seq=%x limit=%llx empty=%d\n", msk, seq, max_seq,
RB_EMPTY_ROOT(&msk->out_of_order_queue));
- if (after64(end_seq, max_seq)) {
+ /* Use the full sequence space to perform the admission checks, to
+ * protect vs possible wrap-arounds.
+ */
+ if (after64(MPTCP_SKB_CB(skb)->map_seq64, max_seq) ||
+ after64(end_seq, max_seq)) {
/* out of window */
mptcp_drop(sk, skb);
pr_debug("oow by %lld, rcv_wnd_sent %llu\n",
@@ -346,7 +353,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
}
/* Can avoid an rbtree lookup if we are adding skb after ooo_last_skb */
- if (!before64(seq, MPTCP_SKB_CB(msk->ooo_last_skb)->end_seq)) {
+ if (!before(seq, MPTCP_SKB_CB(msk->ooo_last_skb)->end_seq)) {
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_OFOQUEUETAIL);
parent = &msk->ooo_last_skb->rbnode;
p = &parent->rb_right;
@@ -358,18 +365,18 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
while (*p) {
parent = *p;
skb1 = rb_to_skb(parent);
- if (before64(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
+ if (before(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
p = &parent->rb_left;
continue;
}
- if (before64(seq, MPTCP_SKB_CB(skb1)->end_seq)) {
- if (!after64(end_seq, MPTCP_SKB_CB(skb1)->end_seq)) {
+ if (before(seq, MPTCP_SKB_CB(skb1)->end_seq)) {
+ if (!after((u32)end_seq, MPTCP_SKB_CB(skb1)->end_seq)) {
/* All the bits are present. Drop. */
mptcp_drop(sk, skb);
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
return;
}
- if (after64(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
+ if (after(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
/* partial overlap:
* | skb |
* | skb1 |
@@ -400,7 +407,7 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
merge_right:
/* Remove other segments covered by skb. */
while ((skb1 = skb_rb_next(skb)) != NULL) {
- if (before64(end_seq, MPTCP_SKB_CB(skb1)->end_seq))
+ if (before((u32)end_seq, MPTCP_SKB_CB(skb1)->end_seq))
break;
rb_erase(&skb1->rbnode, &msk->out_of_order_queue);
mptcp_drop(sk, skb1);
@@ -422,11 +429,13 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
/* the skb map_seq accounts for the skb offset:
* mptcp_subflow_get_mapped_dsn() is based on the current tp->copied_seq
- * value
+ * value; note that end seq number is only available in 32bits format.
*/
- MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow) -
- offset;
+ MPTCP_SKB_CB(skb)->map_seq64 = mptcp_subflow_get_mapped_dsn(subflow) -
+ offset;
+ MPTCP_SKB_CB(skb)->map_seq = (u32)MPTCP_SKB_CB(skb)->map_seq64;
MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;
+ MPTCP_SKB_CB(skb)->flags = 0;
MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;
MPTCP_SKB_CB(skb)->cant_coalesce = 0;
@@ -438,13 +447,13 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
{
- u64 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
+ u32 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
struct mptcp_sock *msk = mptcp_sk(sk);
struct sk_buff *tail;
mptcp_borrow_fwdmem(sk, skb);
- if (MPTCP_SKB_CB(skb)->map_seq == msk->ack_seq) {
+ if (MPTCP_SKB_CB(skb)->map_seq64 == msk->ack_seq) {
/* in sequence */
insert:
if (!mptcp_try_rmem_schedule(sk, skb)) {
@@ -462,14 +471,14 @@ static bool __mptcp_move_skb(struct sock *sk, struct sk_buff *skb)
skb_set_owner_r(skb, sk);
__skb_queue_tail(&sk->sk_receive_queue, skb);
return true;
- } else if (after64(MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq)) {
+ } else if (after64(MPTCP_SKB_CB(skb)->map_seq64, msk->ack_seq)) {
mptcp_data_queue_ofo(msk, skb);
return false;
}
/* Partial packet */
- if (after64(MPTCP_SKB_CB(skb)->end_seq, msk->ack_seq)) {
- copy_len = MPTCP_SKB_CB(skb)->end_seq - msk->ack_seq;
+ if (after64(MPTCP_SKB_CB(skb)->map_seq64 + skb->len, msk->ack_seq)) {
+ copy_len = MPTCP_SKB_CB(skb)->end_seq - (u32)msk->ack_seq;
goto insert;
}
@@ -864,39 +873,37 @@ static bool __mptcp_ofo_queue(struct mptcp_sock *msk)
{
struct sock *sk = (struct sock *)msk;
struct sk_buff *skb, *tail;
+ u32 seq_delta, ack_seq;
bool moved = false;
struct rb_node *p;
- u64 end_seq;
p = rb_first(&msk->out_of_order_queue);
pr_debug("msk=%p empty=%d\n", msk, RB_EMPTY_ROOT(&msk->out_of_order_queue));
while (p) {
+ ack_seq = msk->ack_seq;
skb = rb_to_skb(p);
- if (after64(MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq))
+ if (after(MPTCP_SKB_CB(skb)->map_seq, ack_seq))
break;
p = rb_next(p);
rb_erase(&skb->rbnode, &msk->out_of_order_queue);
- if (unlikely(!after64(MPTCP_SKB_CB(skb)->end_seq,
- msk->ack_seq))) {
+ if (unlikely(!after(MPTCP_SKB_CB(skb)->end_seq, ack_seq))) {
mptcp_drop(sk, skb);
MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);
continue;
}
- end_seq = MPTCP_SKB_CB(skb)->end_seq;
+ seq_delta = MPTCP_SKB_CB(skb)->end_seq - ack_seq;
tail = skb_peek_tail(&sk->sk_receive_queue);
if (!tail || !mptcp_try_coalesce(sk, tail, skb)) {
- int delta = msk->ack_seq - MPTCP_SKB_CB(skb)->map_seq;
-
- pr_debug("uncoalesced seq=%llx ack seq=%llx delta=%d\n",
- MPTCP_SKB_CB(skb)->map_seq, msk->ack_seq,
- delta);
+ pr_debug("uncoalesced seq=%x ack seq=%x delta=%d\n",
+ MPTCP_SKB_CB(skb)->map_seq, ack_seq,
+ ack_seq - MPTCP_SKB_CB(skb)->map_seq);
__skb_queue_tail(&sk->sk_receive_queue, skb);
}
- msk->bytes_received += end_seq - msk->ack_seq;
- WRITE_ONCE(msk->ack_seq, end_seq);
+ msk->bytes_received += seq_delta;
+ WRITE_ONCE(msk->ack_seq, msk->ack_seq + seq_delta);
moved = true;
}
return moved;
@@ -2192,7 +2199,7 @@ static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
int err;
if (msk->copied_seq >= MPTCP_SKB_CB(skb)->map_seq)
- offset = msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
+ offset = (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
/* Skip skbs that are entirely before copied_seq */
if (offset >= skb->len) {
@@ -4602,7 +4609,8 @@ static struct sk_buff *mptcp_recv_skb(struct sock *sk, u32 *off)
mptcp_move_skbs(sk);
while ((skb = skb_peek(&sk->sk_receive_queue)) != NULL) {
- offset = msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
+ offset = (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
+
if (offset < skb->len) {
*off = offset;
return skb;
@@ -4852,11 +4860,23 @@ static int mptcp_napi_poll(struct napi_struct *napi, int budget)
return work_done;
}
+#define CHK_CB_FIELD(mptcp_field, tcp_field) \
+ ({ \
+ BUILD_BUG_ON(offsetof(struct mptcp_skb_cb, mptcp_field) != \
+ offsetof(struct tcp_skb_cb, tcp_field)); \
+ BUILD_BUG_ON(offsetofend(struct mptcp_skb_cb, mptcp_field) != \
+ offsetofend(struct tcp_skb_cb, tcp_field)); \
+ })
+
void __init mptcp_proto_init(void)
{
struct mptcp_delegated_action *delegated;
int cpu;
+ CHK_CB_FIELD(map_seq, seq);
+ CHK_CB_FIELD(end_seq, end_seq);
+ CHK_CB_FIELD(flags, tcp_flags);
+
mptcp_prot.h.hashinfo = tcp_prot.h.hashinfo;
if (percpu_counter_init(&mptcp_sockets_allocated, 0, GFP_KERNEL))
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 5c1fa117e5d5..2ab8342e198d 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -128,10 +128,13 @@
#define MPTCP_SYNC_SNDBUF 7
struct mptcp_skb_cb {
- u64 map_seq;
- u64 end_seq;
+ u32 map_seq;
+ u32 end_seq;
+ u32 unused;
+ u16 flags;
u8 has_rxtstamp;
u8 cant_coalesce;
+ u64 map_seq64;
};
#define MPTCP_SKB_CB(__skb) ((struct mptcp_skb_cb *)&((__skb)->cb[0]))
diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 6a66841ca24a..5a8d837e55d0 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -542,7 +542,8 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
if (skb) {
u64 tfo_map_seq = subflow->iasn - skb->len;
- WRITE_ONCE(MPTCP_SKB_CB(skb)->map_seq, tfo_map_seq);
+ WRITE_ONCE(MPTCP_SKB_CB(skb)->map_seq, (u32)tfo_map_seq);
+ WRITE_ONCE(MPTCP_SKB_CB(skb)->map_seq64, tfo_map_seq);
WRITE_ONCE(MPTCP_SKB_CB(skb)->end_seq, subflow->iasn);
if (READ_ONCE(msk->copied_seq) != -(u64)skb->len)
--
2.55.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH mptcp-next v17 3/4] mptcp: sync mptcp skb cb layout with tcp one
2026-10-30 2:30 ` [PATCH mptcp-next v17 3/4] mptcp: sync mptcp skb cb layout with tcp one Geliang Tang
@ 2026-10-30 2:49 ` sashiko-bot
0 siblings, 0 replies; 8+ messages in thread
From: sashiko-bot @ 2026-10-30 2:49 UTC (permalink / raw)
To: Geliang Tang, mptcp
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] mptcp: TFO skb UAF race and un-rebased copied_seq in subflow_set_remote_key()
- [Critical] mptcp: subtracting offset from map_seq64 in mptcp_init_skb() corrupts OFO data
- [High] mptcp: 64-bit vs 32-bit seq check in __mptcp_recvmsg_mskq() breaks MSG_PEEK
--
commit 62db3c701d55a6fd52b9c1e97a8f06064e99fb14
Author: Paolo Abeni <pabeni@redhat.com>
mptcp: sync mptcp skb cb layout with tcp one
This commit aligns the MPTCP SKB control buffer layout with TCP by narrowing
MPTCP_SKB_CB(skb)->map_seq and end_seq to 32-bit fields while keeping the
full 64-bit sequence number in map_seq64. It updates the receive and
out-of-order queue paths to use 32-bit sequence comparisons after validating
incoming packets against the receive window.
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index f3b1b6b282a2e..8447efa9dee6e 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -422,11 +429,13 @@ static void mptcp_init_skb(struct sock *ssk, struct sk_buff *skb, int offset)
>
> /* the skb map_seq accounts for the skb offset:
> * mptcp_subflow_get_mapped_dsn() is based on the current tp->copied_seq
> - * value
> + * value; note that end seq number is only available in 32bits format.
> */
> - MPTCP_SKB_CB(skb)->map_seq = mptcp_subflow_get_mapped_dsn(subflow) -
> - offset;
> + MPTCP_SKB_CB(skb)->map_seq64 = mptcp_subflow_get_mapped_dsn(subflow) -
> + offset;
> + MPTCP_SKB_CB(skb)->map_seq = (u32)MPTCP_SKB_CB(skb)->map_seq64;
> MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + skb->len;
[Severity: Critical]
Can subtracting offset from map_seq64 in mptcp_init_skb() corrupt stream data
when the subflow SKB has a non-zero offset and carries an out-of-order MPTCP
Data Sequence Mapping?
In __mptcp_move_skbs_from_subflow(), offset = tp->copied_seq -
TCP_SKB_CB(skb)->seq represents already-consumed or discarded subflow bytes at
the front of skb, while mptcp_subflow_get_mapped_dsn(subflow) is the DSN for
the byte starting at offset.
When the mapping is in sequence (mapped_dsn == msk->ack_seq ==
msk->copied_seq), subtracting offset from map_seq64 works because
__mptcp_recvmsg_mskq() later computes offset = (u32)msk->copied_seq -
MPTCP_SKB_CB(skb)->map_seq and skips the first offset bytes.
However, if offset > 0 and mapped_dsn > msk->ack_seq (out-of-order MPTCP
data), subtracting offset shifts map_seq64 and map_seq backwards into the
unreceived sequence range [mapped_dsn - offset, mapped_dsn):
1. In __mptcp_move_skb(), if mapped_dsn - offset <= msk->ack_seq < mapped_dsn,
the packet takes the in-sequence or partial-packet branch instead of
mptcp_data_queue_ofo(), advancing msk->ack_seq and exposing the stale
subflow prefix [0, offset) to user-space.
2. If mapped_dsn - offset > msk->ack_seq, mptcp_data_queue_ofo() inserts skb
into out_of_order_queue claiming coverage of [mapped_dsn - offset,
mapped_dsn + skb->len - offset), which causes legitimate segments for
[mapped_dsn - offset, mapped_dsn) to be dropped as duplicate data and
causes __mptcp_ofo_queue() to dequeue skb as soon as msk->ack_seq reaches
mapped_dsn - offset.
[ ... ]
> @@ -2192,7 +2199,7 @@ static int __mptcp_recvmsg_mskq(struct sock *sk, struct msghdr *msg,
> int err;
>
> if (msk->copied_seq >= MPTCP_SKB_CB(skb)->map_seq)
> - offset = msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
> + offset = (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
>
> /* Skip skbs that are entirely before copied_seq */
> if (offset >= skb->len) {
[Severity: High]
Does comparing the 64-bit msk->copied_seq against the 32-bit
MPTCP_SKB_CB(skb)->map_seq in __mptcp_recvmsg_mskq() break MSG_PEEK across
multiple SKBs and across 32-bit sequence wrap-around?
With struct mptcp_skb_cb narrowing map_seq to u32 while msk->copied_seq
remains u64, the condition:
if (msk->copied_seq >= MPTCP_SKB_CB(skb)->map_seq)
offset = (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
zero-extends MPTCP_SKB_CB(skb)->map_seq to u64. Whenever the upper 32 bits of
msk->copied_seq are non-zero (which is typical since subflow->iasn is derived
from a 64-bit SHA-256 hash, or during TFO when msk->copied_seq is initialized
to -(u64)skb->len), msk->copied_seq >= MPTCP_SKB_CB(skb)->map_seq is
unconditionally true.
When recvmsg() is called with MSG_PEEK and sk->sk_receive_queue contains two
or more SKBs, msk->copied_seq is not updated after peeking the first SKB. On
the second SKB (where map_seq64 > msk->copied_seq), the condition still
evaluates to true, so offset = (u32)msk->copied_seq -
MPTCP_SKB_CB(skb)->map_seq underflows in 32-bit unsigned arithmetic to a huge
value (>= 2^31 > skb->len). That hits:
if (offset >= skb->len) {
*last = skb;
continue;
}
skipping all subsequent SKBs in sk->sk_receive_queue (and hanging in
sk_wait_data() when MSG_PEEK | MSG_WAITALL or SO_RCVLOWAT spans multiple
SKBs).
Conversely, if the upper 32 bits of msk->copied_seq are zero and an SKB
straddles a 32-bit wrap boundary (for example, map_seq = 0xfffffff0 and
copied_seq wraps to 0x10 after a partial read), 0x10ULL >= 0xfffffff0ULL
evaluates to false, leaving offset = 0 and re-reading already-consumed bytes.
Should this check compare against MPTCP_SKB_CB(skb)->map_seq64 (or use
!before((u32)msk->copied_seq, MPTCP_SKB_CB(skb)->map_seq))?
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 6a66841ca24ae..5a8d837e55d04 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -542,7 +542,8 @@ static void subflow_set_remote_key(struct mptcp_sock *msk,
> if (skb) {
> u64 tfo_map_seq = subflow->iasn - skb->len;
>
> - WRITE_ONCE(MPTCP_SKB_CB(skb)->map_seq, tfo_map_seq);
> + WRITE_ONCE(MPTCP_SKB_CB(skb)->map_seq, (u32)tfo_map_seq);
> + WRITE_ONCE(MPTCP_SKB_CB(skb)->map_seq64, tfo_map_seq);
> WRITE_ONCE(MPTCP_SKB_CB(skb)->end_seq, subflow->iasn);
>
> if (READ_ONCE(msk->copied_seq) != -(u64)skb->len)
[Severity: Critical]
What happens in subflow_set_remote_key() on a passive TFO socket if user-space
partially or concurrently reads the TFO SKB before the 3rd ACK arrives?
In mptcp_fastopen_subflow_synack_set_params(), the TFO SKB is queued onto
sk->sk_receive_queue with map_seq64 = -(u64)skb->len and msk->copied_seq =
-(u64)skb->len. There appear to be two issues when the 3rd ACK arrives in
check_fully_established() -> __mptcp_subflow_fully_established() ->
subflow_set_remote_key():
1. If user-space accepts the socket and partially reads N bytes (0 < N <
skb->len) before the 3rd ACK arrives, msk->copied_seq advances to
-(u64)skb->len + N while the SKB remains at the head of
sk->sk_receive_queue. When subflow_set_remote_key() runs:
if (READ_ONCE(msk->copied_seq) != -(u64)skb->len)
return;
it updates the SKB's map_seq, map_seq64, and end_seq to the subflow->iasn
sequence space, but returns early without rebasing msk->copied_seq by
+subflow->iasn (to tfo_map_seq + N). That leaves msk->copied_seq in the
initial 0-based sequence space while map_seq and msk->ack_seq are in the
subflow->iasn space, corrupting offset = (u32)msk->copied_seq -
MPTCP_SKB_CB(skb)->map_seq on subsequent reads.
2. In BH context, check_fully_established() only acquires
mptcp_data_lock(msk):
mptcp_data_lock((struct sock *)msk);
__mptcp_subflow_fully_established(msk, subflow, mp_opt);
mptcp_data_unlock((struct sock *)msk);
Meanwhile, a concurrent reader in __mptcp_recvmsg_mskq() or
mptcp_recv_skb() holds lock_sock(sk) (with sk_lock.slock released) and
calls mptcp_eat_recv_skb(), which unlinks and frees the SKB from
sk->sk_receive_queue without holding mptcp_data_lock(sk):
__skb_unlink(skb, &sk->sk_receive_queue);
skb_attempt_defer_free(skb);
Can subflow_set_remote_key() race with mptcp_eat_recv_skb() and trigger a
use-after-free when peeking sk->sk_receive_queue and writing to
MPTCP_SKB_CB(skb)?
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1793326853.git.tanggeliang@kylinos.cn?part=3
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH mptcp-next v17 4/4] mptcp: defer read_sock cleanup to mptcp_worker
2026-10-30 2:30 [PATCH mptcp-next v17 0/4] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
` (2 preceding siblings ...)
2026-10-30 2:30 ` [PATCH mptcp-next v17 3/4] mptcp: sync mptcp skb cb layout with tcp one Geliang Tang
@ 2026-10-30 2:30 ` Geliang Tang
2026-10-30 4:24 ` [PATCH mptcp-next v17 0/4] Reduce the differences between TCP and MPTCP for TLS usage MPTCP CI
4 siblings, 0 replies; 8+ messages in thread
From: Geliang Tang @ 2026-10-30 2:30 UTC (permalink / raw)
To: mptcp; +Cc: Paolo Abeni, Geliang Tang
From: Paolo Abeni <pabeni@redhat.com>
When MPTCP carries TLS, __mptcp_read_sock() is reached from BH context
(via tls_strp_read_copyin() -> sk->sk_read_sock()), where calling
mptcp_rcv_space_adjust() and mptcp_cleanup_rbuf() is not safe as both
expect process context.
Move both calls to mptcp_worker(): __mptcp_read_sock() accumulates the
consumed bytes in msk->read_copied and signals the worker through a new
MPTCP_WORK_READ_COMPLETE bit. The worker drains the accumulator under
mptcp_data_lock() and runs rcvbuf autotuning and RBU update in process
context.
Also remove the backlog flush from mptcp_recv_skb() (covered by
mptcp_release_cb()), drop the msk_owned_by_me() assertion (BH caller
does not own the msk lock), and drop the redundant mptcp_recv_skb()
call after the main loop.
Initialize msk->read_copied in __mptcp_init_sock() and reset it in
mptcp_disconnect().
Co-developed-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Geliang Tang <geliang@kernel.org>
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
net/mptcp/protocol.c | 37 ++++++++++++++++++++++++++++---------
net/mptcp/protocol.h | 2 ++
2 files changed, 30 insertions(+), 9 deletions(-)
diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
index 8447efa9dee6..8139f457cbb0 100644
--- a/net/mptcp/protocol.c
+++ b/net/mptcp/protocol.c
@@ -3125,6 +3125,21 @@ static void mptcp_backlog_purge(struct sock *sk)
sk_mem_reclaim(sk);
}
+static void mptcp_read_complete(struct sock *sk)
+{
+ struct mptcp_sock *msk = mptcp_sk(sk);
+ int read_copied;
+
+ mptcp_data_lock(sk);
+ read_copied = msk->read_copied;
+ msk->read_copied = 0;
+ mptcp_data_unlock(sk);
+
+ mptcp_rcv_space_adjust(msk, read_copied);
+ if (read_copied > 0)
+ mptcp_cleanup_rbuf(msk, read_copied);
+}
+
static void mptcp_do_fastclose(struct sock *sk)
{
struct mptcp_subflow_context *subflow, *tmp;
@@ -3200,6 +3215,9 @@ static void mptcp_worker(struct work_struct *work)
if (test_and_clear_bit(MPTCP_WORK_RTX, &msk->flags))
__mptcp_retrans(sk);
+ if (test_and_clear_bit(MPTCP_WORK_READ_COMPLETE, &msk->flags))
+ mptcp_read_complete(sk);
+
fail_tout = msk->first ? READ_ONCE(mptcp_subflow_ctx(msk->first)->fail_tout) : 0;
if (fail_tout && time_after(jiffies, fail_tout))
mptcp_mp_fail_no_response(msk);
@@ -3243,6 +3261,7 @@ static void __mptcp_init_sock(struct sock *sk)
msk->last_data_sent = tcp_jiffies32;
msk->last_data_recv = tcp_jiffies32;
msk->last_ack_recv = tcp_jiffies32;
+ msk->read_copied = 0;
mptcp_pm_data_init(msk);
spin_lock_init(&msk->fallback_lock);
@@ -3688,6 +3707,7 @@ static int mptcp_disconnect(struct sock *sk, int flags)
msk->bytes_retrans = 0;
msk->rcvspace_init = 0;
msk->fastclosing = 0;
+ msk->read_copied = 0;
mptcp_init_rtt_est(msk);
/* for fallback's sake */
@@ -4605,9 +4625,6 @@ static struct sk_buff *mptcp_recv_skb(struct sock *sk, u32 *off)
struct sk_buff *skb;
u32 offset;
- if (!list_empty(&msk->backlog_list))
- mptcp_move_skbs(sk);
-
while ((skb = skb_peek(&sk->sk_receive_queue)) != NULL) {
offset = (u32)msk->copied_seq - MPTCP_SKB_CB(skb)->map_seq;
@@ -4623,6 +4640,7 @@ static struct sk_buff *mptcp_recv_skb(struct sock *sk, u32 *off)
/*
* Note:
* - It is assumed that the socket was locked by the caller.
+ * - Can be invoked in BH scope.
*/
static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
sk_read_actor_t recv_actor, bool noack)
@@ -4632,8 +4650,6 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
int copied = 0;
u32 offset;
- msk_owned_by_me(msk);
-
if (sk->sk_state == TCP_LISTEN)
return -ENOTCONN;
while ((skb = mptcp_recv_skb(sk, &offset)) != NULL) {
@@ -4664,11 +4680,14 @@ static int __mptcp_read_sock(struct sock *sk, read_descriptor_t *desc,
if (noack)
goto out;
- mptcp_rcv_space_adjust(msk, copied);
-
+ /* The backlog flushing is only needed when some data is actually
+ * moved and will take place in mptcp_release_cb() via
+ * release_sock() in the worker.
+ */
if (copied > 0) {
- mptcp_recv_skb(sk, &offset);
- mptcp_cleanup_rbuf(msk, copied);
+ msk->read_copied += copied;
+ set_bit(MPTCP_WORK_READ_COMPLETE, &msk->flags);
+ mptcp_schedule_work(sk);
}
out:
return copied;
diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
index 2ab8342e198d..b0082e1c83f0 100644
--- a/net/mptcp/protocol.h
+++ b/net/mptcp/protocol.h
@@ -117,6 +117,7 @@
#define MPTCP_FALLBACK_DONE 2
#define MPTCP_WORK_CLOSE_SUBFLOW 3
#define MPTCP_RTX_ENABLED 4
+#define MPTCP_WORK_READ_COMPLETE 5
/* MPTCP socket release cb flags */
#define MPTCP_PUSH_PENDING 1
@@ -326,6 +327,7 @@ struct mptcp_sock {
u32 last_data_sent;
u32 last_data_recv;
u32 last_ack_recv;
+ int read_copied;
unsigned long timer_ival;
u32 token;
unsigned long flags;
--
2.55.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH mptcp-next v17 0/4] Reduce the differences between TCP and MPTCP for TLS usage
2026-10-30 2:30 [PATCH mptcp-next v17 0/4] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
` (3 preceding siblings ...)
2026-10-30 2:30 ` [PATCH mptcp-next v17 4/4] mptcp: defer read_sock cleanup to mptcp_worker Geliang Tang
@ 2026-10-30 4:24 ` MPTCP CI
4 siblings, 0 replies; 8+ messages in thread
From: MPTCP CI @ 2026-10-30 4:24 UTC (permalink / raw)
To: Paolo Abeni; +Cc: mptcp
Hi Paolo,
Thank you for your modifications, that's great!
Our CI did some validations and here is its report:
- KVM Validation: normal (except selftest_mptcp_join): Unstable: 1 failed test(s): packetdrill_syscalls ⚠️
- KVM Validation: normal (only selftest_mptcp_join): Success! ✅
- KVM Validation: debug (except selftest_mptcp_join): Success! ✅
- KVM Validation: debug (only selftest_mptcp_join): Success! ✅
- KVM Validation: btf-normal (only bpftest_all): Success! ✅
- KVM Validation: btf-debug (only bpftest_all): Success! ✅
- Perf: Unstable: 2 failed test(s): error:sched-bbr validation:sched-weak-secondary ⚠️
- Task: https://github.com/multipath-tcp/mptcp_net-next/actions/runs/40993318561
Initiator: Patchew Applier
Commits: https://github.com/multipath-tcp/mptcp_net-next/commits/1f6607073d0f
Patchwork: https://patchwork.kernel.org/project/mptcp/list/?series=1196753
If there are some issues, you can reproduce them using the same environment as
the one used by the CI thanks to a docker image, e.g.:
$ cd [kernel source code]
$ docker run -v "${PWD}:${PWD}:rw" -w "${PWD}" --privileged --rm -it \
--pull always mptcp/mptcp-upstream-virtme-docker:latest \
auto-normal
For more details:
https://github.com/multipath-tcp/mptcp-upstream-virtme-docker
Please note that despite all the efforts that have been already done to have a
stable tests suite when executed on a public CI like here, it is possible some
reported issues are not due to your modifications. Still, do not hesitate to
help us improve that ;-)
Cheers,
MPTCP GH Action bot
Bot operated by Matthieu Baerts (NGI0 Core)
^ permalink raw reply [flat|nested] 8+ messages in thread