[PATCH mptcp-next 0/2] send MP_FAIL with MP_RST and others

[PATCH mptcp-next 0/2] send MP_FAIL with MP_RST and others

[Geliang Tang]

Two small patches about sending MP_FAIL with MP_RST.

Geliang Tang (2):
  mptcp: send MP_FAIL with MP_RST
  mptcp: print out reset_reason of MP_RST

 net/mptcp/options.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

-- 
2.26.2

[PATCH mptcp-next 1/2] mptcp: send MP_FAIL with MP_RST

[Geliang Tang]

As described in RFC8684, 3.7 Fallback:

'''
Therefore, it is not possible to recover the subflow, and the affected
subflow must be immediately closed with a RST that includes an MP_FAIL
option (Figure 16), which defines the data sequence number at the start
of the segment (defined by the Data Sequence Mapping) that had the
checksum failure.
'''

MP_FAIL could be sent with MP_RST at the same time.

This patch fixed it.

Fixes: c3b546f7292ac ("mptcp: implement fastclose xmit path")
Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/options.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 8a1020e4285c..dc998a14f4cb 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -829,8 +829,11 @@ bool mptcp_established_options(struct sock *sk, struct sk_buff *skb,
 
 	if (unlikely(skb && TCP_SKB_CB(skb)->tcp_flags & TCPHDR_RST)) {
 		if (mptcp_established_options_fastclose(sk, &opt_size, remaining, opts) ||
-		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts) ||
-		    mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
+		    mptcp_established_options_mp_fail(sk, &opt_size, remaining, opts)) {
+			*size += opt_size;
+			remaining -= opt_size;
+		}
+		if (mptcp_established_options_rst(sk, skb, &opt_size, remaining, opts)) {
 			*size += opt_size;
 			remaining -= opt_size;
 		}
-- 
2.26.2

Re: [PATCH mptcp-next 1/2] mptcp: send MP_FAIL with MP_RST

[Matthieu Baerts]

Hi Geliang,

+cc: Paolo

On 23/11/2021 02:23, Geliang Tang wrote:
> As described in RFC8684, 3.7 Fallback:
> 
> '''
> Therefore, it is not possible to recover the subflow, and the affected
> subflow must be immediately closed with a RST that includes an MP_FAIL
> option (Figure 16), which defines the data sequence number at the start
> of the segment (defined by the Data Sequence Mapping) that had the
> checksum failure.
> '''
> 
> MP_FAIL could be sent with MP_RST at the same time.
> 
> This patch fixed it.
> 
> Fixes: c3b546f7292ac ("mptcp: implement fastclose xmit path")

I bugged a bit when looking at your patch because I was not able to see
the issue on -net nor net-next. But then I noticed this patch is in fact
a Squash-to patch :)

Then good catch to have seen that before upstreaming these patches!

But then, I'm not sure this modification is enough: in
mptcp_write_options(), it looks like MP_FASTCLOSE and MP_RST are
mutually exclusive, no?

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next 1/2] mptcp: send MP_FAIL with MP_RST

[Geliang Tang]

On Sat, Nov 27, 2021 at 11:03:50AM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> +cc: Paolo
> 
> On 23/11/2021 02:23, Geliang Tang wrote:
> > As described in RFC8684, 3.7 Fallback:
> > 
> > '''
> > Therefore, it is not possible to recover the subflow, and the affected
> > subflow must be immediately closed with a RST that includes an MP_FAIL
> > option (Figure 16), which defines the data sequence number at the start
> > of the segment (defined by the Data Sequence Mapping) that had the
> > checksum failure.
> > '''
> > 
> > MP_FAIL could be sent with MP_RST at the same time.
> > 
> > This patch fixed it.
> > 
> > Fixes: c3b546f7292ac ("mptcp: implement fastclose xmit path")
> 
> I bugged a bit when looking at your patch because I was not able to see
> the issue on -net nor net-next. But then I noticed this patch is in fact
> a Squash-to patch :)
> 
> Then good catch to have seen that before upstreaming these patches!

