Re: [PATCH mptcp-net] mptcp: reset MP_JOIN subflows on missing or invalid DSS options

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mptcp: DoS and skipped stats via mptcp_subflow_fail() on MP_JOIN
--

commit 7946d5d8343cbb66aa7761dd1659532067edd960
Author: T S Rameshkumar <rameshsv06@gmail.com>

mptcp: reset MP_JOIN subflows on missing or invalid DSS options

This commit enforces RFC 8684 section 3.7 by explicitly resetting
additional MP_JOIN subflows that encounter missing or invalid DSS
mappings. It prevents these subflows from incorrectly falling back
to regular TCP when fallback is already allowed on the connection.

Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/544

> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 2c72cb50fbf36..421832874cc62 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1446,7 +1446,7 @@ static bool subflow_check_data_avail(struct sock *ssk)
>  	return false;
>  
>  fallback:
> -	if (!__mptcp_check_fallback(msk)) {
> +	if (subflow->mp_join || ssk != READ_ONCE(msk->first)) {
>  		/* RFC 8684 section 3.7. */
>  		if (status == MAPPING_BAD_CSUM &&
>  		    (subflow->mp_join || subflow->valid_csum_seen)) {
>  			subflow->send_mp_fail = 1;
>  
>  			if (!mptcp_subflow_fail(msk, ssk)) {

[Severity: High]
Is it possible to trigger the WARN_ON_ONCE() in mptcp_subflow_fail() when
receiving a bad checksum on an MP_JOIN subflow?

In mptcp_subflow_fail(), we have:

net/mptcp/subflow.c:mptcp_subflow_fail() {
	...
	if (!msk->allow_infinite_fallback) {
		spin_unlock_bh(&msk->fallback_lock);
		return false;
	}
	...
	if (WARN_ON_ONCE(ssk != READ_ONCE(msk->first)))
		return false;
	...
}

Could a remote packet with an invalid checksum on a newly established MP_JOIN
subflow hit this warning before allow_infinite_fallback is set to false?

Looking at mptcp_finish_join() in net/mptcp/protocol.c:

	} else {
		/* defer the graft to the msk lock holder */
		set_bit(MPTCP_WORK_RTX, &msk->flags);
		if (list_empty(&subflow->node))
			list_add_tail(&subflow->node, &msk->join_list);
	}

If the user holds the socket lock (for example, during recvmsg()), the subflow
graft is deferred and msk->allow_infinite_fallback may still be true. During
this window, would a bad checksum packet processed in softirq bypass the early
return in mptcp_subflow_fail(), hit the WARN_ON_ONCE() for the MP_JOIN subflow,
and potentially cause a kernel panic?

>  				subflow->reset_transient = 0;
>  				subflow->reset_reason = MPTCP_RST_EMIDDLEBOX;
>  				goto reset;

[Severity: High]
If mptcp_subflow_fail() returns false and we branch to the reset label here,
doesn't this bypass the MPTCP_MIB_DSSRESET counter increment intended by this
patch?

If msk->allow_infinite_fallback is false, mptcp_subflow_fail() will immediately
return false for an MP_JOIN subflow. Does the resulting goto reset unintentionally
skip over the new MPTCP_INC_STATS(..., MPTCP_MIB_DSSRESET) below?

>  			}
>  			WRITE_ONCE(subflow->data_avail, true);
>  			return true;
>  		}
>  
> +		subflow->reset_transient = 0;
> +		subflow->reset_reason = status == MAPPING_NODSS ?
> +					MPTCP_RST_EMIDDLEBOX :
> +					MPTCP_RST_EMPTCP;
> +		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DSSRESET);
> +		goto reset;
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012112625.578911-1-rameshkumar.t@phytecembedded.in?part=1