[MPTCP] Re: [PATCH RFC net-next 1/2] net: mptcp: improve fallback to TCP

[Davide Caratti <dcaratti at redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3342 bytes --]

hello Mat, thanks for looking at this.

On Mon, 2020-05-18 at 17:14 -0700, Mat Martineau wrote:
> On Mon, 18 May 2020, Davide Caratti wrote:
> 
> > current MPTCP resets the connection in case the DSS option is not received
> > after a successful three-way handshake, while RFC8684 §3.7 suggests a
> > fall-back to regular TCP when only the first subflow has been estabilshed.
> > Introduce support for "infinite mapping" to allow continuing without DSS in
> > these cases.
> 
> If I'm understanding the intent of the changes, the idea in this patch is 
> to keep using the MPTCP segmentation & reassembly after fallback, but to 
> short-circuit most of it by (1) not writing the DSS option to the TCP 
> header on the tx side and (2) generating dummy mappings on the rx side.

on a second thought, it would have been better to split this and the
support for infinite maps into two separate patches.

Initially I cleared "is_mptcp" on reception of an infinite map, but then
those "goto fallback" were creating so many troubles that the approach was
flipped to always use msk, also for fallback sockets (unless the ones
passing through subflow_ulp_fallback()).


> I'd also like to avoid overloading the term "infinite mapping", which has 
> specific meaning in the RFC: a DSS mapping with the data-level length set 
> to 0 that is sent under specific circumstances. If that specific DSS 
> mapping isn't present on the wire, maybe "dummy mapping" or "fake mapping" 
> is a useful term to use instead.

good point. Looking at the specifications again, not only the kernel must
be able to process an incoming infinite map; it must also emit an infinite
map, tentatively, in case it detects a middlebox that filters/corrupts the
DSS sub-option.

[...]
> > 	}
> > @@ -639,8 +645,12 @@ static enum mapping_status get_mapping_status(struct sock *ssk)
> > 
> > 	data_len = mpext->data_len;
> > 	if (data_len == 0) {
> > -		pr_err("Infinite mapping not handled");
> > +		/* XXX TODO this should be allowed only if we didn't
> > +		 * receive a DACK yet. Paolo suggests to use a dedicated
> > +		 * flag in msk, but can't we use msk->can_ack? */
> 
> The RFC has a lot of requirements for allowing an infinite mapping, and I 
> don't think they align perfectly with what msk->can_ack means. A dedicated 
> "allow_fallback" flag could be set to 'false' when fallback is no longer 
> allowed: a subflow is added, out-of-order MPTCP mappings are encountered, 
> etc.

sure, and also talking with Paolo "can_ack" didn't really fit, as it means
"can send ack because have both local and remote keys". I was for using
subflow->conn_finished (and for removing all XXX stuff before posting to a
ML :) ), but also this is not fitting well.

> The way I read the RFC (and I could be misreading), there are cases where 
> an infinite mapping can be used after DATA_ACK if other conditions are 
> met (like the checksum error section in 3.7).

let's see if I can add a scenario for this (in addition to those at 
https://github.com/dcaratti/packetdrill/tree/fallback-after3whs )

> > 		MPTCP_INC_STATS(sock_net(ssk), MPTCP_MIB_INFINITEMAPRX);
> > +		if (!subflow->local_id && !READ_ONCE(msk->pm.subflows))
> > +			return MAPPING_INFINITE;
> > 		return MAPPING_INVALID;
> > 	}

-- 
davide