Updated in v2 as a squash-to patch.

> 
> But then, I'm not sure this modification is enough: in
> mptcp_write_options(), it looks like MP_FASTCLOSE and MP_RST are
> mutually exclusive, no?

MP_FAIL can be sent with MP_RST in mptcp_write_options(). So no need to
modify the code in mptcp_write_options().

Thanks,
-Geliang

> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
> 

Re: [PATCH mptcp-next 1/2] mptcp: send MP_FAIL with MP_RST

[Matthieu Baerts]

Hi Geliang,

29 Nov 2021 05:12:36 Geliang Tang <geliang.tang@suse.com>:

> On Sat, Nov 27, 2021 at 11:03:50AM +0100, Matthieu Baerts wrote:
>> Hi Geliang,
>>
>> +cc: Paolo
>>
>> On 23/11/2021 02:23, Geliang Tang wrote:
>>> As described in RFC8684, 3.7 Fallback:
>>>
>>> '''
>>> Therefore, it is not possible to recover the subflow, and the affected
>>> subflow must be immediately closed with a RST that includes an MP_FAIL
>>> option (Figure 16), which defines the data sequence number at the start
>>> of the segment (defined by the Data Sequence Mapping) that had the
>>> checksum failure.
>>> '''
>>>
>>> MP_FAIL could be sent with MP_RST at the same time.
>>>
>>> This patch fixed it.
>>>
>>> Fixes: c3b546f7292ac ("mptcp: implement fastclose xmit path")
>>
>> I bugged a bit when looking at your patch because I was not able to see
>> the issue on -net nor net-next. But then I noticed this patch is in fact
>> a Squash-to patch :)
>>
>> Then good catch to have seen that before upstreaming these patches!
>
> Updated in v2 as a squash-to patch.

Thanks!

>> But then, I'm not sure this modification is enough: in
>> mptcp_write_options(), it looks like MP_FASTCLOSE and MP_RST are
>> mutually exclusive, no?
>
> MP_FAIL can be sent with MP_RST in mptcp_write_options(). So no need to
> modify the code in mptcp_write_options().

If I'm not mistaken, with your patch, you allow MP_RST to be sent with MP_FAIL and MP_FASTCLOSE but MP_FAIL cannot be sent with a MP_FASTCLOSE.

In mptcp_write_options(), we don't do the same: MP_FAIL can be sent with MP_FASTCLOSE and MP_RST but MP_FASTCLOSE cannot be sent with MP_RST, no?

Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next 1/2] mptcp: send MP_FAIL with MP_RST

[Matthieu Baerts]

29 Nov 2021 07:47:59 Matthieu Baerts <matthieu.baerts@tessares.net>:

> Hi Geliang,
>
> 29 Nov 2021 05:12:36 Geliang Tang <geliang.tang@suse.com>:
>
>> On Sat, Nov 27, 2021 at 11:03:50AM +0100, Matthieu Baerts wrote:
>>> Hi Geliang,
>>>
>>> +cc: Paolo
>>>
>>> On 23/11/2021 02:23, Geliang Tang wrote:
>>>> As described in RFC8684, 3.7 Fallback:
>>>>
>>>> '''
>>>> Therefore, it is not possible to recover the subflow, and the affected
>>>> subflow must be immediately closed with a RST that includes an MP_FAIL
>>>> option (Figure 16), which defines the data sequence number at the start
>>>> of the segment (defined by the Data Sequence Mapping) that had the
>>>> checksum failure.
>>>> '''
>>>>
>>>> MP_FAIL could be sent with MP_RST at the same time.
>>>>
>>>> This patch fixed it.
>>>>
>>>> Fixes: c3b546f7292ac ("mptcp: implement fastclose xmit path")
>>>
>>> I bugged a bit when looking at your patch because I was not able to see
>>> the issue on -net nor net-next. But then I noticed this patch is in fact
>>> a Squash-to patch :)
>>>
>>> Then good catch to have seen that before upstreaming these patches!
>>
>> Updated in v2 as a squash-to patch.
>
> Thanks!
>
>>> But then, I'm not sure this modification is enough: in
>>> mptcp_write_options(), it looks like MP_FASTCLOSE and MP_RST are
>>> mutually exclusive, no?
>>
>> MP_FAIL can be sent with MP_RST in mptcp_write_options(). So no need to
>> modify the code in mptcp_write_options().
>
> If I'm not mistaken, with your patch, you allow MP_RST to be sent with MP_FAIL and MP_FASTCLOSE but MP_FAIL cannot be sent with a MP_FASTCLOSE.
>
> In mptcp_write_options(), we don't do the same: MP_FAIL can be sent with MP_FASTCLOSE and MP_RST but MP_FASTCLOSE cannot be sent with MP_RST, no?

