[PATCH mptcp-next] mptcp: full fully established support after ADD_ADDR

[PATCH mptcp-next] mptcp: full fully established support after ADD_ADDR

[Matthieu Baerts]

If directly after an MP_CAPABLE 3WHS, the client receives a valid
ADD_ADDR from the server, it is enough to prove exchanged MPTCP options
are valid. Indeed, the ADD_ADDR contains an HMAC generated using both
the client and server keys.

It was then OK to enable the fully_established flag on the MPTCP socket.

But that is not enough. On one hand, the path-manager has be notified
the state has changed. On the other hand, the fully_established flag on
the subflow socket should be turned on as well, not to re-send the
MP_CAPABLE 3rd ACK content with the next ACK.

Fixes: 84dfe3677a6f ("mptcp: send out dedicated ADD_ADDR packet")
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---

Notes:
    Once applied, we can close issue 221. But this patch alone is not enough
    to fix issue 221, hence no "Closes" tag.
    
    Because the PM was not notified, I think it is best to send it to -net.
    Before, I think we were overriding MPC ACK options when sending the echo
    ADD_ADDR so the issue was not visible.
    
    Here it is visible (and this is covered by out tests) since the
    introduction of commit 32c3e4d50806 ("Squash-to: "mptcp: move
    drop_other_suboptions check under pm lock"")
    
    With this patch, the packetdrill tests are all OK!
    (And the other selftests as well even if I still have other issues with
     mptcp_join.sh but it doesn't seem related.)

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

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index d94ff50c29b3..175563c95c0b 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -945,20 +945,15 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 		return subflow->mp_capable;
 	}
 
