December 2019 - mptcp

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

by Davide Caratti

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 -- 2.23.0

2 years, 6 months

4
4
0 / 0

[PATCH 0/3] mptcp: support for rfc6824 MP CAPABLE hanshake

by Christoph Paasch

Second iteration of the v1 MP_CAPABLE handshake, adding the handling of the option-space issue. The patches apply just before "mptcp: add basic kselftest for mptcp". Christoph Paasch (2): mptcp: parse and emit MP_CAPABLE option according to v1 spec. mptcp: process MP_CAPABLE data option. Paolo Abeni (1): mptcp: move from sha1 (v0) to sha256 (v1) include/linux/tcp.h | 3 +- include/net/mptcp.h | 11 +- net/ipv4/tcp_input.c | 2 +- net/ipv4/tcp_output.c | 2 +- net/mptcp/Kconfig | 10 ++ net/mptcp/crypto.c | 139 +++++++++++++++---------- net/mptcp/options.c | 229 ++++++++++++++++++++++++++++++++---------- net/mptcp/protocol.c | 22 ++-- net/mptcp/protocol.h | 18 ++-- net/mptcp/subflow.c | 43 +++++++- 10 files changed, 349 insertions(+), 130 deletions(-) -- 2.23.0

2 years, 6 months

5
18
0 / 0

Re: MPTCP implementation feedback for RFC6824bis

by Christoph Paasch

