[MPTCP] Re: [PATCH] mptcp: fix option length of mp_capable syn/ack

[MPTCP] Re: [PATCH] mptcp: fix option length of mp_capable syn/ack

[Matthieu Baerts]

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

Hi Davide,

Thank you for the patch!

On 29/11/2019 20:02, Davide Caratti wrote:

[...]

> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 98eb0281d196..cee5c3968741 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -57,7 +57,7 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize,

Here above, you also need to replace TCPOLEN_MPTCP_MPC_SYNACK by 
TCPOLEN_MPTCP_MPC_ACK:

===
diff --git a/net/mptcp/options.c b/net/mptcp/options.c
index 98eb0281d196..9163fddf16b9 100644
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -25,7 +25,7 @@ void mptcp_parse_option(const unsigned char *ptr, int 
opsize,
          */
         case MPTCPOPT_MP_CAPABLE:
                 if (opsize != TCPOLEN_MPTCP_MPC_SYN &&
-                   opsize != TCPOLEN_MPTCP_MPC_SYNACK)
+                   opsize != TCPOLEN_MPTCP_MPC_ACK)
                         break;

                 mp_opt->version = *ptr++ & MPTCP_VERSION_MASK;
===

(or check that the size is not one of the three: I guess (but to be 
confirmed) that the compiler will remove one check if we compare exactly 
the same thing: opsize != 12 && opsize != 12 && opsize != 20)

Otherwise you will never match the if-statement here below.

Note that this code is very similar to what is done in mptcp.org in 
mptcp_parse_option().

>   		mp_opt->sndr_key = get_unaligned_be64(ptr);
>   		ptr += 8;
>   
> -		if (opsize == TCPOLEN_MPTCP_MPC_SYNACK) {
> +		if (opsize == TCPOLEN_MPTCP_MPC_ACK) {
>   			mp_opt->rcvr_key = get_unaligned_be64(ptr);
>   			ptr += 8;
>   			pr_debug("MP_CAPABLE flags=%x, sndr=%llu, rcvr=%llu",
> @@ -650,8 +650,7 @@ void mptcp_write_options(__be32 *ptr, struct mptcp_out_options *opts)
>   				      MPTCP_CAP_HMAC_SHA1);
>   		put_unaligned_be64(opts->sndr_key, ptr);
>   		ptr += 2;
> -		if ((OPTION_MPTCP_MPC_SYNACK |
> -		     OPTION_MPTCP_MPC_ACK) & opts->suboptions) {
> +		if (OPTION_MPTCP_MPC_ACK & opts->suboptions) {
>   			put_unaligned_be64(opts->rcvr_key, ptr);
>   			ptr += 2;
>   		}
> diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h
> index acad61c70de9..10347c9b4dff 100644
> --- a/net/mptcp/protocol.h
> +++ b/net/mptcp/protocol.h
> @@ -34,7 +34,7 @@
>   
>   /* MPTCP suboption lengths */
>   #define TCPOLEN_MPTCP_MPC_SYN		12
> -#define TCPOLEN_MPTCP_MPC_SYNACK	20
> +#define TCPOLEN_MPTCP_MPC_SYNACK	12

Should we remove it if we don't use it?
Or rename 'TCPOLEN_MPTCP_MPC_SYN' to 'TCPOLEN_MPTCP_MPC_SYN_AND_SYNACK'?

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] mptcp: fix option length of mp_capable syn/ack

[Davide Caratti]

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

On Sat, 2019-11-30 at 09:55 +0100, Matthieu Baerts wrote:
> Hi Davide,

[...]

> > diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> > index 98eb0281d196..cee5c3968741 100644
> > --- a/net/mptcp/options.c
> > +++ b/net/mptcp/options.c
> > @@ -57,7 +57,7 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize,
> 
> Here above, you also need to replace TCPOLEN_MPTCP_MPC_SYNACK by 
> TCPOLEN_MPTCP_MPC_ACK:
> 
> ===
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 98eb0281d196..9163fddf16b9 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -25,7 +25,7 @@ void mptcp_parse_option(const unsigned char *ptr, int 
> opsize,
>           */
>          case MPTCPOPT_MP_CAPABLE:
>                  if (opsize != TCPOLEN_MPTCP_MPC_SYN &&
> -                   opsize != TCPOLEN_MPTCP_MPC_SYNACK)
> +                   opsize != TCPOLEN_MPTCP_MPC_ACK)
>                          break;
> 
>                  mp_opt->version = *ptr++ & MPTCP_VERSION_MASK;
> ===
> 
> (or check that the size is not one of the three: I guess (but to be 
> confirmed) that the compiler will remove one check if we compare exactly 
> the same thing: opsize != 12 && opsize != 12 && opsize != 20)

> Otherwise you will never match the if-statement here below.

you are right, will fix this in v3. 

> --- a/net/mptcp/protocol.h
> > +++ b/net/mptcp/protocol.h
> > @@ -34,7 +34,7 @@
> >   
> >   /* MPTCP suboption lengths */
> >   #define TCPOLEN_MPTCP_MPC_SYN		12
> > -#define TCPOLEN_MPTCP_MPC_SYNACK	20
> > +#define TCPOLEN_MPTCP_MPC_SYNACK	12
> 
> Should we remove it if we don't use it?
> Or rename 'TCPOLEN_MPTCP_MPC_SYN' to 'TCPOLEN_MPTCP_MPC_SYN_AND_SYNACK'?

yes, it can be removed - but it's going to return very soon, with the same
value, with v1 protocol. I'm for preserving the testability with
tcpdump/packetdrill in v0 changing the lowest possible number of lines.

-- 
davide