Re: [RFC PATCH mptcp-next 1/1] mptcp: opportunistic reinjection on an idle subflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] mptcp: race on msk->conn_list in mptcp_oppr_check() when socket owned
- [Medium] mptcp: missing csum_enabled check in mptcp_oppr_blocked()
- [Medium] mptcp: mptcp_oppr_send_range() does not cap info->limit to budget
- [Medium] mptcp: mptcp_oppr_collect() does not merge overlapping DSN ranges
- [Medium] mptcp: flawed gain and oppr_una tracking in mptcp_oppr_collect()
--

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

mptcp: opportunistic reinjection on an idle subflow

This commit implements opportunistic reinjection for MPTCP when a connection
cannot send new data while an active subflow is idle and another slower or
inactive subflow still holds unacknowledged data. It collects the unacked
DSN ranges from the other subflows and resends them on the idle subflow via
the MPTCP worker.

> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index adbf8e0773dbb..b2b4e6b23b6b8 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -1154,6 +1154,7 @@ static void ack_update_msk(struct mptcp_sock *msk,
>  		__mptcp_data_acked(sk);
>  	}
>  	msk->last_ack_recv = tcp_jiffies32;
> +	mptcp_oppr_check(sk);
>  	mptcp_data_unlock(sk);
>  
>  	trace_ack_update_msk(mp_opt->data_ack,
> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 02a4f479d9d8c..cd9aebd66b1e3 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c

[ ... ]

> +/* the active subflow with nothing to send or retransmit and the lowest RTT */
> +static struct sock *mptcp_oppr_idle_ssk(struct mptcp_sock *msk)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	struct sock *pick = NULL;
> +	u32 best = U32_MAX;
> +
> +	mptcp_for_each_subflow(msk, subflow) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +		u32 rtt = READ_ONCE(tcp_sk(ssk)->srtt_us);
> +
> +		if (!__mptcp_subflow_active(subflow) || subflow->backup ||
> +		    subflow->request_bkup || subflow->stale || !rtt)
> +			continue;
> +		if (!tcp_rtx_and_write_queues_empty(ssk))
> +			continue;
> +		if (rtt < best) {
> +			best = rtt;
> +			pick = ssk;
> +		}
> +	}
> +	return pick;
> +}
> +
> +static bool mptcp_oppr_blocked(struct sock *sk)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	u64 snd_una = READ_ONCE(msk->snd_una);
> +
> +	if (snd_una == READ_ONCE(msk->snd_nxt) ||
> +	    snd_una == READ_ONCE(msk->oppr_una) ||
> +	    __mptcp_check_fallback(msk))
> +		return false;

[Severity: Medium]
Should mptcp_oppr_blocked() also check READ_ONCE(msk->csum_enabled) here?

In __mptcp_oppr(), the function returns early when msk->csum_enabled is set,
before msk->oppr_una is updated to msk->snd_una:

    if (!mptcp_oppr_blocked(sk) || READ_ONCE(msk->csum_enabled))
            return;
    ...
    una = msk->snd_una;
    WRITE_ONCE(msk->oppr_una, una);

Because mptcp_oppr_blocked() does not check msk->csum_enabled,
ack_update_msk() -> mptcp_oppr_check() will walk msk->conn_list twice and
schedule mptcp_worker on every incoming DATA_ACK while blocked when data
checksums are enabled, only for __mptcp_oppr() to immediately return without
updating msk->oppr_una.

> +
> +	/* new data can still go out: the scheduler will use the idle subflow */
> +	if (mptcp_send_head(sk) &&
> +	    mptcp_wnd_end(msk) - READ_ONCE(msk->snd_nxt) >= MPTCP_OPPR_MIN)
> +		return false;
> +	return true;
> +}

[ ... ]