Hello Alan, > On Dec 4, 2019, at 1:50 PM, Alan Ford <alan.ford(a)gmail.com> wrote: > > Hi Christoph, > > Thank you for the clarifications. I was revisiting the text to see ways to make these clarifications, however I find myself unsure of the need; please see comments below: > > > Section 3.1 clarification > > Towards the end of Section 3.1 we actually say the following: > > The SYN with MP_CAPABLE occupies the first octet of data sequence space, although this does not need to be acknowledged at the connection level until the first data is sent (see Section 3.3). > > Which would seem to cover exactly this concern. I'm not sure how it covers the concern, because the issue I raise is that the client needs to somehow find out if the MP_CAPABLE made it to the receiver. And the clarification I suggest is to spell out that it is the DATA_ACK (see my next comment). > However if you still feel further clarification is required, then we could add this also to the sentence you suggest, i.e.: > > If B has data to send first, then the reliable delivery of the ACK + MP_CAPABLE can be inferred by the receipt of this data with an MPTCP Data Sequence Signal (DSS) option (Section 3.3). > The ambiguity here is that DSS does not imply DATA_ACK. One could send a DSS without a DATA_ACK, but that would not signal to the client the reliable delivery of the MP_CAPABLE. Only the DATA_ACK will do so. > Can change to: > > If B has data to send first, then the reliable delivery of the ACK + MP_CAPABLE can be inferred by the receipt of this data with an MPTCP Data Sequence Signal (DSS) option (Section 3.3) containing a DATA_ACK for the MP_CAPABLE (which is the first octet of the data sequence space). Furthermore, when A receives a DATA_ACK from B it is a signal of the reliable delivery of A's MP_CAPABLE. > > Please confirm if you still feel this is necessary, given the quote I provide. Yes, I still feel that this is necessary. > > > Early Mapping > > A Data Sequence Mapping does not need to be included in every MPTCP > packet, as long as the subflow sequence space in that packet is > covered by a mapping known at the receiver. This can be used to > reduce overhead in cases where the mapping is known in advance. One > such case is when there is a single subflow between the hosts, and > another is when segments of data are scheduled in larger-than-packet- > sized chunks. > > I would suggest simply adding a sentence at the beginning saying “A Data Sequence Mapping MUST appear on a TCP segment which is covered by the mapping”. Yes, adding that sentence would clarify this. I also would remove the sentence "This can be used to..." and "One such case is..." - it adds to the confusion IMO. > > Regarding late mapping, we say: > > Implementations MAY hold onto such unmapped data for a > short while, in the expectation that a mapping will arrive shortly. > Such unmapped data cannot be counted as being within the connection- > level receive window because this is relative to the data sequence > numbers, so if the receiver runs out of memory to hold this data, it > will have to be discarded. If a mapping for that subflow-level > sequence space does not arrive within a receive window of data, that > subflow SHOULD be treated as broken, closed with a RST, and any > unmapped data silently discarded. > > Note the last sentence. We already bound this suggestion to just a subflow receive window of data, and provide a mechanism to reject (RST and silently discard). If we add the above text re early mapping that would also apply in this case and provide your requirement that the mapping lands on a segment covered by the mapping. Yes, this last sentence, combined with the above-mentioned addition should make things fine. Thanks, Christoph > > Any thoughts? > > Best regards, > Alan > >> On 2 Dec 2019, at 17:27, Christoph Paasch <cpaasch(a)apple.com <mailto:cpaasch@apple.com>> wrote: >> >> Hello Alan, >> >> On 29/11/19 - 21:13:38, Alan Ford wrote: >>>> On 28 Nov 2019, at 19:49, Christoph Paasch <cpaasch(a)apple.com <mailto:cpaasch@apple.com>> wrote: >>>>> On Nov 28, 2019, at 8:16 AM, Alan Ford <alan.ford(a)gmail.com <mailto:alan.ford@gmail.com> <mailto:alan.ford@gmail.com <mailto:alan.ford@gmail.com>>> wrote: >>>>>> On 27 Nov 2019, at 19:29, Christoph Paasch <cpaasch(a)apple.com <mailto:cpaasch@apple.com> <mailto:cpaasch@apple.com <mailto:cpaasch@apple.com>>> wrote: >>>>>> Section 3.3.1, page 32 & 33, "A data sequence mapping does not need..." >>>>>> >>>>>> This paragraph states that it is permissive to send a mapping in advance. Late-mapping is specified a bit higher around the sentence >>>>>> "Implementations MAY hold onto such unmapped data for a short while in the expectation that a mapping will arrive shortly" >>>>>> >>>>>> This kind of early/late mapping announcements are difficult to handle in an implementation. The Linux Kernel implementation of multipath-tcp.org <http://multipath-tcp.org/> <http://multipath-tcp.org/ <http://multipath-tcp.org/>> has always disallowed such kind of mappings. Meaning, whenever a DSS-option is received such that the range specified by the relative subflow-sequence number in the DSS-option and the data-length does not (partially) cover the TCP sequence number of the packet itself, the subflow will be killed with a TCP-RST. The problem around handling such early/late mappings is that it is unclear for how long the stack needs to remember these mappings (in the early-mapping case), or for how long he needs to hold on to the data (in the late-mapping case). >>>>>> >>>>>> We thus suggest to change this to the following: >>>>>> Whenever a DSS-option is received on a packet such that the mapping of the subflow-sequence space does not partially cover the TCP-sequence number of the packet itself, the host MUST discard this mapping and MAY destroy the subflow with a TCP-RST. It should be noted that a DATA_FIN that does not come with data has a relative subflow-sequence number of 0 and thus should be handled separately. >>>>> >>>>> This one I do have an issue with: >>>>> >>>>> - It is a technical change >>>>> - Wording to this effect has been in the document since pretty much the beginning >>>>> - It is a MAY which might as well say “there is no guarantee this would work” >>>> >>>> The problem with the MAY is that the sender can't really know if the receiver accepts it (more regarding this below) >>>> >>>>> Most importantly, the replacement text seems not to address this issue at all. If I read it correctly, it says that the data sequence mapping option MUST partially cover the subflow sequence space of the packet itself. But that has nothing to do with late or early mapping, both could partially cover the subflow sequence space and preceding data. >>>>> >>>>> Can you clarify exactly what you want to permit and prevent, here? >>>> >>>> Let me try to clarify what exactly we mean with early/late mapping so that we are all on the same terms here: >>>> >>>> Early mapping: >>>> >>>> A TCP-segment with sequence-number 1 holds a DSS-option with subflow-sequence number 1001 and data-length 100. This means we need to allocate space to store this DSS-option so that when the TCP-segment with seqno 1001 arrives we can know the mapping. There may be coming more of these DSS-options which all need to be stored in allocated memory. It is unclear what the limit to this is and there is no way to communicate this limit to the sender. >>> >>> I don’t think we have ever intended to support a mapping like this. If the text is not clear here then yes, we might have an issue. >> >> Yes, I do think that the text is not very clear on that - we should clarify >> that. >> >>> We intend only to support: A TCP segment with sequence-number 1 holds a DSS option for SSN 1 and length 10000 (so multiple segments in the future). >> >> Sounds good! >> >>> Or, slightly more convoluted, a TCP segment with SN 100, length 100, which holds a DSS option for 151-250, and bytes 50-150 were already covered by a previous mapping. >> >> Also good. >> >>> I do not believe we have ever intended mappings on data segments where the segments do not include any of the mapped data. We did, however, intend to support mappings on pure ACKs in order to avoid any option space limits. >> >> Mappings on a pure ACK are an unlikely use-case. The problem is that in the >> end the mapping needs to reliably make it to the receiver as otherwise he >> needs to throw the data away. Combining it with data implicitly makes it >> reliable. >> >> In case of option-space limits, it is better to send the options that do >> consume a lot of space (ADD_ADDR,...) on the pure ACK. >> >>> >>>> Late mapping: >>>> >>>> The receiver receives data without DSS-options with TCP-sequence 1 to 1001. The corresponding DSS-option however arrives with the TCP-segment with seqno 2001. Here, the receiver needs to hold on to this data, waiting for the TCP-segment with the DSS-option. At one point the receiver needs to drop the data due to memory limits. Again, the sender has no way for knowing what this limit is. >>> >>> So this is slightly more problematic, and this is what the MAY in the text is designed to discuss. >>> >>> I believe we had intended to support a situation where you could have a segment 1-1000 without a DSS option, and 1001-2000 also without, and then 2001-3000 with the option then providing a mapping for 1 to 2001 or higher. >> >> I think this scenario is fine to some extend with a slight change that the >> provided mapping should be for 1 to 2002 or higher (thus, including the byte >> of the 2001-3000 segment). >> >>> This would allow a sender to start pushing out data before it knew what a mapping might look like. >>> It does, however, seem an unlikely situation but you as a receiver could of course reduce the subflow window size in order to limit buffering. The text does recognise the memory issue and point out that this won’t be DATA ACKed and as such a sender should soon realise and retransmit on a separate subflow and then this subflow may eventually be closed. >> >> Yes, it seems like an unlikerly situation and I'm not sure about the >> use-case for doing this. >> >>> Another situation would involve two mappings on the same TCP segment. If the first 50 bytes of a segment are covered by the mapping provided in that segment, but there are then also another 50 bytes, then the mapping can’t be provided until the next segment. >> >> Yes, two mappings on a single packet are problematic simply because of the >> lack of option-space. >> >>> I guess my initial thought here is that this was intended to cover a number of corner cases but if it does not work for you then there are several compliant ways of dealing with it. >>> >>>> When the DSS-option comes together with the corresponding TCP-sequence it is straight-forward to store it together with the data. There are no issues with memory-allocation,... as all of this is accounted together with the announced window (yes, the memory is not counted against the window, but the receiver can foresee the DSS-option overhead when computing what window he should announce). >>>> >>>> When a receiver gets data without a DSS-option, he can store it for up to 64KB of data as that is the maximum data-level length and the last segment of the 64KB-train could be holding the DSS-option. After that he has to drop the data. >>>> >>>> When the mapping partially covers the segment it also isn't a problem as the unmapped part can safely be dropped and the mapped part can be passed on to the MPTCP-layer. >>>> >>>> All of this does not imply that every segment of a mapping needs to hold the DSS-option. Just one of them needs to have it. >>> >>> That last statement aligns with what was intended in the text. But are you really saying that, or are you saying that the first one needs to have it? Because that would be a change. >> >> No, the first one does not need to have it. Just one of them. >> >> >> Cheers, >> Christoph >> >

