Re: [PATCH mptcp-net 08/34] mptcp: force the charge for the first skb of a subflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] mptcp: cross-subflow state leak in __mptcp_push_pending()
  [1]
--

commit 045933b0c1ae73b56c16499f1fb80d8244a44751
Author: Chris Mason <mason@kernel.org>

mptcp: force the charge for the first skb of a subflow

This commit forces the memory charge when allocating the first skb on an
empty MPTCP subflow and uses tcp_wmem_schedule() to bound the payload copy
when charging subflow write memory in mptcp_sendmsg_frag().

> diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c
> index dc8cdda31a984..8d6ed293f6e58 100644
> --- a/net/mptcp/protocol.c
> +++ b/net/mptcp/protocol.c
[ ... ]
> @@ -1449,6 +1453,7 @@ static int mptcp_sendmsg_frag(struct sock *sk, struct sock *ssk,
>  	bool reuse_skb = true;
>  	struct sk_buff *skb;
>  	size_t copy;
> +	int avail;
>  	int i;
>  
>  	pr_debug("msk=%p ssk=%p sending dfrag at seq=%llu len=%u already sent=%u\n",

[Severity: Critical]
This problem wasn't introduced by this patch, but can __mptcp_push_pending()
leak stale info.mss_now and info.size_goal across subflows [1] when
mptcp_sendmsg_frag() returns early?

__mptcp_push_pending() shares a single struct mptcp_sendmsg_info across its
subflow loop without resetting it when switching subflows:

net/mptcp/protocol.c:__mptcp_push_pending() {
	struct mptcp_sendmsg_info info = {
				.flags = flags,
	};
    ...
		mptcp_for_each_subflow(msk, subflow) {
			if (READ_ONCE(subflow->scheduled)) {
    ...
				if (ssk != prev_ssk) {
					if (prev_ssk)
						mptcp_push_release(prev_ssk, &info);
					lock_sock(ssk);
				}
    ...
				ret = __subflow_push_pending(sk, ssk, &info);
				if (ret <= 0) {
    ...
					continue;
				}
    ...
	if (ssk)
		mptcp_push_release(ssk, &info);
}

After mptcp_sendmsg_frag() populates info.mss_now and info.size_goal for one
subflow, a subsequent subflow can fail in mptcp_sendmsg_frag() at
!__tcp_can_send(ssk) before tcp_send_mss() overwrites those fields.  When
mptcp_push_release() runs for that second subflow, it sees the first
subflow's non-zero info.mss_now and calls tcp_push() with the wrong
mss_now and size_goal, causing tcp_write_xmit() to segment queued frames
with another subflow's segment size and exceed the path mtu.

[1] https://sashiko.dev/#/bug/linux-3f08f285-058d-4038-8cd3-6d287fea7814

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-mptcp-cmason-fixes-v1-0-9205b77e9a2f@kernel.org?part=8