> +static bool mptcp_oppr_worth(struct mptcp_sock *msk, struct sock *idle)
> +{
> +	struct mptcp_subflow_context *subflow;
> +
> +	mptcp_for_each_subflow(msk, subflow) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +
> +		if (ssk == idle || tcp_rtx_and_write_queues_empty(ssk))
> +			continue;
> +		if (!__mptcp_subflow_active(subflow) ||
> +		    mptcp_oppr_gain(ssk, idle))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +/* called on each DATA_ACK, under the msk data lock */
> +void mptcp_oppr_check(struct sock *sk)
> +{
> +	struct mptcp_sock *msk = mptcp_sk(sk);
> +	struct sock *idle;
> +
> +	if (!mptcp_oppr_blocked(sk))
> +		return;
> +	idle = mptcp_oppr_idle_ssk(msk);
> +	if (!idle || !mptcp_oppr_worth(msk, idle))
> +		return;
> +	if (!test_and_set_bit(MPTCP_WORK_OPPR, &msk->flags))
> +		mptcp_schedule_work(sk);
> +}

[Severity: Critical]
Can calling mptcp_oppr_check() from ack_update_msk() race with concurrent
modifications to msk->conn_list and subflow teardown when the msk socket is
owned by user context?

In ack_update_msk(), only mptcp_data_lock(sk) (sk->sk_lock.slock) is held.
When a process-context thread holds lock_sock(sk), sk->sk_lock.owned is set
while sk->sk_lock.slock is released. Under lock_sock(sk) and without holding
mptcp_data_lock(sk), msk->conn_list is modified with non-RCU list operations
and subflows are closed and freed, for instance in __mptcp_close_ssk():

    dispose_it = msk->free_first || ssk != msk->first;
    if (dispose_it)
            list_del(&subflow->node);
    ...
    __tcp_close(ssk, 0);
    ...
    sock_put(ssk);

As well as list_add_tail() / list_del() in __mptcp_subflow_connect(),
list_move_tail() in __mptcp_flush_join_list(), and list_del() in
mptcp_subflow_drop_ctx(). Similarly, msk->first_pending (read via
mptcp_send_head(sk) in mptcp_oppr_blocked()) is updated under the msk socket
lock without holding mptcp_data_lock(sk).

Unlike __mptcp_check_push() and __mptcp_data_acked() in ack_update_msk(),
which check !sock_owned_by_user(sk) and defer work via msk->cb_flags when
the socket is owned, mptcp_oppr_check() walks msk->conn_list in
mptcp_oppr_idle_ssk() and mptcp_oppr_worth() and dereferences subflow and
ssk unconditionally. Could this dereference LIST_POISON1/LIST_POISON2 or a
freed ssk/subflow in softirq context?

[ ... ]

> +/* The DSN ranges at or after @una that the subflows slower than @idle still
> + * hold, sent or not: the data the peer is waiting for. Sorted by start.
> + * @gain is set to the largest time the copies can save.
> + */
> +static int mptcp_oppr_collect(struct mptcp_sock *msk, struct sock *idle,
> +			      u64 una, struct mptcp_oppr_range *r, u32 *gain)
> +{
> +	struct mptcp_subflow_context *subflow;
> +	int n = 0, i, k;
> +
> +	mptcp_for_each_subflow(msk, subflow) {
> +		struct sock *ssk = mptcp_subflow_tcp_sock(subflow);
> +		struct sk_buff *skb;
> +		u64 last = 0;
> +		u32 g = 0;
> +
> +		if (ssk == idle)
> +			continue;
> +		if (__mptcp_subflow_active(subflow)) {
> +			g = mptcp_oppr_gain(ssk, idle);
> +			if (!g)
> +				continue;
> +		}
> +		*gain = max(*gain, g);
> +
> +		lock_sock(ssk);
> +		skb_rbtree_walk(skb, &ssk->tcp_rtx_queue)
> +			n = mptcp_oppr_add_skb(skb, una, &last, r, n);
> +		skb_queue_walk(&ssk->sk_write_queue, skb)
> +			n = mptcp_oppr_add_skb(skb, una, &last, r, n);
> +		release_sock(ssk);
> +	}

[Severity: Medium]
Does updating *gain = max(*gain, g) before walking the queues of ssk cause
two issues with how gain is tracked?

First, unlike mptcp_oppr_worth(), this loop does not check whether ssk
actually holds any unacked ranges after una before updating *gain, so an
active slow subflow with empty queues can inflate *gain.

Second, for an inactive subflow (!__mptcp_subflow_active(subflow)), g is
left at 0 so that __mptcp_oppr() does not cap budget by the pacing rate
(since a subflow that is gone saves an MPTCP RTO). However, if another
active subflow on the connection has g > 0, *gain = max(*gain, g) overwrites
the 0 sentinel with g, causing __mptcp_oppr() to cap the budget for the
inactive subflow's data.

In addition, in __mptcp_oppr(), WRITE_ONCE(msk->oppr_una, una) is executed
before calling mptcp_oppr_collect() and before checking whether any bytes
were actually copied. If mptcp_oppr_collect() returns 0 or no bytes are
sent, should msk->oppr_una remain unchanged so reinjection is not
permanently suppressed for that snd_una?

> +
> +	for (i = 1; i < n; i++) {
> +		struct mptcp_oppr_range t = r[i];
> +
> +		for (k = i; k > 0 && after64(r[k - 1].start, t.start); k--)
> +			r[k] = r[k - 1];
> +		r[k] = t;
> +	}
> +	return n;
> +}

[Severity: Medium]
What happens if two non-idle subflows hold overlapping or identical DSN
ranges in their queues (for example, after an earlier retransmission or
subflow closure)?

mptcp_oppr_collect() sorts r[] by start sequence number, but it does not
merge overlapping ranges where before64(r[i].start, r[i - 1].end). When
__mptcp_oppr() iterates over r[] and calls mptcp_oppr_send_range() for each
entry, won't it retransmit the overlapping DSN bytes multiple times on the
same idle subflow and consume the budget on duplicate copies?

> +
> +/* 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);
> +		while (info->sent < info->limit && copied < budget) {
> +			ret = mptcp_sendmsg_frag(sk, ssk, dfrag, info);

[Severity: Medium]
Can mptcp_oppr_send_range() exceed the caller's budget when a dfrag range is
larger than the remaining budget?

In mptcp_sendmsg_frag(), the amount copied in a single call is bounded by
info->limit - info->sent:

    copy = info->size_goal;
    ...
    copy = min_t(size_t, copy, info->limit - info->sent);

Because mptcp_oppr_send_range() sets info->limit from dfrag->already_sent
and end - dfrag->data_seq without clamping it to info->sent + budget -
copied, a single call to mptcp_sendmsg_frag() can copy up to info->size_goal
(or the entire dfrag) before the while loop rechecks copied < budget. Should
info->limit also be capped by info->sent + budget - copied?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025134643.1537069-1-w453y.me@gmail.com?part=1