Re: [MPTCP][PATCH v2 mptcp-next 3/5] mptcp: send out MP_FAIL when data checksum fail

[Mat Martineau <mathew.j.martineau@linux.intel.com>]

On Thu, 24 Jun 2021, Geliang Tang wrote:

> When a bad checksum is detected, send out the MP_FAIL suboption.
>
> Signed-off-by: Geliang Tang <geliangtang@gmail.com>
> ---
> net/mptcp/subflow.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c
> index 0b5d4a3eadcd..13e53a6bd3a1 100644
> --- a/net/mptcp/subflow.c
> +++ b/net/mptcp/subflow.c
> @@ -913,6 +913,7 @@ static enum mapping_status validate_data_csum(struct sock *ssk, struct sk_buff *
> 	csum = csum_partial(&header, sizeof(header), subflow->map_data_csum);
> 	if (unlikely(csum_fold(csum))) {
> 		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_DATACSUMERR);
> +		subflow->send_mp_fail = 1;
> 		return subflow->mp_join ? MAPPING_INVALID : MAPPING_DUMMY;
> 	}
>
> @@ -1159,6 +1160,20 @@ static bool subflow_check_data_avail(struct sock *ssk)
> 	return false;
>
> fallback:
> +	if (subflow->send_mp_fail) {
> +		if (mptcp_has_another_subflow_established(ssk)) {

mptcp_has_another_subflow_established() can return false if there's 
another subflow that's not fully established yet. It's not clear what 
happens to such a join-in-progress subflow in the 'else' case below.

Probably safest to only do the "single subflow fallback" case when there 
are no other subflows at all.

> +			mptcp_subflow_reset(ssk);
> +			while ((skb = skb_peek(&ssk->sk_receive_queue)))
> +				sk_eat_skb(ssk, skb);
> +		} else {
> +			pr_fallback(msk);
> +			__mptcp_do_fallback(msk);
> +		}
> +
> +		WRITE_ONCE(subflow->data_avail, 0);
> +		return true;
> +	}
> +

Existing fallback code has only been implemented for not-fully-established 
connections, when the process is much simpler. This checksum-triggered 
MP_FAIL case could happen when more data is in-flight or queued in the 
MPTCP socket (ooo queue).

The RFC uses an "infinite mapping" to fallback after a checksum failure, 
which isn't implemented yet. The MP_FAIL section of the RFC has several 
requirements around this different type of fallback, including a sender 
knowing that all in-flight data is contiguous, and other requirements for 
clearing send buffers with subflow ACKs. This is probably the biggest 
task for MP_FAIL.

> 	/* RFC 8684 section 3.7. */
> 	if (subflow->mp_join || subflow->fully_established) {
> 		/* fatal protocol error, close the socket.
> -- 
> 2.31.1

Thanks,

--
Mat Martineau
Intel