2 years, 6 months

1
0
0 / 0

[PATCH v3] mptcp: Basic single-subflow DATA_FIN

by Mat Martineau

Send a DATA_FIN along with any subflow TCP FIN flag when the MPTCP socket is closing or shutting down writes. Signed-off-by: Mat Martineau <mathew.j.martineau(a)linux.intel.com> --- The DATA_FIN ack patch (would have been a 2nd patch after this) isn't quite working yet, but I wanted to share the sending patch since there was a minor conflict after some recent squashing. I also confirmed that this applies cleanly after the kselftest patch for "part 1", and that the self tests pass. Changes from v2: * Rebased and confirmed selftests when cherry-picked to the end of the "part 1" (single-subflow) chunk of the patch series. Changes from v1: * Fixed problem with the receive side truncating data. The issue happened when a DSS mapping for DATA_FIN was found on a data segment, where the data in the packet was already covered by an earlier mapping. * Only send DATA_FIN when the subflow is sending a FIN and the MPTCP socket is no longer in the TCP_ESTABLISHED state. This prevents sending DATA_FIN when an individual subflow is removed but the MPTCP-level connection is kept alive. * Changes to warnings and comments suggested in code review. net/mptcp/options.c | 38 ++++++++++++++++++++++++++++++++++++-- net/mptcp/protocol.c | 4 ++++ net/mptcp/subflow.c | 42 ++++++++++++++++++++++++++++-------------- 3 files changed, 68 insertions(+), 16 deletions(-) diff --git a/net/mptcp/options.c b/net/mptcp/options.c index 644c30d46922..be180c4f71a4 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -286,19 +286,49 @@ static bool mptcp_established_options_mp(struct sock *sk, unsigned int *size, return false; } +static void mptcp_write_data_fin(struct mptcp_subflow_context *subflow, + struct mptcp_ext *ext) +{ + ext->data_fin = 1; + + if (!ext->use_map) { + /* RFC6824 requires a DSS mapping with specific values + * if DATA_FIN is set but no data payload is mapped + */ + ext->use_map = 1; + ext->dsn64 = 1; + ext->data_seq = mptcp_sk(subflow->conn)->write_seq; + ext->subflow_seq = 0; + ext->data_len = 1; + } else { + /* If there's an existing DSS mapping, DATA_FIN consumes + * 1 additional byte of mapping space. + */ + ext->data_len++; + } +} + static bool mptcp_established_options_dss(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); unsigned int dss_size = 0; struct mptcp_ext *mpext; struct mptcp_sock *msk; unsigned int ack_size; + u8 tcp_fin; - mpext = skb ? mptcp_get_ext(skb) : NULL; + if (skb) { + mpext = mptcp_get_ext(skb); + tcp_fin = TCP_SKB_CB(skb)->tcp_flags & TCPHDR_FIN; + } else { + mpext = NULL; + tcp_fin = 0; + } - if (!skb || (mpext && mpext->use_map)) { + if (!skb || (mpext && mpext->use_map) || tcp_fin) { unsigned int map_size; map_size = TCPOLEN_MPTCP_DSS_BASE + TCPOLEN_MPTCP_DSS_MAP64; @@ -307,6 +337,10 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb, dss_size = map_size; if (mpext) opts->ext_copy = *mpext; + + if (skb && tcp_fin && + subflow->conn->sk_state != TCP_ESTABLISHED) + mptcp_write_data_fin(subflow, &opts->ext_copy); } ack_size = TCPOLEN_MPTCP_DSS_ACK64; diff --git a/net/mptcp/protocol.c b/net/mptcp/protocol.c index 9b384da910ce..f95a3f824a18 100644 --- a/net/mptcp/protocol.c +++ b/net/mptcp/protocol.c @@ -919,6 +919,10 @@ static int mptcp_shutdown(struct socket *sock, int how) pr_debug("sk=%p, how=%d", msk, how); lock_sock(sock->sk); + + if (how == SHUT_WR || how == SHUT_RDWR) + inet_sk_state_store(sock->sk, TCP_FIN_WAIT1); + ssock = __mptcp_fallback_get_ref(msk); if (ssock) { release_sock(sock->sk); diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index 998fc6066ea5..ee29281d4570 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -277,6 +277,7 @@ static enum mapping_status get_mapping_status(struct sock *ssk) struct mptcp_subflow_context *subflow = mptcp_subflow_ctx(ssk); struct mptcp_ext *mpext; struct sk_buff *skb; + u16 data_len; u64 map_seq; skb = skb_peek(&ssk->sk_receive_queue); @@ -301,25 +302,38 @@ static enum mapping_status get_mapping_status(struct sock *ssk) if (!subflow->map_valid) return MAPPING_INVALID; + goto validate_seq; } - pr_debug("seq=%llu is64=%d ssn=%u data_len=%u", mpext->data_seq, - mpext->dsn64, mpext->subflow_seq, mpext->data_len); + pr_debug("seq=%llu is64=%d ssn=%u data_len=%u data_fin=%d", + mpext->data_seq, mpext->dsn64, mpext->subflow_seq, + mpext->data_len, mpext->data_fin); - if (mpext->data_len == 0) { + data_len = mpext->data_len; + if (data_len == 0) { pr_err("Infinite mapping not handled"); return MAPPING_INVALID; - } else if (mpext->subflow_seq == 0 && - mpext->data_fin == 1) { - if (WARN_ON_ONCE(mpext->data_len != 1)) - return false; + } - /* do not try hard to handle this any better, till we have - * real data_fin support - */ - pr_debug("DATA_FIN with no payload"); - return MAPPING_DATA_FIN; + if (mpext->data_fin == 1) { + if (data_len == 1) { + pr_debug("DATA_FIN with no payload"); + if (subflow->map_valid) { + /* A DATA_FIN might arrive in a DSS + * option before the previous mapping + * has been fully consumed. Continue + * handling the existing mapping. + */ + skb_ext_del(skb, SKB_EXT_MPTCP); + return MAPPING_OK; + } else { + return MAPPING_DATA_FIN; + } + } + + /* Adjust for DATA_FIN using 1 byte of sequence space */ + data_len--; } if (!mpext->dsn64) { @@ -334,7 +348,7 @@ static enum mapping_status get_mapping_status(struct sock *ssk) /* Allow replacing only with an identical map */ if (subflow->map_seq == map_seq && subflow->map_subflow_seq == mpext->subflow_seq && - subflow->map_data_len == mpext->data_len) { + subflow->map_data_len == data_len) { skb_ext_del(skb, SKB_EXT_MPTCP); return MAPPING_OK; } @@ -351,7 +365,7 @@ static enum mapping_status get_mapping_status(struct sock *ssk) subflow->map_seq = map_seq; subflow->map_subflow_seq = mpext->subflow_seq; - subflow->map_data_len = mpext->data_len; + subflow->map_data_len = data_len; subflow->map_valid = 1; pr_debug("new map seq=%llu subflow_seq=%u data_len=%u", subflow->map_seq, subflow->map_subflow_seq, -- 2.24.0

2 years, 6 months

4
4
0 / 0

Weekly meeting - 5 December 2019 17:00 UTC (9am PST, 6pm CET)

by Matthieu Baerts

Hello everybody! Our public MPTCP upstreaming weekly web conference is scheduled for tomorrow. The list of topics is available here: https://annuel2.framapad.org/p/mptcp_upstreaming_20191205 Feel free to add/modify topics! Anybody can join the meeting using Google Meet website/app/phone: https://meet.google.com/edk-rkrk-vaj Speak to you tomorrow! Matthieu -- 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

2 years, 6 months

1
0
0 / 0

[PATCH 0/3] sendmsg: fix pending issues.

by Paolo Abeni

This is just a rebase of the initial RFC post. The patch 1 should be inserted just after "tcp: clean ext on tx recycle" The patches 2 and 3 have explicit individual squash-to tags, but will casue a lot of conflicts, the uneasy ones to solve are on patch "mptcp: rework mptcp_sendmsg_frag to accept optional dfrag" A tree rebased with the above patches applied is available at: https://github.com/pabeni/mptcp/tree/sendmsg_fix_2 The original cover letter was as follow (TL;DR: patch 1 and 2 can be controversial, feedback needed!!!) --- This series addresses the known pending issue in mptcp_sendmsg, specifically: * the crash repored by matt (patch 3/3) * the possible data corruption on ext allocation failure - the "TODO"" in sendmsg_frag (patches 1,2/3) uunfortunately none of the above is actually tested, since I can't reproduce the issues - still the problems look real. This is not squashed yet, as it will conflict with most patches, I'm just trying to collect feedback. Open points: The patch 2/3 introduces a per socket cache of a single skb_ext. As per recent upstream changes related to TCP rx/tx skb cache, that is debatable, could have subtle performance and memory usage implications. We could avoid such cache allways allocating the extension for each sendmsg_frag() call, and than free it if unused. Both options looks sub-optiomal to me do you have any preference? - I do have some for the selected impl ;) The new API exposed by patch 1/3 is open to misusage. e.g. the caller could allocate a skb_ext, set arbitrary ext->offset[id] values and than call __skb_ext_add(). That will lead to memory leak or GPF. I did no try to address the issue, the '__' prefix should guide the users to extra care. Can we do any better? (this is a pre-existing topic) the sk_stream_wait_memory() calls in sk_stream_wait_memory(), waits on the subflow. Should we wait on the mptcp socket instead? Otherwise should we release the mptcp socket lock, before such call? Paolo Abeni (1): skb: add helpers to allocate ext independently from sk_buff include/linux/skbuff.h | 3 +++ net/core/skbuff.c | 16 +++++++++++++++- 2 files changed, 18 insertions(+), 1 deletion(-) -- 2.21.0

2 years, 6 months

3
10
0 / 0

[RFC PATCH 0/3] sendmsg: fix pending issues.

by Paolo Abeni

This series addresses the known pending issue in mptcp_sendmsg, specifically: * the crash repored by matt (patch 3/3) * the possible data corruption on ext allocation failure - the "TODO"" in sendmsg_frag (patches 1,2/3) uunfortunately none of the above is actually tested, since I can't reproduce the issues - still the problems look real. This is not squashed yet, as it will conflict with most patches, I'm just trying to collect feedback. Open points: The patch 2/3 introduces a per socket cache of a single skb_ext. As per recent upstream changes related to TCP rx/tx skb cache, that is debatable, could have subtle performance and memory usage implications. We could avoid such cache allways allocating the extension for each sendmsg_frag() call, and than free it if unused. Both options looks sub-optiomal to me do you have any preference? - I do have some for the selected impl ;) The new API exposed by patch 1/3 is open to misusage. e.g. the caller could allocate a skb_ext, set arbitrary ext->offset[id] values and than call __skb_ext_add(). That will lead to memory leak or GPF. I did no try to address the issue, the '__' prefix should guide the users to extra care. Can we do any better? (this is a pre-existing topic) the sk_stream_wait_memory() calls in sk_stream_wait_memory(), waits on the subflow. Should we wait on the mptcp socket instead? Otherwise should we release the mptcp socket lock, before such call? Paolo Abeni (3): skb: add helpers to allocate ext independently from sk_buff mptcp: avoid data stream corruption on allocation failure. mptcp: properly detect skb collapsing include/linux/skbuff.h | 3 ++ net/core/skbuff.c | 16 ++++++++- net/mptcp/protocol.c | 79 ++++++++++++++++++++++++++---------------- net/mptcp/protocol.h | 1 + 4 files changed, 68 insertions(+), 31 deletions(-) -- 2.21.0

2 years, 6 months

2
5
0 / 0

[Weekly meetings] MoM - 2nd of December 2019

by Matthieu Baerts

Hello, Yesterday, we had our 77th meeting with Mat, Peter and Ossama (Intel OTC), Christoph (Apple), Paolo, Florian and Davide (RedHat) and myself (Tessares). Thanks again for this new good meeting! Here are the minutes of the meeting: Accepted patches: - The list of accepted patches can be seen on PatchWork: https://patchwork.ozlabs.org/project/mptcp/list/?state=3 - By Davide Caratti, Florian Westphal, Paolo Abeni 1202768 selftests: remove pcap captures with 'make clean' 1201563 init ssn_offset for incoming/outgoing join requests 1200453 subflow: don't increment msk refcount on join connect completio 1200038 [3/7] remove second conditional 1200037 [2/7] mptcp: must drop reqsk as well 1200035 [1/7] subflow: fix join error handling 1199503 [2/4] squash-to "mptcp: Write MPTCP DSS headers to outgoing dat 1199499 [3/4] Squash-to: "mptcp: Add handling of outgoing MP_JOIN reque 1199502 [4/4] Squash-to: mptcp: Add handling of incoming MP_JOIN reques 1199501 [1/4] crypto: refactor crypto helpers. Pending patches: - The list of pending patches can be seen on PatchWork: https://patchwork.ozlabs.org/project/mptcp/list/?state=* - By Christoph Paasch, Davide Caratti, Florian Westphal, Mat Martineau, Matthieu Baerts, Paolo Abeni, Peter Krystad 1194592: Changes Requested: [RFC,1/1] mptcp: Optimize struct mptcp_recei 1196109: RFC: [10/10,RFC] selftests:mptcp: decrease timeout to 100 sec 1199714: Awaiting Upstream: [v3] mptcp: Basic single-subflow DATA_FIN 1202759: Awaiting Upstream: [0/9] mptcp: support mptcp joins 1202758: Awaiting Upstream: [1/9] token: handle join-before-accept-compl 1202760: Awaiting Upstream: [2/9] mptcp: fix race from mptcp_close/mptcp 1202761: Awaiting Upstream: [3/9] copy connection id from first subflow 1202762: Awaiting Upstream: [4/9] store tcp_sk, not socket 1202763: Awaiting Upstream: [5/9] add mptcp_subflow_shutdown 1202764: Awaiting Upstream: [6/9] make accept not allocate kernel socket 1202765: Awaiting Upstream: [7/9] pass subflow socket to mptcp_finish_co 1202766: Awaiting Upstream: [8/9] subflow: place further subflows on new 1203040: New: [v3] mptcp: fix option length of mp_capable syn/ack 1203059: Needs Review / ACK: [1/3] skb: add helpers to allocate ext inde 1203061: Needs Review / ACK: [2/3] mptcp: avoid data stream corruption o 1203062: Needs Review / ACK: [3/3] imptcp: properly detect skb collapsin 1203224: New: [1/3] mptcp: move from sha1 (v0) to sha256 (v1) 1203225: New: [2/3] mptcp: parse and emit MP_CAPABLE option according to 1203227: New: [3/3] mptcp: process MP_CAPABLE data option. 1203258: Awaiting Upstream: [v2,9/9] don't flag parent socket as RCV_SH FYI: Current Roadmap: - Part 1 (mainly TCP changes, will be sent with Part 2): - MAINTAINERS file [TODO] - Part 2 (minimum set for MPTCP, up to KSelftests, one subflow): - MPTCPv1 support [Patches waiting for final review] - opti in TCP options? [Patch shared but it needs rebase and split] - Send DATA_FIN, no corner cases [Patches can be applied] - IPv6 support [Done] - if the peer never sends MPTCP-level ACK, a lot of memory will be used [Patches to be applied] - Part 3 (after the KSelftests, to be sent ideally before the end of the year) - Full DATA_FIN support [WIP] - Shared recv window (drop data received on other subflows) [TODO] - Active backup support [WIP] - Part 4: - Shared recv window (full support) - IPv6 - IPv4 mapped support - not dropping MPTCP options (ADD_ADDR, etc.) - FAST_CLOSE - full MPTCP v1 support (reliable add_addr, etc.) - Part 5: - opti/perfs - TFO Items for the initial submission: Update: - MPTCP v1 support: - Christoph is working on it: - new version sent during the meeting - Paolo is adding SHA256 support: - Christoph has the patches in his tree - DATA_FIN: - Mat has shared a patch, waiting to be applied - Mat has a second part for that, working on that - optimisation of options in TCP "struct mptcp_options_received": - Peter has shared a patch but waiting for the git tree to stabilized before doing a rebase + split - MAINTAINERS file: - TODO Other WIP items: - Active backup support: - Florian is working on it, patches have been shared - WIP: limit the number of MP_JOIN requests and nb of subflow sockets - we can drop the patch exporting __tcp_poll → TODO when we apply the patches removing the use of this function Patches to apply (by Matth): - 1) DATA_FIN - 2) MPTCPv1 → wait a bit for a review - 3) SendMsg "refactor" → if reviewed - Also the series from Florian (mptcp: support mptcp joins) - And the fix from Davide (option length of mp_capable syn/ack v0) MP_JOIN: - From Paolo: "My [mis?]understanding is that MP_JOIN should not be attempted before the msk is fully established (fourth_ack)" - the idea of fully_established: we validated that we can transfer "MPTCP data" (no middlebox) - we should then accept MP_JOIN even if we are not "fully established" (making sure we don't have nasty middleboxes) RFC6824bis: - will we have an update before the "release" (new RFC)? - yes, the proposed changes should be accepted ML: - it seems that people who don't subscribe to the list cannot send messages - should we allow that? (if there is no spam) - yes but some bugs in the ML software cannot allow them for the moment. Mat is going to look at it Misc.: - https://rule11.tech/the-history-of-multipath-tcp-with-olivier-bonaventure/ - http://vger.kernel.org/~davem/net-next.html Next meeting: - /!\ *Back to Thursday, already this week* - We propose to have the next meeting on Thursday, the 5th of December. - Usual time: 17:00 UTC (9am PST, 6pm CET) - Still open to everyone! - https://annuel2.framapad.org/p/mptcp_upstreaming_20191205 Feel free to comment on these points and propose new ones for the next meeting! Talk to you in 2 days, 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

2 years, 6 months

1
0
0 / 0

Re: MPTCP implementation feedback for RFC6824bis

by Christoph Paasch

Hello Alan, > On Nov 28, 2019, at 8:16 AM, Alan Ford <alan.ford(a)gmail.com> wrote: > > Hi Christoph, > > Thank you for the feedback. Comments inline... > >> On 27 Nov 2019, at 19:29, Christoph Paasch <cpaasch(a)apple.com <mailto:cpaasch@apple.com>> wrote: >> >> Hello, >> >> as I mentioned during the meeting last week in Singapore, the team working on upstreaming MPTCP into the Linux Kernel has some feedback on RFC6824bis that would be good to be factored into the RFC before publication. >> >> Here is the list of comments/changes the team is suggesting: >> >> Section 3.1, page 19, "If B has data to send first,..." >> >> The first phrase states that when A receives a DSS from B it indicates successful delivery of the MP_CAPABLE. However, it is not the DSS but rather the DATA_ACK that indicates this. Digging through some past e-mail exchanges I had with Alan I see that that was indeed the intention. >> >> We are thus suggesting to change this text to: >> If B has data to send first, then the reliable delivery of the ACK+MP_CAPABLE can be inferred by the receipt of this data with a MPTCP Data Sequence Signal (DSS) option that includes a DATA_ACK (Section 3.3). Further, whenever A receives a DATA_ACK from B it is a signal of the reliable delivery of A's MP_CAPABLE. After this, A can send data to B with a regular DSS option. > > That seems entirely logical clarification, and was the intention anyway. A developer probably infers the meaning but clarification is no bad thing. Sounds good! It would be good to clarify this. > >> Section 3.3.1, page 32 & 33, "A data sequence mapping does not need..." >> >> This paragraph states that it is permissive to send a mapping in advance. Late-mapping is specified a bit higher around the sentence >> "Implementations MAY hold onto such unmapped data for a short while in the expectation that a mapping will arrive shortly" >> >> This kind of early/late mapping announcements are difficult to handle in an implementation. The Linux Kernel implementation of multipath-tcp.org <http://multipath-tcp.org/> has always disallowed such kind of mappings. Meaning, whenever a DSS-option is received such that the range specified by the relative subflow-sequence number in the DSS-option and the data-length does not (partially) cover the TCP sequence number of the packet itself, the subflow will be killed with a TCP-RST. The problem around handling such early/late mappings is that it is unclear for how long the stack needs to remember these mappings (in the early-mapping case), or for how long he needs to hold on to the data (in the late-mapping case). >> >> We thus suggest to change this to the following: >> Whenever a DSS-option is received on a packet such that the mapping of the subflow-sequence space does not partially cover the TCP-sequence number of the packet itself, the host MUST discard this mapping and MAY destroy the subflow with a TCP-RST. It should be noted that a DATA_FIN that does not come with data has a relative subflow-sequence number of 0 and thus should be handled separately. > > This one I do have an issue with: > > - It is a technical change > - Wording to this effect has been in the document since pretty much the beginning > - It is a MAY which might as well say “there is no guarantee this would work” The problem with the MAY is that the sender can't really know if the receiver accepts it (more regarding this below) > Most importantly, the replacement text seems not to address this issue at all. If I read it correctly, it says that the data sequence mapping option MUST partially cover the subflow sequence space of the packet itself. But that has nothing to do with late or early mapping, both could partially cover the subflow sequence space and preceding data. > > Can you clarify exactly what you want to permit and prevent, here? Let me try to clarify what exactly we mean with early/late mapping so that we are all on the same terms here: Early mapping: A TCP-segment with sequence-number 1 holds a DSS-option with subflow-sequence number 1001 and data-length 100. This means we need to allocate space to store this DSS-option so that when the TCP-segment with seqno 1001 arrives we can know the mapping. There may be coming more of these DSS-options which all need to be stored in allocated memory. It is unclear what the limit to this is and there is no way to communicate this limit to the sender. Late mapping: The receiver receives data without DSS-options with TCP-sequence 1 to 1001. The corresponding DSS-option however arrives with the TCP-segment with seqno 2001. Here, the receiver needs to hold on to this data, waiting for the TCP-segment with the DSS-option. At one point the receiver needs to drop the data due to memory limits. Again, the sender has no way for knowing what this limit is. When the DSS-option comes together with the corresponding TCP-sequence it is straight-forward to store it together with the data. There are no issues with memory-allocation,... as all of this is accounted together with the announced window (yes, the memory is not counted against the window, but the receiver can foresee the DSS-option overhead when computing what window he should announce). When a receiver gets data without a DSS-option, he can store it for up to 64KB of data as that is the maximum data-level length and the last segment of the 64KB-train could be holding the DSS-option. After that he has to drop the data. When the mapping partially covers the segment it also isn't a problem as the unmapped part can safely be dropped and the mapped part can be passed on to the MPTCP-layer. All of this does not imply that every segment of a mapping needs to hold the DSS-option. Just one of them needs to have it. Does this help? It is all about making things more deterministic as it is very hard for the receiver to handle these late/early mappings. Thanks, Christoph

2 years, 6 months

2
1
0 / 0

MPTCP implementation feedback for RFC6824bis

by Christoph Paasch

Hello, as I mentioned during the meeting last week in Singapore, the team working on upstreaming MPTCP into the Linux Kernel has some feedback on RFC6824bis that would be good to be factored into the RFC before publication. Here is the list of comments/changes the team is suggesting: Section 3.1, page 19, "If B has data to send first,..." The first phrase states that when A receives a DSS from B it indicates successful delivery of the MP_CAPABLE. However, it is not the DSS but rather the DATA_ACK that indicates this. Digging through some past e-mail exchanges I had with Alan I see that that was indeed the intention. We are thus suggesting to change this text to: If B has data to send first, then the reliable delivery of the ACK+MP_CAPABLE can be inferred by the receipt of this data with a MPTCP Data Sequence Signal (DSS) option that includes a DATA_ACK (Section 3.3). Further, whenever A receives a DATA_ACK from B it is a signal of the reliable delivery of A's MP_CAPABLE. After this, A can send data to B with a regular DSS option. Section 3.3.1, page 32 & 33, "A data sequence mapping does not need..." This paragraph states that it is permissive to send a mapping in advance. Late-mapping is specified a bit higher around the sentence "Implementations MAY hold onto such unmapped data for a short while in the expectation that a mapping will arrive shortly" This kind of early/late mapping announcements are difficult to handle in an implementation. The Linux Kernel implementation of multipath-tcp.org <http://multipath-tcp.org/> has always disallowed such kind of mappings. Meaning, whenever a DSS-option is received such that the range specified by the relative subflow-sequence number in the DSS-option and the data-length does not (partially) cover the TCP sequence number of the packet itself, the subflow will be killed with a TCP-RST. The problem around handling such early/late mappings is that it is unclear for how long the stack needs to remember these mappings (in the early-mapping case), or for how long he needs to hold on to the data (in the late-mapping case). We thus suggest to change this to the following: Whenever a DSS-option is received on a packet such that the mapping of the subflow-sequence space does not partially cover the TCP-sequence number of the packet itself, the host MUST discard this mapping and MAY destroy the subflow with a TCP-RST. It should be noted that a DATA_FIN that does not come with data has a relative subflow-sequence number of 0 and thus should be handled separately. Would it be possible to make these adjustments to the RFC? Regards, Christoph (on behalf of the MPTCP upstreaming community)

2 years, 6 months

2
1
0 / 0

2022

2021

2020

2019

2018

2017

mptcp December 2019