It would be good to have a regression test making sure we have this behaviour and we don't change it but mistake. I think packetdrill would be very helpful for that :)

Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

[PATCH mptcp-next 2/2] mptcp: print out reset_reason of MP_RST

[Geliang Tang]

This patch printed out the reset_reason of MP_RST in mptcp_parse_option()
to show that MP_RST is received.

Signed-off-by: Geliang Tang <geliang.tang@suse.com>
---
 net/mptcp/options.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index dc998a14f4cb..085d11666fd6 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -336,6 +336,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
 		flags = *ptr++;
 		mp_opt->reset_transient = flags & MPTCP_RST_TRANSIENT;
 		mp_opt->reset_reason = *ptr;
+		pr_debug("MP_RST: reset_reason=%u", mp_opt->reset_reason);
 		break;
 
 	case MPTCPOPT_MP_FAIL:
-- 
2.26.2

Re: [PATCH mptcp-next 2/2] mptcp: print out reset_reason of MP_RST

[Matthieu Baerts]

Hi Geliang,

On 23/11/2021 02:23, Geliang Tang wrote:
> This patch printed out the reset_reason of MP_RST in mptcp_parse_option()
> to show that MP_RST is received.

Thank you for looking at that!

> Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> ---
>  net/mptcp/options.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index dc998a14f4cb..085d11666fd6 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -336,6 +336,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
>  		flags = *ptr++;
>  		mp_opt->reset_transient = flags & MPTCP_RST_TRANSIENT;
>  		mp_opt->reset_reason = *ptr;
> +		pr_debug("MP_RST: reset_reason=%u", mp_opt->reset_reason);

Should we also print the "transient" bit?

And should we also squash this in "mptcp: implement fastclose xmit path"
patch?

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: [PATCH mptcp-next 2/2] mptcp: print out reset_reason of MP_RST

[Geliang Tang]

Hi Matt,

Thanks for your review.

On Sat, Nov 27, 2021 at 11:04:38AM +0100, Matthieu Baerts wrote:
> Hi Geliang,
> 
> On 23/11/2021 02:23, Geliang Tang wrote:
> > This patch printed out the reset_reason of MP_RST in mptcp_parse_option()
> > to show that MP_RST is received.
> 
> Thank you for looking at that!
> 
> > Signed-off-by: Geliang Tang <geliang.tang@suse.com>
> > ---
> >  net/mptcp/options.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index dc998a14f4cb..085d11666fd6 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -336,6 +336,7 @@ static void mptcp_parse_option(const struct sk_buff *skb,
> >  		flags = *ptr++;
> >  		mp_opt->reset_transient = flags & MPTCP_RST_TRANSIENT;
> >  		mp_opt->reset_reason = *ptr;
> > +		pr_debug("MP_RST: reset_reason=%u", mp_opt->reset_reason);
> 
> Should we also print the "transient" bit?
> 

Updated in v2.

> And should we also squash this in "mptcp: implement fastclose xmit path"
> patch?

No, I think it's better to keep it as a standalone patch.

Thanks,
-Geliang

> 
> Cheers,
> Matt
> -- 
> Tessares | Belgium | Hybrid Access Solutions
> www.tessares.net
>