[PATCH v2 mptcp-net 1/2] mptcp: always parse mptcp options for MPC reqsk

[PATCH v2 mptcp-net 1/2] mptcp: always parse mptcp options for MPC reqsk

[Paolo Abeni]

In subflow_syn_recv_sock() we currently skip options parsing
for OoO packet, given that such packets may not carry the relevant
MPC option.

If the peer generates an MPC+data TSO packet and some of the early
segments are lost or get reorder, we server will ignore the peer key,
causing transient, unexpected fallback to TCP.

The solution is always parsing the incoming MPTCP options, and
do the fallback only for in-order packets. This actually cleans
the existing code a bit.

Reported-by: Matthieu Baerts <matthieu.baerts@tessares.net>
Fixes: d22f4988ffec ("mptcp: process MP_CAPABLE data option")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
a note on data ack len: with this patch the server will use
ack32 for OoO MPC+data pkts, and will move to ack64 ASA will get
the first in order MPC+data pkt.

We can clean-up/make more consistent the behavior with some additional
check in mptcp_sk_clone and/or subflow_syn_recv_sock(), but I prefer
to not introduce only partially related changes here
---
 net/mptcp/subflow.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 554e7ccee02a..278986585088 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -633,21 +633,20 @@ static struct sock *subflow_syn_recv_sock(const struct sock *sk,
 
 	/* if the sk is MP_CAPABLE, we try to fetch the client key */
 	if (subflow_req->mp_capable) {
-		if (TCP_SKB_CB(skb)->seq != subflow_req->ssn_offset + 1) {
-			/* here we can receive and accept an in-window,
-			 * out-of-order pkt, which will not carry the MP_CAPABLE
-			 * opt even on mptcp enabled paths
-			 */
-			goto create_msk;
-		}
-
+		/* we can receive and accept an in-window, out-of-order pkt,
+		 * which may not carry the MP_CAPABLE opt even on mptcp enabled
+		 * paths: always try to extract the peer key, and fallback
+		 * for packets missing it.
+		 * Even OoO DSS packets coming legitly after dropped or
+		 * reordered MPC will cause fallback, but we don't have other
+		 * options.
+		 */
 		mptcp_get_options(sk, skb, &mp_opt);
 		if (!mp_opt.mp_capable) {
 			fallback = true;
 			goto create_child;
 		}
 
-create_msk:
 		new_msk = mptcp_sk_clone(listener->conn, &mp_opt, req);
 		if (!new_msk)
 			fallback = true;
-- 
2.26.3

[PATCH v2 mptcp-net 2/2] mptcp: do not reset MP_CAPABLE subflow on mapping errors

[Paolo Abeni]

When some mapping related errors occours we close the main
MPC subflow with a RST. We should instead fallback gracefully
to TCP, and do the reset only for MPJ subflows.

Fixes: d22f4988ffec ("mptcp: process MP_CAPABLE data option")
Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/192
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/mptcp/subflow.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
index 278986585088..9befe9fe7bca 100644
--- a/net/mptcp/subflow.c
+++ b/net/mptcp/subflow.c
@@ -1110,10 +1110,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
 
 		status = get_mapping_status(ssk, msk);
 		trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
-		if (unlikely(status == MAPPING_INVALID)) {
-			ssk->sk_err = EBADMSG;
-			goto fatal;
-		}
+		if (unlikely(status == MAPPING_INVALID))
+			goto fallback;
+
 		if (unlikely(status == MAPPING_DUMMY))
 			goto fallback;
 
@@ -1128,10 +1127,8 @@ static bool subflow_check_data_avail(struct sock *ssk)
 		 * MP_CAPABLE-based mapping
 		 */
 		if (unlikely(!READ_ONCE(msk->can_ack))) {
-			if (!subflow->mpc_map) {
-				ssk->sk_err = EBADMSG;
-				goto fatal;
-			}
+			if (!subflow->mpc_map)
+				goto fallback;
 			WRITE_ONCE(msk->remote_key, subflow->remote_key);
 			WRITE_ONCE(msk->ack_seq, subflow->map_seq);
 			WRITE_ONCE(msk->can_ack, true);
@@ -1160,19 +1157,21 @@ static bool subflow_check_data_avail(struct sock *ssk)
 	subflow_sched_work_if_closed(msk, ssk);
 	return false;
 
-fatal:
-	/* fatal protocol error, close the socket */
-	/* This barrier is coupled with smp_rmb() in tcp_poll() */
-	smp_wmb();
-	ssk->sk_error_report(ssk);
-	tcp_set_state(ssk, TCP_CLOSE);
-	subflow->reset_transient = 0;
-	subflow->reset_reason = MPTCP_RST_EMPTCP;
-	tcp_send_active_reset(ssk, GFP_ATOMIC);
-	subflow->data_avail = 0;
-	return false;
-
 fallback:
+	if (subflow->mp_join) {
+		/* fatal protocol error, close the socket */
+		/* This barrier is coupled with smp_rmb() in tcp_poll() */
+		smp_wmb();
+		ssk->sk_err = EBADMSG;
+		ssk->sk_error_report(ssk);
+		tcp_set_state(ssk, TCP_CLOSE);
+		subflow->reset_transient = 0;
+		subflow->reset_reason = MPTCP_RST_EMPTCP;
+		tcp_send_active_reset(ssk, GFP_ATOMIC);
+		subflow->data_avail = 0;
+		return false;
+	}
+
 	__mptcp_do_fallback(msk);
 	skb = skb_peek(&ssk->sk_receive_queue);
 	subflow->map_valid = 1;
-- 
2.26.3

[PATCH v2 mptcp-net 3/2] mptcp: update selftest for fallback due to OoO

[Paolo Abeni]

The previous commit noted that we can have fallback
scenario due to OoO (or packet drop). Update the self-tests
accordingly

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 tools/testing/selftests/net/mptcp/mptcp_connect.sh | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/net/mptcp/mptcp_connect.sh b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
index 605b8b929f72..69351c3eb68c 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_connect.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_connect.sh
@@ -512,6 +512,7 @@ do_transfer()
 	local stat_ackrx_now_l=$(get_mib_counter "${listener_ns}" "MPTcpExtMPCapableACKRX")
 	local stat_cookietx_now=$(get_mib_counter "${listener_ns}" "TcpExtSyncookiesSent")
 	local stat_cookierx_now=$(get_mib_counter "${listener_ns}" "TcpExtSyncookiesRecv")
+	local stat_ooo_now=$(get_mib_counter "${listener_ns}" "TcpExtTCPOFOQueue")
 
 	expect_synrx=$((stat_synrx_last_l))
 	expect_ackrx=$((stat_ackrx_last_l))
@@ -529,10 +530,14 @@ do_transfer()
 			"${stat_synrx_now_l}" "${expect_synrx}" 1>&2
 		retc=1
 	fi
-	if [ ${stat_ackrx_now_l} -lt ${expect_ackrx} ]; then
-		printf "[ FAIL ] lower MPC ACK rx (%d) than expected (%d)\n" \
-			"${stat_ackrx_now_l}" "${expect_ackrx}" 1>&2
-		rets=1
+	if [ ${stat_ackrx_now_l} -lt ${expect_ackrx} -a ${stat_ooo_now} -eq 0 ]; then
+		if [ ${stat_ooo_now} -eq 0 ]; then
+			printf "[ FAIL ] lower MPC ACK rx (%d) than expected (%d)\n" \
+				"${stat_ackrx_now_l}" "${expect_ackrx}" 1>&2
+			rets=1
+		else
+			printf "[ Note ] fallback due to TCP OoO"
+		fi
 	fi
 
 	if [ $retc -eq 0 ] && [ $rets -eq 0 ]; then
-- 
2.26.3

Re: [PATCH v2 mptcp-net 2/2] mptcp: do not reset MP_CAPABLE subflow on mapping errors

[Mat Martineau]

On Mon, 17 May 2021, Paolo Abeni wrote:

> When some mapping related errors occours we close the main
> MPC subflow with a RST. We should instead fallback gracefully
> to TCP, and do the reset only for MPJ subflows.
>
> Fixes: d22f4988ffec ("mptcp: process MP_CAPABLE data option")
> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/192
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
> net/mptcp/subflow.c | 39 +++++++++++++++++++--------------------
> 1 file changed, 19 insertions(+), 20 deletions(-)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 278986585088..9befe9fe7bca 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -1110,10 +1110,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
>
> 		status = get_mapping_status(ssk, msk);
> 		trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
> -		if (unlikely(status == MAPPING_INVALID)) {
> -			ssk->sk_err = EBADMSG;
> -			goto fatal;
> -		}
> +		if (unlikely(status == MAPPING_INVALID))
> +			goto fallback;
> +

There are a bunch of other ways to get MAPPING_INVALID during the life of 
a connection, including when there are multiple subflows active and 
fallback is not a valid option. Can the new fallback condition be more 
targeted to this "out of order / packet loss at connection time" issue so 
truly fatal MAPPING_INVALID cases still reset the connection?

-Mat


> 		if (unlikely(status == MAPPING_DUMMY))
> 			goto fallback;
>
> @@ -1128,10 +1127,8 @@ static bool subflow_check_data_avail(struct sock *ssk)
> 		 * MP_CAPABLE-based mapping
> 		 */
> 		if (unlikely(!READ_ONCE(msk->can_ack))) {
> -			if (!subflow->mpc_map) {
> -				ssk->sk_err = EBADMSG;
> -				goto fatal;
> -			}
> +			if (!subflow->mpc_map)
> +				goto fallback;
> 			WRITE_ONCE(msk->remote_key, subflow->remote_key);
> 			WRITE_ONCE(msk->ack_seq, subflow->map_seq);
> 			WRITE_ONCE(msk->can_ack, true);
> @@ -1160,19 +1157,21 @@ static bool subflow_check_data_avail(struct sock *ssk)
> 	subflow_sched_work_if_closed(msk, ssk);
> 	return false;
>
> -fatal:
> -	/* fatal protocol error, close the socket */
> -	/* This barrier is coupled with smp_rmb() in tcp_poll() */
> -	smp_wmb();
> -	ssk->sk_error_report(ssk);
> -	tcp_set_state(ssk, TCP_CLOSE);
> -	subflow->reset_transient = 0;
> -	subflow->reset_reason = MPTCP_RST_EMPTCP;
> -	tcp_send_active_reset(ssk, GFP_ATOMIC);
> -	subflow->data_avail = 0;
> -	return false;
> -
> fallback:
> +	if (subflow->mp_join) {
> +		/* fatal protocol error, close the socket */
> +		/* This barrier is coupled with smp_rmb() in tcp_poll() */
> +		smp_wmb();
> +		ssk->sk_err = EBADMSG;
> +		ssk->sk_error_report(ssk);
> +		tcp_set_state(ssk, TCP_CLOSE);
> +		subflow->reset_transient = 0;
> +		subflow->reset_reason = MPTCP_RST_EMPTCP;
> +		tcp_send_active_reset(ssk, GFP_ATOMIC);
> +		subflow->data_avail = 0;
> +		return false;
> +	}
> +
> 	__mptcp_do_fallback(msk);
> 	skb = skb_peek(&ssk->sk_receive_queue);
> 	subflow->map_valid = 1;
> -- 
> 2.26.3
>
>
>

--
Mat Martineau
Intel

Re: [PATCH v2 mptcp-net 2/2] mptcp: do not reset MP_CAPABLE subflow on mapping errors

[Paolo Abeni]

On Mon, 2021-05-17 at 17:16 -0700, Mat Martineau wrote:
> On Mon, 17 May 2021, Paolo Abeni wrote:
> 
> > When some mapping related errors occours we close the main
> > MPC subflow with a RST. We should instead fallback gracefully
> > to TCP, and do the reset only for MPJ subflows.
> > 
> > Fixes: d22f4988ffec ("mptcp: process MP_CAPABLE data option")
> > Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/192
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> > net/mptcp/subflow.c | 39 +++++++++++++++++++--------------------
> > 1 file changed, 19 insertions(+), 20 deletions(-)
> > 
> > diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> > index 278986585088..9befe9fe7bca 100644
> > --- a/net/mptcp/subflow.c
> > +++ b/net/mptcp/subflow.c
> > @@ -1110,10 +1110,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
> > 
> > 		status = get_mapping_status(ssk, msk);
> > 		trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
> > -		if (unlikely(status == MAPPING_INVALID)) {
> > -			ssk->sk_err = EBADMSG;
> > -			goto fatal;
> > -		}
> > +		if (unlikely(status == MAPPING_INVALID))
> > +			goto fallback;
> > +
> 
> There are a bunch of other ways to get MAPPING_INVALID during the life of 
> a connection, including when there are multiple subflows active and 
> fallback is not a valid option. Can the new fallback condition be more 
> targeted to this "out of order / packet loss at connection time" issue so 
> truly fatal MAPPING_INVALID cases still reset the connection?

What we should do if we get a MAPPING_INVALID and we have a single
(MPC) subflow? I could not find any specific reference in the RFC. I
think it's roughly the same as 'no mapping' at all: if we can fallback
we do fallback, otherwise we reset.

Note that with this patch, under the 'fallback' label, checks if a
reset is needed or not. The current patch does a reset only if the
subflow is an MP_JOIN one, but that condition could be additionally
extended to 'msk has multiple subflows' - even if the latter looks like
a net-next patch.

WDYT?

Thanks!

Paolo

Re: [PATCH v2 mptcp-net 2/2] mptcp: do not reset MP_CAPABLE subflow on mapping errors

[Mat Martineau]

On Tue, 18 May 2021, Paolo Abeni wrote:

> On Mon, 2021-05-17 at 17:16 -0700, Mat Martineau wrote:
>> On Mon, 17 May 2021, Paolo Abeni wrote:
>>
>>> When some mapping related errors occours we close the main
>>> MPC subflow with a RST. We should instead fallback gracefully
>>> to TCP, and do the reset only for MPJ subflows.
>>>
>>> Fixes: d22f4988ffec ("mptcp: process MP_CAPABLE data option")
>>> Closes: https://github.com/multipath-tcp/mptcp_net-next/issues/192
>>> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>>> ---
>>> net/mptcp/subflow.c | 39 +++++++++++++++++++--------------------
>>> 1 file changed, 19 insertions(+), 20 deletions(-)
>>>
>>> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
>>> index 278986585088..9befe9fe7bca 100644
>>> --- a/net/mptcp/subflow.c
>>> +++ b/net/mptcp/subflow.c
>>> @@ -1110,10 +1110,9 @@ static bool subflow_check_data_avail(struct sock *ssk)
>>>
>>> 		status = get_mapping_status(ssk, msk);
>>> 		trace_subflow_check_data_avail(status, skb_peek(&ssk->sk_receive_queue));
>>> -		if (unlikely(status == MAPPING_INVALID)) {
>>> -			ssk->sk_err = EBADMSG;
>>> -			goto fatal;
>>> -		}
>>> +		if (unlikely(status == MAPPING_INVALID))
>>> +			goto fallback;
>>> +
>>
>> There are a bunch of other ways to get MAPPING_INVALID during the life of
>> a connection, including when there are multiple subflows active and
>> fallback is not a valid option. Can the new fallback condition be more
>> targeted to this "out of order / packet loss at connection time" issue so
>> truly fatal MAPPING_INVALID cases still reset the connection?
>
> What we should do if we get a MAPPING_INVALID and we have a single
> (MPC) subflow? I could not find any specific reference in the RFC. I
> think it's roughly the same as 'no mapping' at all: if we can fallback
> we do fallback, otherwise we reset.

Looking at section 3.7, there aren't many cases where fallback is an 
option during operation (after the initial data has been successfully 
acked in each direction).

"If a subflow breaks during operation ... then once this is detected ...
the subflow SHOULD be treated as broken and closed with a RST, since no 
data can be delivered to the application layer and no fallback signal
can be reliably sent."

In general, it looks like a bad mapping should reset the *subflow* (rather 
than the whole MPTCP connection) - what I said above was unclear about 
what exactly should be reset.

It seems like the only way to fall back the whole connection later in its 
life involves checksum failure. In that case the RFC describes the 
conditions where an "infinite mapping" can be used for fallback - but 
that's different from the beginning-of-connection fallback code we have 
today.

>
> Note that with this patch, under the 'fallback' label, checks if a
> reset is needed or not. The current patch does a reset only if the
> subflow is an MP_JOIN one, but that condition could be additionally
> extended to 'msk has multiple subflows' - even if the latter looks like
> a net-next patch.
>
> WDYT?

As long as the subflow is reset when the mapping failed, that's fine. It 
doesn't seem like there are conditions during operation (long after 
connection time) where it's correct to do fallback without an infinite 
mapping.

--
Mat Martineau
Intel