From: sashiko-bot@kernel.org
To: "Geliang Tang" <geliang@kernel.org>, mptcp@lists.linux.dev
Subject: Re: [PATCH mptcp-next v26 7/8] mptcp: sync mptcp skb cb layout with tcp one
Date: Fri, 13 Nov 2026 10:30:02 +0000 [thread overview]
Message-ID: <sashiko-outbox-212622@kernel.org> (raw)
In-Reply-To: <328ddc33883506606aecad77f6645616df082740.1794563432.git.tanggeliang@kylinos.cn>
Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] mptcp: u32 underflow of partial-packet copy_len in
__mptcp_move_skb()
- [High] mptcp: __mptcp_try_coalesce() overwrites target skb offset in
end_seq
- [High] mptcp: mptcp_data_queue_ofo() uses map_seq instead of payload
start dsn
- [High] mptcp: rmem and fwdmem leak in __mptcp_move_skb() on
rcv_data_fin drop
- [Medium] mptcp: incomplete DATA_FIN sequence and state check in
__mptcp_move_skb()
--
commit af240c23f3ffb2394bc65420f6d1a5d0d83763e5
Author: Paolo Abeni <pabeni@redhat.com>
mptcp: sync mptcp skb cb layout with tcp one
This commit aligns struct mptcp_skb_cb with struct tcp_skb_cb by converting
map_seq and end_seq to u32, adding a 64-bit map_seq64 field, and updating
the receive and out-of-order paths to use 32-bit sequence helpers where
applicable.
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 99dfcd0488a45..d124b702c9576 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -167,14 +167,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)->map_seq +
> - to->len ||
> + if (MPTCP_SKB_CB(from)->map_seq64 != MPTCP_SKB_CB(to)->map_seq64 +
> + to->len ||
> MPTCP_SKB_CB(from)->flags & MPTCP_SKB_CB_F_OFFSET ||
> ((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;
[Severity: High]
Does assigning MPTCP_SKB_CB(to)->end_seq = MPTCP_SKB_CB(from)->end_seq lose
the subflow offset encoded in to when to has MPTCP_SKB_CB_F_OFFSET set?
mptcp_init_skb() sets end_seq to map_seq + copy_len (which is map_seq +
skb->len - offset), and __mptcp_move_skb() and __mptcp_ofo_queue() recover
copy_len = end_seq - map_seq and dsn = map_seq64 + skb->len - copy_len.
When from (with offset == 0 and end_seq == from->map_seq + from->len) is
coalesced into a target skb that had offset > 0, overwriting to->end_seq
with from->end_seq instead of adding from->len makes to->end_seq -
to->map_seq equal the full new to->len. When the coalesced skb is later
dequeued from the backlog or out-of-order queue, its recovered dsn is
shifted backwards by offset bytes, causing premature dequeue across gaps and
delivering stale subflow prefix bytes to user space.
[ ... ]
> @@ -303,14 +303,15 @@ 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)->map_seq + skb->len;
> + end_seq = MPTCP_SKB_CB(skb)->map_seq64 + skb->len;
> max_seq = atomic64_read(&msk->rcv_wnd_sent);
[ ... ]
> @@ -360,19 +361,19 @@ 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)->map_seq + skb1->len)) {
> - if (!after64(end_seq,
> - MPTCP_SKB_CB(skb1)->map_seq + skb1->len)) {
> + if (before(seq, MPTCP_SKB_CB(skb1)->map_seq + skb1->len)) {
> + if (!after((u32)end_seq,
> + MPTCP_SKB_CB(skb1)->map_seq + skb1->len)) {
> /* 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)) {
[Severity: High]
Should mptcp_data_queue_ofo() order and deduplicate out-of-order segments
using the start DSN of their mapped payload rather than
MPTCP_SKB_CB(skb)->map_seq?
When an out-of-order skb has offset > 0, map_seq is dsn - offset, so
mptcp_data_queue_ofo() treats the unmapped subflow prefix [dsn - offset,
dsn) as present data in msk->out_of_order_queue. A valid out-of-order
segment covering [dsn - offset, dsn) is then dropped as MPTCP_MIB_DUPDATA
(or evicted by rb_replace_node() / merge_right), while __mptcp_ofo_queue()
computes the true dsn and waits for msk->ack_seq to reach dsn before
dequeuing the skb, stalling the connection.
[ ... ]
> @@ -455,19 +457,20 @@ void __mptcp_sync_rcv_sequence(struct sock *sk)
> if (!skb)
> return;
>
> - MPTCP_SKB_CB(skb)->map_seq = msk->tfo_iasn - skb->len;
> + MPTCP_SKB_CB(skb)->map_seq64 = msk->tfo_iasn - skb->len;
> + 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;
> }
>
> 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;
> - u64 dsn = MPTCP_SKB_CB(skb)->map_seq + skb->len - copy_len;
> + u32 copy_len = MPTCP_SKB_CB(skb)->end_seq - MPTCP_SKB_CB(skb)->map_seq;
> + u64 dsn = MPTCP_SKB_CB(skb)->map_seq64 + skb->len - copy_len;
> struct mptcp_sock *msk = mptcp_sk(sk);
> struct sk_buff *tail;
>
> if (READ_ONCE(msk->rcv_data_fin) &&
> - !before64(MPTCP_SKB_CB(skb)->map_seq,
> + !before64(MPTCP_SKB_CB(skb)->map_seq64,
> READ_ONCE(msk->rcv_data_fin_seq))) {
[Severity: High]
Does dropping skb in this rcv_data_fin check before calling
mptcp_borrow_fwdmem(sk, skb) leak ssk->sk_rmem_alloc and lent forward
memory?
Callers of __mptcp_move_skb() first call mptcp_subflow_lend_fwdmem(), which
clears skb->destructor while leaving skb->truesize charged to
ssk->sk_rmem_alloc. Because mptcp_drop(sk, skb) is called here before
mptcp_borrow_fwdmem(sk, skb) transfers the forward memory to sk and
subtracts skb->truesize from ssk->sk_rmem_alloc, ssk->sk_rmem_alloc remains
permanently charged when the subflow socket closes.
[Severity: Medium]
Can segments spanning or arriving after rcv_data_fin_seq bypass this
DATA_FIN check?
Checking MPTCP_SKB_CB(skb)->map_seq64 instead of MPTCP_SKB_CB(skb)->map_seq64
+ skb->len accepts segments that start before rcv_data_fin_seq (including
segments at rcv_data_fin_seq with offset > 0) and extend past it, advancing
msk->ack_seq past rcv_data_fin_seq so mptcp_pending_data_fin() never
matches. In addition, once mptcp_check_data_fin() processes DATA_FIN, it
clears msk->rcv_data_fin to 0 and increments msk->ack_seq to
rcv_data_fin_seq + 1:
net/mptcp/protocol.c:mptcp_check_data_fin() {
if (mptcp_pending_data_fin(sk, &rcv_data_fin_seq)) {
WRITE_ONCE(msk->ack_seq, msk->ack_seq + 1);
WRITE_ONCE(msk->rcv_data_fin, 0);
WRITE_ONCE(sk->sk_shutdown, sk->sk_shutdown | RCV_SHUTDOWN);
}
A subsequent segment at rcv_data_fin_seq + 1 then sees msk->rcv_data_fin == 0
and dsn == msk->ack_seq and is queued to sk_receive_queue while
msk->copied_seq stays at rcv_data_fin_seq, causing offset in
__mptcp_recvmsg_mskq() to wrap to U32_MAX and skip freeing the skb.
> @@ -505,8 +508,8 @@ 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;
> + 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;
> }
[Severity: High]
Can copy_len undercount by offset or underflow u32 when a partial packet has
a non-zero subflow offset?
mptcp_init_skb() sets MPTCP_SKB_CB(skb)->end_seq to map_seq + copy_len,
which is map_seq + skb->len - offset. When offset > 0 and msk->ack_seq
falls in the last offset bytes of the skb ([map_seq64 + skb->len - offset,
map_seq64 + skb->len)), the after64() check passes while end_seq <=
(u32)msk->ack_seq, so MPTCP_SKB_CB(skb)->end_seq - (u32)msk->ack_seq wraps
around to ~4 GiB and corrupts msk->bytes_received and msk->ack_seq at
insert. Should this compute MPTCP_SKB_CB(skb)->map_seq + skb->len -
(u32)msk->ack_seq, matching __mptcp_ofo_queue()?
--
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794563432.git.tanggeliang@kylinos.cn?part=7
next prev parent reply other threads:[~2026-11-13 10:30 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-11-13 9:58 [PATCH mptcp-next v26 0/8] Reduce the differences between TCP and MPTCP for TLS usage Geliang Tang
2026-11-13 9:58 ` [PATCH mptcp-net v26 1/8] mptcp: fix TFO SIOCINQ overflow in mptcp_inq_hint Geliang Tang
2026-11-13 9:58 ` [PATCH mptcp-next v26 2/8] selftests: mptcp: connect: check TFO SIOCINQ overflow Geliang Tang
2026-11-13 9:58 ` [PATCH mptcp-net v26 3/8] mptcp: fix fallback dummy mapping anchor Geliang Tang
2026-11-13 9:58 ` [PATCH mptcp-next v26 4/8] mptcp: drop the mptcp_ooo_try_coalesce() helper Geliang Tang
2026-11-13 9:58 ` [PATCH mptcp-next v26 5/8] mptcp: drop the cant_coalesce CB field Geliang Tang
2026-11-13 9:58 ` [PATCH mptcp-next v26 6/8] mptcp: remove CB offset field Geliang Tang
2026-11-13 10:28 ` sashiko-bot
2026-11-13 9:58 ` [PATCH mptcp-next v26 7/8] mptcp: sync mptcp skb cb layout with tcp one Geliang Tang
2026-11-13 10:30 ` sashiko-bot [this message]
2026-11-13 9:58 ` [PATCH mptcp-next v26 8/8] mptcp: defer read_sock cleanup to mptcp_worker Geliang Tang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=sashiko-outbox-212622@kernel.org \
--to=sashiko-bot@kernel.org \
--cc=geliang@kernel.org \
--cc=mptcp@lists.linux.dev \
--cc=sashiko-reviews@lists.linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox