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

[Paolo Abeni <pabeni@redhat.com>]

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