Re: [PATCH mptcp-next v2 3/8] mptcp: sched: let get_retrans reinject on an idle subflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] mptcp: 32-bit overflow in __mptcp_oppr() and mptcp_oppr_send_range() budget math
- [Medium] mptcp: __mptcp_oppr() continues to later DSN ranges after partial send failure
--

commit be2c71f8537cecaea5b15cddbe5f79d5a824718f
Author: Abdul Wasey <w453y.me@gmail.com>

mptcp: sched: let get_retrans reinject on an idle subflow

This commit extends the MPTCP scheduler get_retrans callback to support
opportunistic reinjection on an idle subflow when no new data can be sent.
The core collects the unacknowledged DSN ranges held by other subflows and
resends them on the selected subflow up to reinject_limit and one cwnd.

Link: https://github.com/multipath-tcp/mptcp_net-next/issues/332 [1]

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index d960ab9c76b32..7117786e52513 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -3057,6 +3057,92 @@ static int __mptcp_push_retrans(struct sock *sk, struct mptcp_data_frag *dfrag,
>  	return len;
>  }
>  
> +/* resend [start, end) on @ssk from the msk rtx queue; < 0 on a send error */
> +static int mptcp_oppr_send_range(struct sock *sk, struct sock *ssk, u64 start,
> +				 u64 end, int budget,
> +				 struct mptcp_sendmsg_info *info)
> +{
> +	struct mptcp_data_frag *dfrag;
> +	int copied = 0, ret;
> +
> +	list_for_each_entry(dfrag, &mptcp_sk(sk)->rtx_queue, list) {
> +		if (!dfrag->already_sent || !before64(dfrag->data_seq, end))
> +			break;
> +		if (!after64(dfrag->data_seq + dfrag->already_sent, start))
> +			continue;
> +
> +		info->sent = before64(dfrag->data_seq, start) ?
> +			     start - dfrag->data_seq : 0;
> +		info->limit = min_t(u64, dfrag->already_sent,
> +				    end - dfrag->data_seq);
> +		info->limit = min_t(u64, info->limit,
> +				    info->sent + budget - copied);
> +		while (info->sent < info->limit && copied < budget) {
> +			ret = mptcp_sendmsg_frag(sk, ssk, dfrag, info);
> +			if (ret <= 0)
> +				return copied ? : -1;
> +			copied += ret;
> +			info->sent += ret;
> +		}
> +		if (copied >= budget)
> +			break;
> +	}
> +	return copied;
> +}
> +
> +/* Opportunistic reinjection: the scheduling part is in sched.c */
> +static void __mptcp_oppr(struct sock *sk)
> +{
> +	struct mptcp_sendmsg_info info = { .data_lock_held = true, };
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	int copied = 0, budget, ret, i;
> +	struct mptcp_sched_oppr oppr;
> +	struct sock *ssk;
> +
> +	mptcp_data_lock(sk);
> +	__mptcp_clean_una_wakeup(sk);
> +	oppr.una = msk->snd_una;
> +	mptcp_data_unlock(sk);
> +
> +	if (!mptcp_sched_oppr_get(msk, &oppr))
> +		return;
> +
> +	ssk = oppr.ssk;
> +	lock_sock(ssk);
> +	/* the scheduler's limit, and no more than one cwnd */
> +	budget = min_t(u64, oppr.limit,
> +		       tcp_snd_cwnd(tcp_sk(ssk)) * tcp_sk(ssk)->mss_cache);

[Severity: Medium]
Can the budget calculation in __mptcp_oppr() and mptcp_oppr_send_range()
overflow 32-bit integers when a scheduler sets a large subflow->reinject_limit
on a subflow with a large cwnd?

In __mptcp_oppr(), tcp_snd_cwnd(tcp_sk(ssk)) and tcp_sk(ssk)->mss_cache are
both u32, so their multiplication is evaluated in 32-bit unsigned arithmetic
before min_t(u64, ...) promotes the product to u64, wrapping if the product
reaches 2^32.

In addition, oppr.limit is u32 while budget and copied are signed int. If both
oppr.limit and the cwnd * mss_cache product are >= 0x80000000U (for example
when a scheduler sets reinject_limit = U32_MAX on a high-BDP path), assigning
the min_t(u64, ...) result to signed int budget makes budget negative. The
loop condition copied < budget in __mptcp_oppr() is then immediately false,
skipping reinjection without updating msk->oppr_una.

Similarly, in mptcp_oppr_send_range():

	info->limit = min_t(u64, info->limit,
			    info->sent + budget - copied);

info->sent + budget - copied is evaluated as a signed 32-bit int before
min_t(u64, ...) casts it to u64. If budget is near INT_MAX and info->sent >
copied (such as when start > dfrag->data_seq), the signed addition overflows
to a negative int, which sign-extends to a large u64 and fails to clamp
info->limit to the remaining budget.

> +
> +	spin_lock_bh(&msk->fallback_lock);
> +	if (__mptcp_check_fallback(msk) || !msk->allow_subflows)
> +		goto unlock;
> +
> +	for (i = 0; i < oppr.n && copied < budget; i++) {
> +		ret = mptcp_oppr_send_range(sk, ssk, oppr.r[i].start,
> +					    oppr.r[i].end, budget - copied,
> +					    &info);
> +		if (ret < 0)
> +			break;
> +		copied += ret;
> +	}

[Severity: Medium]
If mptcp_sendmsg_frag() fails partway through sending a DSN range in
mptcp_oppr_send_range(), should __mptcp_oppr() stop rather than continuing to
subsequent ranges in oppr.r[]?

In mptcp_oppr_send_range(), when mptcp_sendmsg_frag() returns <= 0 (for
example -ENOMEM from sk_wmem_schedule() or 0 when hitting mptcp_wnd_end(msk))
after an earlier fragment in the same range already sent data (copied > 0),
it returns the positive copied count via:

	if (ret <= 0)
		return copied ? : -1;

Because the loop in __mptcp_oppr() only breaks on ret < 0, a partial send on
oppr.r[i] causes __mptcp_oppr() to advance to oppr.r[i + 1]. If a later,
smaller range succeeds, higher-DSN data is transmitted while leaving an unsent
gap in the earlier range that holds back snd_una, and msk->oppr_una is still
recorded as oppr.una so the failed portion is not retried for this snd_una.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261031-mptcp-sched-opp-retrans-v2-0-e9c4a30f6841@kernel.org?part=3