[PATCH mptcp-net v2 0/2] mptcp: ADD_ADDR and fully_estab

[PATCH mptcp-net v2 0/2] mptcp: ADD_ADDR and fully_estab

[Matthieu Baerts]

This is the v2 of "mptcp: full fully established support after ADD_ADDR"
but now we also make sure the HMAC bit is present (patch 1/2, as
suggested by Mat, thanks!).

Matthieu Baerts (2):
  mptcp: check for HMAC presence in ADD_ADDR
  mptcp: full fully established support after ADD_ADDR

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

-- 
2.32.0

[PATCH mptcp-net v2 1/2] mptcp: check for HMAC presence in ADD_ADDR

[Matthieu Baerts]

Before switching to a "fully established" mode, which confirms the other
end seems to really talk MPTCP, best to check if the ADD_ADDR looks
valid by looking if it contains an HMAC (no 'echo' bit).

If an ADD_ADDR echo is received while we were not in "fully established"
mode, it is strange, best not to switch now.

Fixes: 84dfe3677a6f ("mptcp: send out dedicated ADD_ADDR packet")
Suggested-by: Mat Martineau <mathew.j.martineau@linux.intel.com>
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---
 net/mptcp/options.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index d94ff50c29b3..a83550de54db 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -954,7 +954,7 @@ static bool check_fully_established(struct mptcp_sock *msk, struct sock *ssk,
 		goto fully_established;
 	}
 
-	if (mp_opt->add_addr) {
+	if (mp_opt->add_addr && !mp_opt->echo) {
 		WRITE_ONCE(msk->fully_established, true);
 		return true;
 	}
-- 
2.32.0

[PATCH mptcp-net v2 2/2] mptcp: full fully established support after ADD_ADDR

[Matthieu Baerts]

If directly after an MP_CAPABLE 3WHS, the client receives an ADD_ADDR
with HMAC from the server, it is enough to switch to a "fully
established" mode because it has received more MPTCP options.

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")
Signed-off-by: Matthieu Baerts <matthieu.baerts@tessares.net>
---

Notes:
    - v2: reword the commit message not to mention "valid" content (Mat)

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

diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index a83550de54db..bec3ed82e253 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -945,20 +945,16 @@ 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 && !mp_opt->echo)) {
 		/* 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 && !mp_opt->echo) {
-		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-net v2 0/2] mptcp: ADD_ADDR and fully_estab

[Mat Martineau]

On Mon, 16 Aug 2021, Matthieu Baerts wrote:

> This is the v2 of "mptcp: full fully established support after ADD_ADDR"
> but now we also make sure the HMAC bit is present (patch 1/2, as
> suggested by Mat, thanks!).
>
> Matthieu Baerts (2):
>  mptcp: check for HMAC presence in ADD_ADDR
>  mptcp: full fully established support after ADD_ADDR
>
> net/mptcp/options.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> -- 
> 2.32.0

Thanks for the v2.

I'd prefer that these are squashed, I don't think it's important to change 
the single line in patch 1 that just gets deleted in patch 2. It wasn't my 
intent to ask for two patches in my previous review, if that was a factor.

Reviewed-by: Mat Martineau <mathew.j.martineau@linux.intel.com>

--
Mat Martineau
Intel

Re: [PATCH mptcp-net v2 0/2] mptcp: ADD_ADDR and fully_estab

[Matthieu Baerts]

Hi Mat,

On 17/08/2021 01:53, Mat Martineau wrote:
> On Mon, 16 Aug 2021, Matthieu Baerts wrote:
> 
>> This is the v2 of "mptcp: full fully established support after ADD_ADDR"
>> but now we also make sure the HMAC bit is present (patch 1/2, as
>> suggested by Mat, thanks!).
>>
>> Matthieu Baerts (2):
>>  mptcp: check for HMAC presence in ADD_ADDR
>>  mptcp: full fully established support after ADD_ADDR
>>
>> net/mptcp/options.c | 10 +++-------
>> 1 file changed, 3 insertions(+), 7 deletions(-)
>>
>> -- 
>> 2.32.0
> 
> Thanks for the v2.

Thank you for the review!

> I'd prefer that these are squashed, I don't think it's important to
> change the single line in patch 1 that just gets deleted in patch 2. It
> wasn't my intent to ask for two patches in my previous review, if that
> was a factor.

Sure I can squash them. I initially had one single patch but I decided
to split it in two to avoid adding something like "oh and BTW, an
additional check has been added" in the commit message.
But fine to do that.

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

Re: [PATCH mptcp-net v2 0/2] mptcp: ADD_ADDR and fully_estab

[Paolo Abeni]

On Tue, 2021-08-17 at 08:48 +0200, Matthieu Baerts wrote:
> Hi Mat,
> 
> On 17/08/2021 01:53, Mat Martineau wrote:
> > On Mon, 16 Aug 2021, Matthieu Baerts wrote:
> > 
> > > This is the v2 of "mptcp: full fully established support after ADD_ADDR"
> > > but now we also make sure the HMAC bit is present (patch 1/2, as
> > > suggested by Mat, thanks!).
> > > 
> > > Matthieu Baerts (2):
> > >  mptcp: check for HMAC presence in ADD_ADDR
> > >  mptcp: full fully established support after ADD_ADDR
> > > 
> > > net/mptcp/options.c | 10 +++-------
> > > 1 file changed, 3 insertions(+), 7 deletions(-)
> > > 
> > > -- 
> > > 2.32.0
> > 
> > Thanks for the v2.
> 
> Thank you for the review!
> 
> > I'd prefer that these are squashed, I don't think it's important to
> > change the single line in patch 1 that just gets deleted in patch 2. It
> > wasn't my intent to ask for two patches in my previous review, if that
> > was a factor.
> 
> Sure I can squash them. I initially had one single patch but I decided
> to split it in two to avoid adding something like "oh and BTW, an
> additional check has been added" in the commit message.
> But fine to do that.

+1 on squashing - if not already done ;)

Cheers,

Paolo