* [MPTCP] Re: [PATCH 1/2] mptcp: parse and emit MP_CAPABLE option according to v1 spec.
@ 2019-11-20 14:56 Matthieu Baerts
0 siblings, 0 replies; 6+ messages in thread
From: Matthieu Baerts @ 2019-11-20 14:56 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 2591 bytes --]
Hi Christoph,
On 07/11/2019 16:28, Christoph Paasch wrote:
> Local key is sent on syn/ack, and both keys are sent on 3rd ack.
> MP_CAPABLE messages len are updated accordingly.
> We need the skbuff to emit correctly the above, so we push the skbuff
> struct as an argument all the way from tcp code to the relevant mptcp
> callbacks.
>
> When processing incoming MP_CAPABLE + data, build a full blown DSS-like
> map info, to simplify later processing.
> On child socket creation, we need to record the remote key, if available.
>
> Signed-off-by: Christoph Paasch <cpaasch(a)apple.com>
Thank you for sharing this! It looks very nice!
I just have a small question, please see below:
[...]
> diff --git a/net/mptcp/options.c b/net/mptcp/options.c
> index 1b08e1193991..9cdbd617c49a 100644
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
[...]
> @@ -24,12 +24,24 @@ void mptcp_parse_option(const unsigned char *ptr, int opsize,
> * 10-17: Receiver key (optional)
> */
> case MPTCPOPT_MP_CAPABLE:
> - if (opsize != TCPOLEN_MPTCP_MPC_SYN &&
> - opsize != TCPOLEN_MPTCP_MPC_SYNACK)
> + /* strict size checking */
> + if (!(TCP_SKB_CB(skb)->tcp_flags & TCPHDR_SYN)) {
> + if (skb->len > tcp_hdr(skb)->doff << 2)
> + expected_opsize = TCPOLEN_MPTCP_MPC_ACK_DATA;
> + else
> + expected_opsize = TCPOLEN_MPTCP_MPC_ACK;
> + } else {
> + if (TCP_SKB_CB(skb)->tcp_flags & TCPHDR_ACK)
> + expected_opsize = TCPOLEN_MPTCP_MPC_SYNACK;
> + else
> + expected_opsize = TCPOLEN_MPTCP_MPC_SYN;
> + }
> + if (opsize != expected_opsize)
> break;
>
> + /* only support V1 of the protocol */
> mp_opt->version = *ptr++ & MPTCP_VERSION_MASK;
> - if (mp_opt->version != 0)
> + if (mp_opt->version != 1)
Should we not only "break" here is the version is < 1? To be compatible
with possible newer versions.
It seems confirmed by the RFC, section 3.1:
If a host supports multiple versions of MPTCP,
the sender of the MP_CAPABLE option SHOULD signal the highest version
number it supports. In return, in its MP_CAPABLE option, the
receiver will signal the version number it wishes to use, which MUST
be equal to or lower than the version number indicated in the initial
MP_CAPABLE.
detail: maybe we need a #define for this minimal version that we support?
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] 6+ messages in thread
* [MPTCP] Re: [PATCH 1/2] mptcp: parse and emit MP_CAPABLE option according to v1 spec.
@ 2019-11-30 8:46 Matthieu Baerts
0 siblings, 0 replies; 6+ messages in thread
From: Matthieu Baerts @ 2019-11-30 8:46 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 4194 bytes --]
Hi Davide,
I prepared this email yesterday but it seems I put my laptop to sleep
too quickly :)
On 29/11/2019 15:44, Davide Caratti wrote:
> hello Matthieu, thanks a lot for looking at this!
That's the least I can do :-)
> On Fri, 2019-11-29 at 14:33 +0100, Matthieu Baerts wrote:
[...]
>> May you send a "proper patch"? If there is no objection (and an
>> additional review?), I can apply it.
>
> I'm doubtful of what should we do with the first hunk, i.e.
Sorry, I misunderstood this. I guess here the code below is valid if you
simply replace TCPOLEN_MPTCP_MPC_SYNACK by TCPOLEN_MPTCP_MPC_ACK.
And I see it is what you propose below.
When we parse the SYN. we should parse the sender key (first line) and
in the 3rd ACK both key (the if-statement).
> --- 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
>
> here the kernel stores in mp_opt->rcvr_key the value received on the
> "longer" mp_capable option (that is, the 3rd packet in the 3-way
> handshake). I don't really get what the server should do with the echoed
> key received by the client, and a grep over the code didn't help:
I think you need it to send a MPTCP Fast Close which is not implemented
yet. I don't think we really need it somewhere else. Could it be used
with ADD_ADDR in MPTCPv1? I would need to re-read this.
> $ grep -r rcvr_key net/* include/*
We can see who has a SSD with NVME and can use "grep" instead of "git
grep" or others :-þ
> net/mptcp/options.c: opts->rcvr_key = subflow->remote_key;
> net/mptcp/options.c: opts->rcvr_key = subflow_req->remote_key;
> net/mptcp/options.c: put_unaligned_be64(opts->rcvr_key, ptr);
> include/linux/tcp.h: u64 rcvr_key;
> include/net/mptcp.h: u64 rcvr_key;
>
> I take as a friday activity (when packetdrill will be in a decent
> shape... so, not not this friday :) ) to understand what happens to a
> mptcp-net-next server when the client echoes back a wrong key.
> To be as much conservative as possible (and preserve the debug printout),
> we could just replace the above hunk with the following one:
>
> --- >8 ---
> --- a/net/mptcp/options.c
> +++ b/net/mptcp/options.c
> @@ -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",
> --- >8 ---
>
> that will also create less troubles to Christian when he rebases his v1
> mp_capable patch on top of this.
>
> WDYT?
Indeed, it seems to be the good fix to do: we should replace the two
TCPOLEN_MPTCP_MPC_SYNACK by TCPOLEN_MPTCP_MPC_ACK in mptcp_parse_option().
Thank you for looking at that!
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] 6+ messages in thread
* [MPTCP] Re: [PATCH 1/2] mptcp: parse and emit MP_CAPABLE option according to v1 spec.
@ 2019-11-29 14:44 Davide Caratti
0 siblings, 0 replies; 6+ messages in thread
From: Davide Caratti @ 2019-11-29 14:44 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 3542 bytes --]
hello Matthieu, thanks a lot for looking at this!
On Fri, 2019-11-29 at 14:33 +0100, Matthieu Baerts wrote:
> Hi Davide,
>
>
> > https://github.com/dcaratti/packetdrill/issues/1#issuecomment-559569544
>
> The new version of packetdrill is already helping us finding issues \o/
yay! :)
> > 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.
I'm doubtful of what should we do with the first hunk, i.e.
--- 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
here the kernel stores in mp_opt->rcvr_key the value received on the
"longer" mp_capable option (that is, the 3rd packet in the 3-way
handshake). I don't really get what the server should do with the echoed
key received by the client, and a grep over the code didn't help:
$ grep -r rcvr_key net/* include/*
net/mptcp/options.c: opts->rcvr_key = subflow->remote_key;
net/mptcp/options.c: opts->rcvr_key = subflow_req->remote_key;
net/mptcp/options.c: put_unaligned_be64(opts->rcvr_key, ptr);
include/linux/tcp.h: u64 rcvr_key;
include/net/mptcp.h: u64 rcvr_key;
I take as a friday activity (when packetdrill will be in a decent
shape... so, not not this friday :) ) to understand what happens to a
mptcp-net-next server when the client echoes back a wrong key.
To be as much conservative as possible (and preserve the debug printout),
we could just replace the above hunk with the following one:
--- >8 ---
--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -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",
--- >8 ---
that will also create less troubles to Christian when he rebases his v1
mp_capable patch on top of this.
WDYT?
--
davide
^ permalink raw reply [flat|nested] 6+ messages in thread
* [MPTCP] Re: [PATCH 1/2] mptcp: parse and emit MP_CAPABLE option according to v1 spec.
@ 2019-11-29 13:33 Matthieu Baerts
0 siblings, 0 replies; 6+ messages in thread
From: Matthieu Baerts @ 2019-11-29 13:33 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 2494 bytes --]
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
--
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] 6+ messages in thread
* [MPTCP] Re: [PATCH 1/2] mptcp: parse and emit MP_CAPABLE option according to v1 spec.
@ 2019-11-29 12:54 Davide Caratti
0 siblings, 0 replies; 6+ messages in thread
From: Davide Caratti @ 2019-11-29 12:54 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 3831 bytes --]
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;
}
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
--- >8 ---
any feedback appreciated, thank you in advance!
--
davide
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [MPTCP] Re: [PATCH 1/2] mptcp: parse and emit MP_CAPABLE option according to v1 spec.
@ 2019-11-07 15:54 Paolo Abeni
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Abeni @ 2019-11-07 15:54 UTC (permalink / raw)
To: mptcp
[-- Attachment #1: Type: text/plain, Size: 2439 bytes --]
Hi,
On Thu, 2019-11-07 at 07:28 -0800, Christoph Paasch wrote:
> @@ -356,28 +387,66 @@ void mptcp_rcv_synsent(struct sock *sk)
> }
> }
>
> -static bool mptcp_established_options_mp(struct sock *sk, unsigned int *size,
> +static bool mptcp_established_options_mp(struct sock *sk, struct sk_buff *skb,
> + unsigned int *size,
> unsigned int remaining,
> struct mptcp_out_options *opts)
> {
> struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(sk);
> + struct mptcp_ext *mpext;
> + unsigned int data_len;
> +
> + pr_debug("subflow=%p fourth_ack=%d seq=%x:%x remaining=%d", subflow,
> + subflow->fourth_ack, subflow->snd_isn,
> + skb ? TCP_SKB_CB(skb)->seq : 0, remaining);
> +
> + if (subflow->mp_capable && !subflow->fourth_ack && skb &&
> + subflow->snd_isn == TCP_SKB_CB(skb)->seq) {
> + /* When skb is not available, we better over-estimate the
> + * emitted options len. A full DSS option is longer than
> + * TCPOLEN_MPTCP_MPC_ACK_DATA, so let's the caller try to fit
> + * that.
> + */
>
> - if (subflow->mp_capable && !subflow->fourth_ack &&
> - remaining >= TCPOLEN_MPTCP_MPC_ACK) {
> - opts->suboptions = OPTION_MPTCP_MPC_ACK;
> - opts->sndr_key = subflow->local_key;
> - opts->rcvr_key = subflow->remote_key;
> - *size = TCPOLEN_MPTCP_MPC_ACK;
> - subflow->fourth_ack = 1;
> - pr_debug("subflow=%p, local_key=%llu, remote_key=%llu",
> - subflow, subflow->local_key, subflow->remote_key);
> - return true;
> + mpext = mptcp_get_ext(skb);
> + data_len = mpext ? mpext->data_len : 0;
> + /* Section 3.1.
> + * The MP_CAPABLE option is carried on the SYN, SYN/ACK, and ACK
> + * packets that start the first subflow of an MPTCP connection,
> + * as well as the first packet that carries data
> + */
> + if (data_len > 0 && remaining >= TCPOLEN_MPTCP_MPC_ACK_DATA) {
It's not clear to me which is the expected behavior when we can't fit
MPTCP options inside the TCP header. As the maximum MP_CAPABLE/MP_JOIN
opt len is 24 bytes, and we have at most 16 bytes in the previous opt
(tstamp, md5) it looks like this can't ever happen, so could emit a
WARN_ON_ONCE() here?
BTW we currently emit a warn if DSS exceeds the TCP option space, and
that can actually happen (max DSS without csum is 26 bytes, with csum
28). Perhaps we could switch to 32 bits seq if we hit such condition?
Cheers,
Paolo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-11-30 8:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-20 14:56 [MPTCP] Re: [PATCH 1/2] mptcp: parse and emit MP_CAPABLE option according to v1 spec Matthieu Baerts
-- strict thread matches above, loose matches on Subject: below --
2019-11-30 8:46 Matthieu Baerts
2019-11-29 14:44 Davide Caratti
2019-11-29 13:33 Matthieu Baerts
2019-11-29 12:54 Davide Caratti
2019-11-07 15:54 Paolo Abeni
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.