* [MPTCP] Re: [PATCH v3] mptcp: fix option length of mp_capable syn/ack
@ 2019-12-02 10:25 Matthieu Baerts
0 siblings, 0 replies; 4+ messages in thread
From: Matthieu Baerts @ 2019-12-02 10:25 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]
Hi Davide,
On 02/12/2019 11:04, Davide Caratti wrote:
> in current MPTCP, the server sends the client's key back in the syn/ack
> packet. Because of that, both tcpdump and packetdrill refuse to parse
> the mp_capable option in the syn/ack. Both RFC6824 and RFC6824bis don't
> require this in section 3.1, only the server's key should be sent in
> the syn/ack.
>
> - change TCPOLEN_MPTCP_MPC_SYNACK from 20 (4+8+8) to 12 (4+8)
> - don't write the client's key in mp_capable syn/ack
> - adjust tests in mptcp_parse_option() to cope with the changed
> value of TCPOLEN_MPTCP_MPC_SYNACK
[...]
> v2: more conservative behavior when parsing received options
> v3: adjust tests in mptcp_parse_option() to catch all possible
> values of MP_CAPABLE option length specified by v0 protocol,
> thanks Matthieu Baerts.
Thank you for this new version! It looks good to me.
I will wait for Paolo and/or Christoph's ACK as you added them in cc
(for the discussion in the MPTCPv1 patches).
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [MPTCP] Re: [PATCH v3] mptcp: fix option length of mp_capable syn/ack
@ 2019-12-05 10:52 Davide Caratti
0 siblings, 0 replies; 4+ messages in thread
From: Davide Caratti @ 2019-12-05 10:52 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 1550 bytes --]
hello Peter!
On Wed, 2019-12-04 at 11:35 -0800, Peter Krystad wrote:
> Hi Davide -
>
> On Mon, 2019-12-02 at 11:04 +0100, Davide Caratti wrote:
> > in current MPTCP, the server sends the client's key back in the syn/ack
> > packet. Because of that, both tcpdump and packetdrill refuse to parse
> > the mp_capable option in the syn/ack. Both RFC6824 and RFC6824bis don't
> > require this in section 3.1, only the server's key should be sent in
> > the syn/ack.
[...]
> > 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
> > #define TCPOLEN_MPTCP_MPC_ACK 20
> > #define TCPOLEN_MPTCP_MPJ_SYN 12
> > #define TCPOLEN_MPTCP_MPJ_SYNACK 16
>
> This is minor but I think for clarity in mptcp_synack_options() we should
> remove the line that copies the remote_key into the option space:
>
> if (subflow_req->mp_capable) {
> opts->suboptions = OPTION_MPTCP_MPC_SYNACK;
> opts->sndr_key = subflow_req->local_key;
> opts->rcvr_key = subflow_req->remote_key; <== delete this
> *size = TCPOLEN_MPTCP_MPC_SYNACK;
> The new value for TCPOLEN_MPTCP_MPC_SYNACK means it won't be sent as intended
> by this change, but this line is now irrevelant.
correct, I will fix this in v4.
thanks!
--
davide
^ permalink raw reply [flat|nested] 4+ messages in thread
* [MPTCP] Re: [PATCH v3] mptcp: fix option length of mp_capable syn/ack
@ 2019-12-05 0:52 Mat Martineau
0 siblings, 0 replies; 4+ messages in thread
From: Mat Martineau @ 2019-12-05 0:52 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 4549 bytes --]
On Mon, 2 Dec 2019, Davide Caratti wrote:
> in current MPTCP, the server sends the client's key back in the syn/ack
> packet. Because of that, both tcpdump and packetdrill refuse to parse
> the mp_capable option in the syn/ack. Both RFC6824 and RFC6824bis don't
> require this in section 3.1, only the server's key should be sent in
> the syn/ack.
>
> - change TCPOLEN_MPTCP_MPC_SYNACK from 20 (4+8+8) to 12 (4+8)
> - don't write the client's key in mp_capable syn/ack
> - adjust tests in mptcp_parse_option() to cope with the changed
> value of TCPOLEN_MPTCP_MPC_SYNACK
>
> 3-way handshakes obtained with
>
> # ./mptcp_connect.sh -4 -c -e tx
> [...]
> # tcpdump -c3 -tnnr ns1-5de04ec0-TGs4tf-ns1-5de04ec0-TGs4tf-MPTCP-MPTCP-10.0.1.1.pcap tcp
>
> unpatched kernel:
>
> 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
>
> patched kernel:
>
> IP 10.0.1.1.50850 > 10.0.1.1.10000: Flags [S], seq 626898452, win 65495, options [mss 65495,sackOK,TS val 2266878231 ecr 0,nop,wscale 7,mptcp capable {0x7fe37bfd872b9f9f}], length 0
> IP 10.0.1.1.10000 > 10.0.1.1.50850: Flags [S.], seq 3966041155, ack 626898453, win 65483, options [mss 65495,sackOK,TS val 2266878232 ecr 2266878231,nop,wscale 7,mptcp capable {0x8a167e3af39b5fc1}], length 0
> ^^^ no more bad option
> IP 10.0.1.1.50850 > 10.0.1.1.10000: Flags [.], ack 1, win 512, options [nop,nop,TS val 2266878232 ecr 2266878232,mptcp capable {0x7fe37bfd872b9f9f,0x8a167e3af39b5fc1}], length 0
>
> v2: more conservative behavior when parsing received options
> v3: adjust tests in mptcp_parse_option() to catch all possible
> values of MP_CAPABLE option length specified by v0 protocol,
> thanks Matthieu Baerts.
>
> Fixes: 8fa520f034ed ("mptcp: Handle MPTCP TCP options")
> Signed-off-by: Davide Caratti <dcaratti(a)redhat.com>
> ---
> net/mptcp/options.c | 7 +++----
> net/mptcp/protocol.h | 2 +-
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 98eb0281d196..eb29da31c7bd 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)
This preserves the old parsing behavior for MPTCPv0, which I think is the
right thing to achieve in this patch. I do prefer the checking that is
implemented in the MPTCPv1 patchset that uses the actual TCP flags to
confirm the expected option lengths, and that will get merged soon!
Looks good to me for the MPTCPv0 code.
Mat
> break;
>
> mp_opt->version = *ptr++ & MPTCP_VERSION_MASK;
> @@ -57,7 +57,7 @@ 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) {
> + 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
> #define TCPOLEN_MPTCP_MPC_ACK 20
> #define TCPOLEN_MPTCP_MPJ_SYN 12
> #define TCPOLEN_MPTCP_MPJ_SYNACK 16
--
Mat Martineau
Intel
^ permalink raw reply [flat|nested] 4+ messages in thread
* [MPTCP] Re: [PATCH v3] mptcp: fix option length of mp_capable syn/ack
@ 2019-12-04 19:35 Peter Krystad
0 siblings, 0 replies; 4+ messages in thread
From: Peter Krystad @ 2019-12-04 19:35 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 4768 bytes --]
Hi Davide -
On Mon, 2019-12-02 at 11:04 +0100, Davide Caratti wrote:
> in current MPTCP, the server sends the client's key back in the syn/ack
> packet. Because of that, both tcpdump and packetdrill refuse to parse
> the mp_capable option in the syn/ack. Both RFC6824 and RFC6824bis don't
> require this in section 3.1, only the server's key should be sent in
> the syn/ack.
>
> - change TCPOLEN_MPTCP_MPC_SYNACK from 20 (4+8+8) to 12 (4+8)
> - don't write the client's key in mp_capable syn/ack
> - adjust tests in mptcp_parse_option() to cope with the changed
> value of TCPOLEN_MPTCP_MPC_SYNACK
>
> 3-way handshakes obtained with
>
> # ./mptcp_connect.sh -4 -c -e tx
> [...]
> # tcpdump -c3 -tnnr ns1-5de04ec0-TGs4tf-ns1-5de04ec0-TGs4tf-MPTCP-MPTCP-10.0.1.1.pcap tcp
>
> unpatched kernel:
>
> 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
>
> patched kernel:
>
> IP 10.0.1.1.50850 > 10.0.1.1.10000: Flags [S], seq 626898452, win 65495, options [mss 65495,sackOK,TS val 2266878231 ecr 0,nop,wscale 7,mptcp capable {0x7fe37bfd872b9f9f}], length 0
> IP 10.0.1.1.10000 > 10.0.1.1.50850: Flags [S.], seq 3966041155, ack 626898453, win 65483, options [mss 65495,sackOK,TS val 2266878232 ecr 2266878231,nop,wscale 7,mptcp capable {0x8a167e3af39b5fc1}], length 0
> ^^^ no more bad option
> IP 10.0.1.1.50850 > 10.0.1.1.10000: Flags [.], ack 1, win 512, options [nop,nop,TS val 2266878232 ecr 2266878232,mptcp capable {0x7fe37bfd872b9f9f,0x8a167e3af39b5fc1}], length 0
>
> v2: more conservative behavior when parsing received options
> v3: adjust tests in mptcp_parse_option() to catch all possible
> values of MP_CAPABLE option length specified by v0 protocol,
> thanks Matthieu Baerts.
>
> Fixes: 8fa520f034ed ("mptcp: Handle MPTCP TCP options")
> Signed-off-by: Davide Caratti <dcaratti(a)redhat.com>
> ---
> net/mptcp/options.c | 7 +++----
> net/mptcp/protocol.h | 2 +-
> 2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 98eb0281d196..eb29da31c7bd 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;
> @@ -57,7 +57,7 @@ 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) {
> + 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
> #define TCPOLEN_MPTCP_MPC_ACK 20
> #define TCPOLEN_MPTCP_MPJ_SYN 12
> #define TCPOLEN_MPTCP_MPJ_SYNACK 16
This is minor but I think for clarity in mptcp_synack_options() we should
remove the line that copies the remote_key into the option space:
if (subflow_req->mp_capable) {
opts->suboptions = OPTION_MPTCP_MPC_SYNACK;
opts->sndr_key = subflow_req->local_key;
opts->rcvr_key = subflow_req->remote_key; <== delete this
*size = TCPOLEN_MPTCP_MPC_SYNACK;
The new value for TCPOLEN_MPTCP_MPC_SYNACK means it won't be sent as intended
by this change, but this line is now irrevelant.
Peter.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-12-05 10:52 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-02 10:25 [MPTCP] Re: [PATCH v3] mptcp: fix option length of mp_capable syn/ack Matthieu Baerts
2019-12-04 19:35 Peter Krystad
2019-12-05 0:52 Mat Martineau
2019-12-05 10:52 Davide Caratti
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.