Re: [PATCH 1/2] mptcp: parse and emit MP_CAPABLE option according to v1 spec.

On Thu, 2019-11-07 at 07:28 -0800, Christoph Paasch wrote:
> -#define TCPOLEN_MPTCP_MPC_SYNACK       20
> +#define TCPOLEN_MPTCP_MPC_SYN          4
> +#define TCPOLEN_MPTCP_MPC_SYNACK       12
>  #define TCPOLEN_MPTCP_MPC_ACK          20

hello Christoph and Paolo,

the length of the 'mp_capable' option in the SYN/ACK doesn't really change
when moving from v0 to v1 version of the MPTCP protocol.

Current mptcp net-next echoes the client's key in the SYN/ACK but I think
this is not intentional (at least, it does not seem to adhere to section
3.1 of RFC https://tools.ietf.org/html/rfc6824).

For the record, this is why tcpdump refuses to dissect captures of
selftests:

 # ./mptcp_connect.sh -4 -c -e

 IP 10.0.1.1.49752 > 10.0.1.1.10000: Flags [S], seq 2594718977, win 65535, options [mss 65495,sackOK,TS val 3944347269 ecr 0,nop,wscale 8,mptcp capable {0xf931b304deb39a42}], length 0
 IP 10.0.1.1.10000 > 10.0.1.1.49752: Flags [S.], seq 3996234497, ack 2594718978, win 65535, options [mss 65495,sackOK,TS val 3944347270 ecr 3944347269,nop,wscale 8,mptcp capable[bad opt]>
^^^ bad option
 IP 10.0.1.1.49752 > 10.0.1.1.10000: Flags [.], ack 1, win 256, options [nop,nop,TS val 3944347270 ecr 3944347270,mptcp capable {0xf931b304deb39a42,0x96614f185e633ce}], length 0

and also, it's the reason why packetdrill never stores the server key in
its variables, see:

https://github.com/dcaratti/packetdrill/issues/1#issuecomment-559569544

is it possible to fix size of the syn/ack generated by v0 version of the
protocol as a separate patch? Otherwise I will need to add a special
handling for SYN/ACK packets generated by v0 net-next kernels packets.

(the code I tried is quite trivial, and it passes the smoke-test in
tools/testing/selftests)

--- >8 ---
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -57,16 +57,8 @@ void mptcp_parse_option(const unsigned char *ptr, int
opsize,
                mp_opt->sndr_key = get_unaligned_be64(ptr);
                ptr += 8;
 
-               if (opsize == TCPOLEN_MPTCP_MPC_SYNACK) {
-                       mp_opt->rcvr_key = get_unaligned_be64(ptr);
-                       ptr += 8;
-                       pr_debug("MP_CAPABLE flags=%x, sndr=%llu,
rcvr=%llu",
-                                mp_opt->flags, mp_opt->sndr_key,
-                                mp_opt->rcvr_key);
-               } else {
-                       pr_debug("MP_CAPABLE flags=%x, sndr=%llu",
-                                mp_opt->flags, mp_opt->sndr_key);
-               }
+               pr_debug("MP_CAPABLE flags=%x, sndr=%llu",
+                        mp_opt->flags, mp_opt->sndr_key);
                break;
 
        /* MPTCPOPT_MP_JOIN
@@ -650,8 +642,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;
                }

any feedback appreciated, thank you in advance!

Hi Davide,

On 29/11/2019 13:54, Davide Caratti wrote:
> On Thu, 2019-11-07 at 07:28 -0800, Christoph Paasch wrote:
>> -#define TCPOLEN_MPTCP_MPC_SYNACK       20
>> +#define TCPOLEN_MPTCP_MPC_SYN          4
>> +#define TCPOLEN_MPTCP_MPC_SYNACK       12
>>   #define TCPOLEN_MPTCP_MPC_ACK          20
> 
> hello Christoph and Paolo,
> 
> the length of the 'mp_capable' option in the SYN/ACK doesn't really change
> when moving from v0 to v1 version of the MPTCP protocol.
> 
> Current mptcp net-next echoes the client's key in the SYN/ACK but I think
> this is not intentional (at least, it does not seem to adhere to section
> 3.1 of RFC https://tools.ietf.org/html/rfc6824).

Indeed, good catch, MP_CAPABLE options are taking 12 bytes for both the 
SYN and SYN/ACK.

> For the record, this is why tcpdump refuses to dissect captures of
> selftests:
> 
>   # ./mptcp_connect.sh -4 -c -e
> 
>   IP 10.0.1.1.49752 > 10.0.1.1.10000: Flags [S], seq 2594718977, win 65535, options [mss 65495,sackOK,TS val 3944347269 ecr 0,nop,wscale 8,mptcp capable {0xf931b304deb39a42}], length 0
>   IP 10.0.1.1.10000 > 10.0.1.1.49752: Flags [S.], seq 3996234497, ack 2594718978, win 65535, options [mss 65495,sackOK,TS val 3944347270 ecr 3944347269,nop,wscale 8,mptcp capable[bad opt]>
> ^^^ bad option
>   IP 10.0.1.1.49752 > 10.0.1.1.10000: Flags [.], ack 1, win 256, options [nop,nop,TS val 3944347270 ecr 3944347270,mptcp capable {0xf931b304deb39a42,0x96614f185e633ce}], length 0
> 
> and also, it's the reason why packetdrill never stores the server key in
> its variables, see:
> 
> https://github.com/dcaratti/packetdrill/issues/1#issuecomment-559569544

The new version of packetdrill is already helping us finding issues \o/

> is it possible to fix size of the syn/ack generated by v0 version of the
> protocol as a separate patch? Otherwise I will need to add a special
> handling for SYN/ACK packets generated by v0 net-next kernels packets.
> 
> (the code I tried is quite trivial, and it passes the smoke-test in
> tools/testing/selftests)

Thank you for the patch! It looks good to me.
I think it would be good to apply it before applying v1 patch.

May you send a "proper patch"? If there is no objection (and an 
additional review?), I can apply it.

Cheers,
Matt