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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.