-	if (mp_opt->dss && mp_opt->use_ack) {
+	if ((mp_opt->dss && mp_opt->use_ack) || mp_opt->add_addr) {
 		/* subflows are fully established as soon as we get any
-		 * additional ack.
+		 * additional ack, including ADD_ADDR.
 		 */
 		subflow->fully_established = 1;
 		WRITE_ONCE(msk->fully_established, true);
 		goto fully_established;
 	}
 
-	if (mp_opt->add_addr) {
-		WRITE_ONCE(msk->fully_established, true);
-		return true;
-	}
-
 	/* If the first established packet does not contain MP_CAPABLE + data
 	 * then fallback to TCP. Fallback scenarios requires a reset for
 	 * MP_JOIN subflows.
-- 
2.32.0

Re: [PATCH mptcp-next] mptcp: full fully established support after ADD_ADDR

[Paolo Abeni]

On Thu, 2021-08-12 at 18:27 +0200, Matthieu Baerts wrote:
> If directly after an MP_CAPABLE 3WHS, the client receives a valid
> ADD_ADDR from the server, it is enough to prove exchanged MPTCP options
> are valid. Indeed, the ADD_ADDR contains an HMAC generated using both
> the client and server keys.
> 
> It was then OK to enable the fully_established flag on the MPTCP socket.
> 
> But that is not enough. On one hand, the path-manager has be notified
> the state has changed. On the other hand, the fully_established flag on
> the subflow socket should be turned on as well, not to re-send the
> MP_CAPABLE 3rd ACK content with the next ACK.
> 
> Fixes: 84dfe3677a6f ("mptcp: send out dedicated ADD_ADDR packet")
> Suggested-by: Paolo Abeni <pabeni@redhat.com>

I guess you can drop this tag, as you come-up to this patch by yourself
;)

LGTM, thanks!

Paolo

> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
> 
> Notes:
>     Once applied, we can close issue 221. But this patch alone is not enough
>     to fix issue 221, hence no "Closes" tag.
>     
>     Because the PM was not notified, I think it is best to send it to -net.
>     Before, I think we were overriding MPC ACK options when sending the echo
>     ADD_ADDR so the issue was not visible.

Ack!

/P

Re: [PATCH mptcp-next] mptcp: full fully established support after ADD_ADDR

[Mat Martineau]

On Thu, 12 Aug 2021, Matthieu Baerts wrote:

> If directly after an MP_CAPABLE 3WHS, the client receives a valid
> ADD_ADDR from the server, it is enough to prove exchanged MPTCP options
> are valid. Indeed, the ADD_ADDR contains an HMAC generated using both
> the client and server keys.

The ADD_ADDR HMAC is validated after the call to 
check_fully_established(), so I'm not sure this condition is fully met 
with the patch as-is. Looks like an ADD_ADDR with a bad HMAC could change 
the fully_established flag when it shouldn't.

Mat

>
> It was then OK to enable the fully_established flag on the MPTCP socket.
>
> But that is not enough. On one hand, the path-manager has be notified
> the state has changed. On the other hand, the fully_established flag on
> the subflow socket should be turned on as well, not to re-send the
> MP_CAPABLE 3rd ACK content with the next ACK.
>
> Fixes: 84dfe3677a6f ("mptcp: send out dedicated ADD_ADDR packet")
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
> ---
>
> Notes:
>    Once applied, we can close issue 221. But this patch alone is not enough
>    to fix issue 221, hence no "Closes" tag.
>
>    Because the PM was not notified, I think it is best to send it to -net.
>    Before, I think we were overriding MPC ACK options when sending the echo
>    ADD_ADDR so the issue was not visible.
>
>    Here it is visible (and this is covered by out tests) since the
>    introduction of commit 32c3e4d50806 ("Squash-to: "mptcp: move
>    drop_other_suboptions check under pm lock"")
>
>    With this patch, the packetdrill tests are all OK!
>    (And the other selftests as well even if I still have other issues with
>     mptcp_join.sh but it doesn't seem related.)
>
> net/mptcp/options.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index d94ff50c29b3..175563c95c0b 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -945,20 +945,15 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
> 		return subflow->mp_capable;
> 	}
>
> -	if (mp_opt->dss && mp_opt->use_ack) {
> +	if ((mp_opt->dss && mp_opt->use_ack) || mp_opt->add_addr) {
> 		/* subflows are fully established as soon as we get any
> -		 * additional ack.
> +		 * additional ack, including ADD_ADDR.
> 		 */
> 		subflow->fully_established = 1;
> 		WRITE_ONCE(msk->fully_established, true);
> 		goto fully_established;
> 	}
>
> -	if (mp_opt->add_addr) {
> -		WRITE_ONCE(msk->fully_established, true);
> -		return true;
> -	}
> -
> 	/* If the first established packet does not contain MP_CAPABLE + data
> 	 * then fallback to TCP. Fallback scenarios requires a reset for
> 	 * MP_JOIN subflows.
> -- 
> 2.32.0
>
>
>

--
Mat Martineau
Intel

Re: [PATCH mptcp-next] mptcp: full fully established support after ADD_ADDR

[Matthieu Baerts]

Hi Mat,

Thank you for your review!

On 13/08/2021 00:30, Mat Martineau wrote:
> On Thu, 12 Aug 2021, Matthieu Baerts wrote:
> 
>> If directly after an MP_CAPABLE 3WHS, the client receives a valid
>> ADD_ADDR from the server, it is enough to prove exchanged MPTCP options
>> are valid. Indeed, the ADD_ADDR contains an HMAC generated using both
>> the client and server keys.
> 
> The ADD_ADDR HMAC is validated after the call to
> check_fully_established(), so I'm not sure this condition is fully met
> with the patch as-is. Looks like an ADD_ADDR with a bad HMAC could
> change the fully_established flag when it shouldn't.
That's a very good point.

If I'm not mistaken, we have the same issue when receiving a DATA_ACK:
here we just check if one is present, not if it is valid, no?

Should we delay when we mark subflows and msks as "fully established" or
is it not an issue to be in "fully established" too soon?

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

Re: [PATCH mptcp-next] mptcp: full fully established support after ADD_ADDR

[Paolo Abeni]

On Fri, 2021-08-13 at 13:06 +0200, Matthieu Baerts wrote:
> Hi Mat,
> 
> Thank you for your review!
> 
> On 13/08/2021 00:30, Mat Martineau wrote:
> > On Thu, 12 Aug 2021, Matthieu Baerts wrote:
> > 
> > > If directly after an MP_CAPABLE 3WHS, the client receives a valid
> > > ADD_ADDR from the server, it is enough to prove exchanged MPTCP options
> > > are valid. Indeed, the ADD_ADDR contains an HMAC generated using both
> > > the client and server keys.
> > 
> > The ADD_ADDR HMAC is validated after the call to
> > check_fully_established(), so I'm not sure this condition is fully met
> > with the patch as-is. Looks like an ADD_ADDR with a bad HMAC could
> > change the fully_established flag when it shouldn't.
> That's a very good point.
> 
> If I'm not mistaken, we have the same issue when receiving a DATA_ACK:
> here we just check if one is present, not if it is valid, no?
> 
> Should we delay when we mark subflows and msks as "fully established" or
> is it not an issue to be in "fully established" too soon?

I *think* we could/should start with this simpler implementation/fix -
eventually adding a comment explaining the problematic scenario.

A more correct state handling requires more extensive changes, likely
too invasive for a resonably timely fix.

Cheers,

Paolo

Re: [PATCH mptcp-next] mptcp: full fully established support after ADD_ADDR

[Mat Martineau]

On Fri, 13 Aug 2021, Paolo Abeni wrote:

> On Fri, 2021-08-13 at 13:06 +0200, Matthieu Baerts wrote:
>> Hi Mat,
>>
>> Thank you for your review!
>>
>> On 13/08/2021 00:30, Mat Martineau wrote:
>>> On Thu, 12 Aug 2021, Matthieu Baerts wrote:
>>>
>>>> If directly after an MP_CAPABLE 3WHS, the client receives a valid
>>>> ADD_ADDR from the server, it is enough to prove exchanged MPTCP options
>>>> are valid. Indeed, the ADD_ADDR contains an HMAC generated using both
>>>> the client and server keys.
>>>
>>> The ADD_ADDR HMAC is validated after the call to
>>> check_fully_established(), so I'm not sure this condition is fully met
>>> with the patch as-is. Looks like an ADD_ADDR with a bad HMAC could
>>> change the fully_established flag when it shouldn't.
>> That's a very good point.
>>
>> If I'm not mistaken, we have the same issue when receiving a DATA_ACK:
>> here we just check if one is present, not if it is valid, no?
>>

I see Matthieu's point about validity vs. presence when using the ADD_ADDR 
here. Maybe we had different ideas in our minds with "valid" meaning "a 
properly formatted ADD_ADDR option w/ HMAC" vs. "an ADD_ADDR with 
validated HMAC".

An invalid HMAC would be like an out-of-window DATA_ACK, so we're not 
creating a new problem if the connection becomes "fully established" due 
to an ADD_ADDR with a bad HMAC.


So, if only the presence of the HMAC is important:

* the patch should check for (mp_opt->add_addr && !mp_opt->echo) to ensure 
the HMAC is present

* commit message and code comments should refer to ADD_ADDR w/ HMAC being 
"present" rather than "valid"?


>> Should we delay when we mark subflows and msks as "fully established" or
>> is it not an issue to be in "fully established" too soon?
>
> I *think* we could/should start with this simpler implementation/fix -
> eventually adding a comment explaining the problematic scenario.
>

I basically agree, with the tweaks above.

> A more correct state handling requires more extensive changes, likely
> too invasive for a resonably timely fix.

We could call add_addr_hmac_valid() from check_fully_established() to do 
an extra HMAC validation *if* it offers any benefit - extra CPU cycles but 
a very small addition to the code.


Thanks,

--
Mat Martineau
Intel