Re: [PATCH net 2/2] mptcp: close race between scheduler and state change

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] Incomplete fix: the identical `mss_now == 0` divide-by-zero that
  the patch guards in `mptcp_push_release()` (net/mptcp/protocol.c:1667)…

--

mptcp: close race between scheduler and state change

The patch guards the tcp_push() call in mptcp_push_release() with a check
for info->mss_now being non-zero, so that a subflow which failed
transmission (leaving mss_now at 0) is not used for a divide operation.

Closes: https://lore.kernel.org/20260525194828.1137119-1-shardul.b@mpiricsoftware.com

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index 0098e28309318..709610db3ce9d 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
> @@ -1664,7 +1664,9 @@ struct sock *mptcp_subflow_get_send(struct mptcp_sock *msk)
>  
>  static void mptcp_push_release(struct sock *ssk, struct mptcp_sendmsg_info *info)
>  {
> -	tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle, info->size_goal);
> +	if (info->mss_now)
> +		tcp_push(ssk, 0, info->mss_now, tcp_sk(ssk)->nonagle,
> +			 info->size_goal);
>  	release_sock(ssk);
>  }

[Severity: High]
Should the same guard also be added to the second tcp_push() call site, in
__mptcp_subflow_push_pending()?

That function starts with a zeroed info:

net/mptcp/protocol.c:__mptcp_subflow_push_pending() {
	struct mptcp_sendmsg_info info = {
		.data_lock_held = true,
	};
	...
	int copied = 0;

and info.mss_now is only ever assigned inside mptcp_sendmsg_frag(), after
the early return the commit message refers to:

	if (unlikely(!__tcp_can_send(ssk)))
		return -EAGAIN;

	/* compute send limit */
	...
	info->mss_now = tcp_send_mss(ssk, &info->size_goal, info->flags);

In the non-first iteration, the return value is accumulated into copied
without filtering negatives:

net/mptcp/protocol.c:__mptcp_subflow_push_pending() {
		if (READ_ONCE(subflow->scheduled)) {
			mptcp_subflow_set_scheduled(subflow, false);
			ret = __subflow_push_pending(sk, ssk, &info);
			if (ret <= 0)
				keep_pushing = false;
			copied += ret;
		}

__subflow_push_pending() passes mptcp_sendmsg_frag()'s -EAGAIN through
verbatim when the first frag fails, so copied becomes -EAGAIN, which is
non-zero, while info.mss_now is still 0:

out:
	if (copied) {
		tcp_push(ssk, 0, info.mss_now, tcp_sk(ssk)->nonagle,
			 info.size_goal);

Can that reach the divide the patch is trying to avoid?  With a non-empty
subflow write queue, tcp_push() does not take the autocork early return
(it requires skb->len < size_goal, i.e. skb->len < 0), so it calls
__tcp_push_pending_frames(sk, 0, nonagle), which only bails for TCP_CLOSE.
For FIN_WAIT1/FIN_WAIT2/CLOSING/LAST_ACK it continues into
tcp_write_xmit(sk, 0, ...) and tcp_tso_segs() -> tcp_tso_autosize():

net/ipv4/tcp_output.c:tcp_tso_autosize() {
	return max_t(u32, bytes / mss_now, min_tso_segs);

tcp_set_skb_tso_segs() has the same shape with
DIV_ROUND_UP(skb->len, mss_now).

Is the "scheduled but no longer sendable" state reachable on this path?
mptcp_sched_get_send() returns early without re-validating sendability if
any subflow already has the flag set:

net/mptcp/sched.c:mptcp_sched_get_send() {
	mptcp_for_each_subflow(msk, subflow) {
		if (READ_ONCE(subflow->scheduled))
			return 0;
	}

and __mptcp_subflow_push_pending() itself leaves the flag set on the
subflows it delegates via mptcp_subflow_delegate().  The tree also comments
on leftover flags elsewhere ("there could be left-over scheduled subflows;
clear them all or later xmit could use bad ones"), and
__mptcp_push_pending() already anticipates -EAGAIN together with
FIN_WAIT1/FIN_WAIT2 - which are exactly the states where
__tcp_push_pending_frames() does not bail out.

The entry point with first == false is __mptcp_check_push():

	if (!sock_owned_by_user(sk))
		__mptcp_subflow_push_pending(sk, ssk, false);

reached from ack_update_msk() in net/mptcp/options.c on incoming DSS acks,
so this runs in softirq context on peer-supplied input.

A second, smaller question on the same statement: since copied mixes byte
counts with negative errnos, can a positive count followed by an error that
sums to exactly zero (for example 11 bytes then -EAGAIN) make the
if (copied) test false and skip both the flush and
mptcp_reset_rtx_timer() even though data was queued?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260915-net-mptcp-misc-fixes-7-3-rc4-v1-0-a257797dd5d0%40kernel.org