[MPTCP] Re: [PATCH 0/2] Fix bug processing options

[MPTCP] Re: [PATCH 0/2] Fix bug processing options

[Matthieu Baerts]

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

Hi,

It seems this email I wanted to send end of last week didn't go through.

On 26/10/2019 10:04, Matthieu Baerts wrote:
> Hi Peter, Mat,
> 
> On 26/10/2019 01:40, Mat Martineau wrote:
>>
>> On Tue, 22 Oct 2019, Peter Krystad wrote:
>>
>>> Do not process incoming options if the subflow is not MPTCP.
>>>
>>> Observed crash when running against out-of-tree kernel with
>>> mis-matched checksum options.
>>>
>>> Peter Krystad (2):
>>>  mptcp: Ignore incoming MPTCP options when not MP_CAPABLE
>>>  mptcp: Ignore incoming MPTCP options when not MP_CAPABLE
>>>
>>> net/mptcp/options.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>
>> Both changes look good to me. Thanks, Peter!
> 
> Thank you for the patches and the reviews!
> 
> - 6eb607af7475: "squashed" patch 1/2 in "mptcp: Implement MPTCP receive 
> path"
> - ea443b296417: "squashed" patch 2/2 in "mptcp: Add handling of outgoing 
> MP_JOIN requests"
> - 22b6803a0c48..a89f2feb5b90: result
> 
> Tests are still OK!

I had to do a small change to allow the compilation of each commit:


diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index ca589ab9a02a..bbf9e9aa53da 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -391,6 +391,7 @@ bool mptcp_synack_options(const struct request_sock 
*req, unsigned int *size,
  void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb,
                             struct tcp_options_received *opt_rx)
  {
+       struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
         struct mptcp_options_received *mp_opt;
         struct mptcp_ext *mpext;


The subflow variable is declared later on in "mptcp: Add path manager 
interface"

- c559c9eb4f9f: "squashed" in "mptcp: Implement MPTCP receive path"
- 9c984d940c41: conflict in t/mptcp-Add-path-manager-interface
- no diff at the end.

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [PATCH 0/2] Fix bug processing options

[Matthieu Baerts]

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

Hi Peter, Mat,

On 26/10/2019 01:40, Mat Martineau wrote:
> 
> On Tue, 22 Oct 2019, Peter Krystad wrote:
> 
>> Do not process incoming options if the subflow is not MPTCP.
>>
>> Observed crash when running against out-of-tree kernel with
>> mis-matched checksum options.
>>
>> Peter Krystad (2):
>>  mptcp: Ignore incoming MPTCP options when not MP_CAPABLE
>>  mptcp: Ignore incoming MPTCP options when not MP_CAPABLE
>>
>> net/mptcp/options.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
> 
> Both changes look good to me. Thanks, Peter!

Thank you for the patches and the reviews!

- 6eb607af7475: "squashed" patch 1/2 in "mptcp: Implement MPTCP receive 
path"
- ea443b296417: "squashed" patch 2/2 in "mptcp: Add handling of outgoing 
MP_JOIN requests"
- 22b6803a0c48..a89f2feb5b90: result

Tests are still OK!

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium

[MPTCP] Re: [PATCH 0/2] Fix bug processing options

[Mat Martineau]

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


On Tue, 22 Oct 2019, Peter Krystad wrote:

> Do not process incoming options if the subflow is not MPTCP.
>
> Observed crash when running against out-of-tree kernel with
> mis-matched checksum options.
>
> Peter Krystad (2):
>  mptcp: Ignore incoming MPTCP options when not MP_CAPABLE
>  mptcp: Ignore incoming MPTCP options when not MP_CAPABLE
>
> net/mptcp/options.c | 3 +++
> 1 file changed, 3 insertions(+)
>

Both changes look good to me. Thanks, Peter!

--
Mat Martineau
Intel

[MPTCP] Re: [PATCH 0/2] Fix bug processing options

[Peter Krystad]

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

On Wed, 2019-10-23 at 14:19 +0200, Matthieu Baerts wrote:
> Hi Peter,
> 
> On 23/10/2019 00:54, Peter Krystad wrote:
> > Observed crash when running against out-of-tree kernel with
> > mis-matched checksum options.
> 
> Thank you for doing this kind of tests!

It was an accident, I forgot to disable checksum on the out-of-tree side...

> 
>  > Do not process incoming options if the subflow is not MPTCP.
> 
> Are we not supposed not to deal with MPTCP options after a fallback to TCP?
> 
> If we fallback to TCP, should we not always set 'tp->is_mptcp' to false?

Currently for fallback we set is_mptcp=0 after the connection negotiation is
complete [in mptcp_accept()] and presumeably everything is fine after that
point. In this specific situation (we don't send MP_CAPABLE in the third ack
because we don't support checksum) the out-of-tree kernel resends the syn-ack
with DSS options, and these packets are processed before is_mptcp is cleared.
I think we are in uncharted territory here because of how we reject the other
sides request for checksums, but this check can't hurt.

Peter.

 
> 
> Cheers,
> Matt

[MPTCP] Re: [PATCH 0/2] Fix bug processing options

[Matthieu Baerts]

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

Hi Peter,

On 23/10/2019 00:54, Peter Krystad wrote:
> Observed crash when running against out-of-tree kernel with
> mis-matched checksum options.

Thank you for doing this kind of tests!

 > Do not process incoming options if the subflow is not MPTCP.

Are we not supposed not to deal with MPTCP options after a fallback to TCP?

If we fallback to TCP, should we not always set 'tp->is_mptcp' to false?

Cheers,
Matt
-- 
Matthieu Baerts | R&D Engineer
matthieu.baerts(a)tessares.net
Tessares SA | Hybrid Access Solutions
www.tessares.net
1 Avenue Jean Monnet, 1348 Louvain-la-Neuve, Belgium