Re: [PATCH mptcp-next v27 6/8] mptcp: remove CB offset field

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] mptcp: raw map_seq used instead of mptcp_dsn() in
  mptcp_data_queue_ofo()
- [Medium] mptcp: raw map_seq and end_seq used in
  mptcp_prune_ofo_queue() and tracepoint
--

commit 9093e378164791c8e9a9ecffc602703ba0b1790e
Author: Paolo Abeni <pabeni@redhat.com>

mptcp: remove CB offset field

This commit removes the offset field from struct mptcp_skb_cb, tracking
consumed bytes via msk->copied_seq and encoding any subflow skb offset into
MPTCP_SKB_CB(skb)->map_seq alongside the mptcp_dsn() and mptcp_end_dsn()
helpers.

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 41fc2429d2fed..f4ab3bdd368f2 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> @@ -362,8 +363,8 @@ static void mptcp_data_queue_ofo(struct mptcp_sock *msk, struct sk_buff *skb)
>  			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 (before64(seq, mptcp_end_dsn(skb1))) {
> +			if (!after64(end_seq, mptcp_end_dsn(skb1))) {
>  				/* All the bits are present. Drop. */
>  				mptcp_drop(sk, skb);
>  				MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_DUPDATA);

[Severity: High]
Should the RB-tree traversal in mptcp_data_queue_ofo() compare seq against
mptcp_dsn(skb1) instead of MPTCP_SKB_CB(skb1)->map_seq?

net/mptcp/protocol.c:mptcp_data_queue_ofo() {
    ...
	while (*p) {
		parent = *p;
		skb1 = rb_to_skb(parent);
		if (before64(seq, MPTCP_SKB_CB(skb1)->map_seq)) {
			p = &parent->rb_left;
			continue;
		}
		if (before64(seq, mptcp_end_dsn(skb1))) {
			if (!after64(end_seq, mptcp_end_dsn(skb1))) {
				/* 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)) {
    ...
}

When mptcp_init_skb() initializes an skb with a non-zero subflow offset,
MPTCP_SKB_CB(skb1)->map_seq is set to mptcp_dsn(skb1) - offset.  If that skb
is queued in msk->out_of_order_queue and a subsequent out-of-order segment
arrives with seq in [mptcp_dsn(skb1) - offset, mptcp_dsn(skb1)), the
before64(seq, MPTCP_SKB_CB(skb1)->map_seq) check is false instead of moving to
rb_left.  Any segment in that range with end_seq <= mptcp_end_dsn(skb1) (such
as the segment immediately preceding skb1) is then dropped as duplicate data,
or if end_seq > mptcp_end_dsn(skb1) it is inserted into the right subtree of
skb1 despite having a smaller DSN, stalling the receive queue when
__mptcp_ofo_queue() later stops at mptcp_dsn(skb1) > msk->ack_seq.

[ ... ]

> @@ -425,9 +426,10 @@ 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)->map_seq = mptcp_subflow_get_mapped_dsn(subflow) -
> +				     offset;
>  	MPTCP_SKB_CB(skb)->end_seq = MPTCP_SKB_CB(skb)->map_seq + copy_len;
> -	MPTCP_SKB_CB(skb)->offset = offset;
> +	MPTCP_SKB_CB(skb)->flags = offset ? MPTCP_SKB_CB_F_OFFSET : 0;
>  	MPTCP_SKB_CB(skb)->has_rxtstamp = has_rxtstamp;

[Severity: Medium]
Now that map_seq and end_seq are shifted down by offset, should
mptcp_prune_ofo_queue() and trace_mptcp_rcvbuf_grow() also use mptcp_dsn() and
mptcp_end_dsn()?

net/mptcp/protocol.c:mptcp_prune_ofo_queue() {
    ...
		/* 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))
			break;
    ...
}

When either in_skb or the tail skb in msk->out_of_order_queue has a non-zero
offset, comparing raw map_seq values can stop pruning early (causing
__mptcp_move_skb() or mptcp_data_queue_ofo() to drop an earlier segment under
memory pressure) or prune out-of-order segments whose actual DSN precedes
in_skb.  Similarly, the mptcp_rcvbuf_grow tracepoint in
include/trace/events/mptcp.h still computes ooo_space from
MPTCP_SKB_CB(msk->ooo_last_skb)->end_seq - msk->ack_seq instead of
mptcp_end_dsn(msk->ooo_last_skb) - msk->ack_seq.

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