| Message ID | bf93c52f5b3e93ad3ba851dd8d76b80adda95e98.1601986828.git.dcaratti@redhat.com |
|---|---|
| State | Accepted, archived |
| Commit | 79519fc3f52055e8e092c0db07f35ff68dd06029 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [net] net: mptcp: make DACK4/DACK8 usage consistent among all subflows | expand |
On Tue, 2020-10-06 at 14:22 +0200, Davide Caratti wrote: > using packetdrill it's possible to observe the same MPTCP DSN being acked > by different subflows with DACK4 and DACK8. This is in contrast with what > specified in RFC8684 §3.3.2: if an MPTCP endpoint transmits a 64-bit wide > DSN, it MUST be acknowledged with a 64-bit wide DACK. Fix 'use_64bit_ack' > variable to make it a property of MPTCP sockets, not TCP subflows. > > Fixes: a0c1d0eafd1e ("mptcp: Use 32-bit DATA_ACK when possible") > Signed-off-by: Davide Caratti <dcaratti@redhat.com> LGTM, thanks Davide! /P
Hi Davide, Paolo, On 06/10/2020 15:07, Paolo Abeni wrote: > On Tue, 2020-10-06 at 14:22 +0200, Davide Caratti wrote: >> using packetdrill it's possible to observe the same MPTCP DSN being acked >> by different subflows with DACK4 and DACK8. This is in contrast with what >> specified in RFC8684 §3.3.2: if an MPTCP endpoint transmits a 64-bit wide >> DSN, it MUST be acknowledged with a 64-bit wide DACK. Fix 'use_64bit_ack' >> variable to make it a property of MPTCP sockets, not TCP subflows. >> >> Fixes: a0c1d0eafd1e ("mptcp: Use 32-bit DATA_ACK when possible") >> Signed-off-by: Davide Caratti <dcaratti@redhat.com> > > LGTM, thanks Davide! Thank you for the patch and the review! @Davide: be careful that this patch doesn't apply as is on -net (or net-next). I had a small conflict when applying it on top of net-next, not at the end of the "export" branch? - 79519fc3f520: net: mptcp: make DACK4/DACK8 usage consistent among all subflows - be078f4e24b5: conflict in t/mptcp-refactor-shutdown-and-close - Results: b74dfa0c4018..9984fc6a69ca Tests + export are in progress! Cheers, Matt
On Tue, 2020-10-06 at 16:03 +0200, Matthieu Baerts wrote: > Hi Davide, Paolo, > > On 06/10/2020 15:07, Paolo Abeni wrote: > > On Tue, 2020-10-06 at 14:22 +0200, Davide Caratti wrote: > > > using packetdrill it's possible to observe the same MPTCP DSN being acked > > > by different subflows with DACK4 and DACK8. This is in contrast with what > > > specified in RFC8684 §3.3.2: if an MPTCP endpoint transmits a 64-bit wide > > > DSN, it MUST be acknowledged with a 64-bit wide DACK. Fix 'use_64bit_ack' > > > variable to make it a property of MPTCP sockets, not TCP subflows. > > > > > > Fixes: a0c1d0eafd1e ("mptcp: Use 32-bit DATA_ACK when possible") > > > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > > > > LGTM, thanks Davide! > > Thank you for the patch and the review! hello Matthieu, thanks for looking at this. > @Davide: be careful that this patch doesn't apply as is on -net (or > net-next). I had a small conflict when applying it on top of net-next, > not at the end of the "export" branch? right, I did that on top of export/20201006T083856 - but it requires some adjustment to apply to "net", because context mismatches in protocol.h. FTR, this is another strange thing I noticed while working on https://github.com/multipath-tcp/mptcp_net-next/issues/94 : after a call to close(), subflow-0 does DSS (DACK4+DATAFIN), while subflow-1 does DSS(DACK8+DATAFIN) :)
Hi Davide, On 06/10/2020 16:30, Davide Caratti wrote: > On Tue, 2020-10-06 at 16:03 +0200, Matthieu Baerts wrote: >> Hi Davide, Paolo, >> >> On 06/10/2020 15:07, Paolo Abeni wrote: >>> On Tue, 2020-10-06 at 14:22 +0200, Davide Caratti wrote: >>>> using packetdrill it's possible to observe the same MPTCP DSN being acked >>>> by different subflows with DACK4 and DACK8. This is in contrast with what >>>> specified in RFC8684 §3.3.2: if an MPTCP endpoint transmits a 64-bit wide >>>> DSN, it MUST be acknowledged with a 64-bit wide DACK. Fix 'use_64bit_ack' >>>> variable to make it a property of MPTCP sockets, not TCP subflows. >>>> >>>> Fixes: a0c1d0eafd1e ("mptcp: Use 32-bit DATA_ACK when possible") >>>> Signed-off-by: Davide Caratti <dcaratti@redhat.com> >>> >>> LGTM, thanks Davide! >> >> Thank you for the patch and the review! > > hello Matthieu, thanks for looking at this. > >> @Davide: be careful that this patch doesn't apply as is on -net (or >> net-next). I had a small conflict when applying it on top of net-next, >> not at the end of the "export" branch? > > right, I did that on top of export/20201006T083856 - but it requires > some adjustment to apply to "net", because context mismatches in > protocol.h. It looks like if you cherry-pick your patch that is going to be in the next "export" branch (or 79519fc3f520 from the TopGit tree), you will not have any conflicts! :-) > FTR, this is another strange thing I noticed while working on > https://github.com/multipath-tcp/mptcp_net-next/issues/94 : after a call > to close(), subflow-0 does DSS (DACK4+DATAFIN), while subflow-1 does > DSS(DACK8+DATAFIN) :) Good catch! Always good to play with packetdrill to fix new bugs :-) Cheers, Matt
Hi Davide, On 06/10/2020 16:03, Matthieu Baerts wrote: > Hi Davide, Paolo, > > On 06/10/2020 15:07, Paolo Abeni wrote: >> On Tue, 2020-10-06 at 14:22 +0200, Davide Caratti wrote: >>> using packetdrill it's possible to observe the same MPTCP DSN being >>> acked >>> by different subflows with DACK4 and DACK8. This is in contrast with >>> what >>> specified in RFC8684 §3.3.2: if an MPTCP endpoint transmits a 64-bit >>> wide >>> DSN, it MUST be acknowledged with a 64-bit wide DACK. Fix >>> 'use_64bit_ack' >>> variable to make it a property of MPTCP sockets, not TCP subflows. >>> >>> Fixes: a0c1d0eafd1e ("mptcp: Use 32-bit DATA_ACK when possible") >>> Signed-off-by: Davide Caratti <dcaratti@redhat.com> >> >> LGTM, thanks Davide! > > Thank you for the patch and the review! > > @Davide: be careful that this patch doesn't apply as is on -net (or > net-next). I had a small conflict when applying it on top of net-next, > not at the end of the "export" branch? > > - 79519fc3f520: net: mptcp: make DACK4/DACK8 usage consistent among all > subflows > - be078f4e24b5: conflict in t/mptcp-refactor-shutdown-and-close > - Results: b74dfa0c4018..9984fc6a69ca > > Tests + export are in progress! It looks unrelated but one selftest -- mptcp_join.sh, test 11 -- was failing with a debug kernel: # selftests: net/mptcp: mptcp_join.sh # Created /tmp/tmp.TXFPMQuIYt (size 1 KB) containing data sent by client # Created /tmp/tmp.rZmBg6Ql4I (size 1 KB) containing data sent by server # 01 no JOIN syn[ ok ] - synack[ ok ] - ack[ ok ] # 02 single subflow, limited by client syn[ ok ] - synack[ ok ] - ack[ ok ] # 03 single subflow, limited by server syn[ ok ] - synack[ ok ] - ack[ ok ] # 04 single subflow syn[ ok ] - synack[ ok ] - ack[ ok ] # 05 multiple subflows syn[ ok ] - synack[ ok ] - ack[ ok ] # 06 multiple subflows, limited by server syn[ ok ] - synack[ ok ] - ack[ ok ] # 07 unused signal address syn[ ok ] - synack[ ok ] - ack[ ok ] # add[ ok ] - echo [ ok ] # 08 signal address syn[ ok ] - synack[ ok ] - ack[ ok ] # add[ ok ] - echo [ ok ] # 09 subflow and signal syn[ ok ] - synack[ ok ] - ack[ ok ] # add[ ok ] - echo [ ok ] # 10 multiple subflows and signal syn[ ok ] - synack[ ok ] - ack[ ok ] # add[ ok ] - echo [ ok ] # 11 signal address, ADD_ADDR timeout syn[ ok ] - synack[ ok ] - ack[ ok ] # add[fail] got 2 ADD_ADDR[s] expected 4 # - echo [ ok ] # Server ns stats # MPTcpExtMPCapableSYNRX 1 0.0 # MPTcpExtMPCapableACKRX 1 0.0 # MPTcpExtMPTCPRetrans 1 0.0 # MPTcpExtMPJoinSynRx 1 0.0 # MPTcpExtMPJoinAckRx 1 0.0 # MPTcpExtDuplicateData 1 0.0 # Client ns stats # MPTcpExtMPTCPRetrans 1 0.0 # MPTcpExtMPJoinSynAckRx 1 0.0 # MPTcpExtDuplicateData 1 0.0 # MPTcpExtAddAddr 2 0.0 # 12 remove single subflow syn[ ok ] - synack[ ok ] - ack[ ok ] # rm [ ok ] - sf [ ok ] # 13 remove multiple subflows syn[ ok ] - synack[ ok ] - ack[ ok ] # rm [ ok ] - sf [ ok ] # 14 remove single address syn[ ok ] - synack[ ok ] - ack[ ok ] # add[ ok ] - echo [ ok ] # rm [ ok ] - sf [ ok ] # 15 remove subflow and signal syn[ ok ] - synack[ ok ] - ack[ ok ] # add[ ok ] - echo [ ok ] # rm [ ok ] - sf [ ok ] # 16 remove subflows and signal syn[ ok ] - synack[ ok ] - ack[ ok ] # add[ ok ] - echo [ ok ] # rm [ ok ] - sf [ ok ] # 17 single subflow with syn cookies syn[ ok ] - synack[ ok ] - ack[ ok ] # 18 multiple subflows with syn cookies syn[ ok ] - synack[ ok ] - ack[ ok ] # 19 subflows limited by server w cookies syn[ ok ] - synack[ ok ] - ack[ ok ] # 20 signal address with syn cookies syn[ ok ] - synack[ ok ] - ack[ ok ] # add[ ok ] - echo [ ok ] # 21 subflow and signal w cookies syn[ ok ] - synack[ ok ] - ack[ ok ] # add[ ok ] - echo [ ok ] # 22 subflows and signal w. cookies syn[ ok ] - synack[ ok ] - ack[ ok ] # add[ ok ] - echo [ ok ] not ok 3 selftests: net/mptcp: mptcp_join.sh # exit=1 I don't think it is due to your modification. I didn't have the issue without the debug kconfig. Cheers, Matt
On Tue, 2020-10-06 at 18:35 +0200, Matthieu Baerts wrote: > Hi Davide, > > On 06/10/2020 16:03, Matthieu Baerts wrote: > > Hi Davide, Paolo, > > > > On 06/10/2020 15:07, Paolo Abeni wrote: > > > On Tue, 2020-10-06 at 14:22 +0200, Davide Caratti wrote: > > > > using packetdrill it's possible to observe the same MPTCP DSN being > > > > acked > > > > by different subflows with DACK4 and DACK8. This is in contrast with > > > > what > > > > specified in RFC8684 §3.3.2: if an MPTCP endpoint transmits a 64-bit > > > > wide > > > > DSN, it MUST be acknowledged with a 64-bit wide DACK. Fix > > > > 'use_64bit_ack' > > > > variable to make it a property of MPTCP sockets, not TCP subflows. > > > > > > > > Fixes: a0c1d0eafd1e ("mptcp: Use 32-bit DATA_ACK when possible") > > > > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > > > > > > LGTM, thanks Davide! > > > > Thank you for the patch and the review! > > > > @Davide: be careful that this patch doesn't apply as is on -net (or > > net-next). I had a small conflict when applying it on top of net-next, > > not at the end of the "export" branch? > > > > - 79519fc3f520: net: mptcp: make DACK4/DACK8 usage consistent among all > > subflows > > - be078f4e24b5: conflict in t/mptcp-refactor-shutdown-and-close > > - Results: b74dfa0c4018..9984fc6a69ca > > > > Tests + export are in progress! > > It looks unrelated but one selftest -- mptcp_join.sh, test 11 -- was > failing with a debug kernel: > > > # selftests: net/mptcp: mptcp_join.sh > # Created /tmp/tmp.TXFPMQuIYt (size 1 KB) containing data sent by client > # Created /tmp/tmp.rZmBg6Ql4I (size 1 KB) containing data sent by server > # 01 no JOIN syn[ ok ] - synack[ ok ] - > ack[ ok ] > # 02 single subflow, limited by client syn[ ok ] - synack[ ok ] - > ack[ ok ] > # 03 single subflow, limited by server syn[ ok ] - synack[ ok ] - > ack[ ok ] > # 04 single subflow syn[ ok ] - synack[ ok ] - > ack[ ok ] > # 05 multiple subflows syn[ ok ] - synack[ ok ] - > ack[ ok ] > # 06 multiple subflows, limited by server syn[ ok ] - synack[ ok ] - > ack[ ok ] > # 07 unused signal address syn[ ok ] - synack[ ok ] - > ack[ ok ] > # add[ ok ] - echo [ ok ] > # 08 signal address syn[ ok ] - synack[ ok ] - > ack[ ok ] > # add[ ok ] - echo [ ok ] > # 09 subflow and signal syn[ ok ] - synack[ ok ] - > ack[ ok ] > # add[ ok ] - echo [ ok ] > # 10 multiple subflows and signal syn[ ok ] - synack[ ok ] - > ack[ ok ] > # add[ ok ] - echo [ ok ] > # 11 signal address, ADD_ADDR timeout syn[ ok ] - synack[ ok ] - > ack[ ok ] > # add[fail] got 2 ADD_ADDR[s] > expected 4 > # - echo [ ok ] > # Server ns stats > # MPTcpExtMPCapableSYNRX 1 0.0 > # MPTcpExtMPCapableACKRX 1 0.0 > # MPTcpExtMPTCPRetrans 1 0.0 > # MPTcpExtMPJoinSynRx 1 0.0 > # MPTcpExtMPJoinAckRx 1 0.0 > # MPTcpExtDuplicateData 1 0.0 > # Client ns stats > # MPTcpExtMPTCPRetrans 1 0.0 > # MPTcpExtMPJoinSynAckRx 1 0.0 > # MPTcpExtDuplicateData 1 0.0 > # MPTcpExtAddAddr 2 0.0 > # 12 remove single subflow syn[ ok ] - synack[ ok ] - > ack[ ok ] > # rm [ ok ] - sf [ ok ] > # 13 remove multiple subflows syn[ ok ] - synack[ ok ] - > ack[ ok ] > # rm [ ok ] - sf [ ok ] > # 14 remove single address syn[ ok ] - synack[ ok ] - > ack[ ok ] > # add[ ok ] - echo [ ok ] > # rm [ ok ] - sf [ ok ] > # 15 remove subflow and signal syn[ ok ] - synack[ ok ] - > ack[ ok ] > # add[ ok ] - echo [ ok ] > # rm [ ok ] - sf [ ok ] > # 16 remove subflows and signal syn[ ok ] - synack[ ok ] - > ack[ ok ] > # add[ ok ] - echo [ ok ] > # rm [ ok ] - sf [ ok ] > # 17 single subflow with syn cookies syn[ ok ] - synack[ ok ] - > ack[ ok ] > # 18 multiple subflows with syn cookies syn[ ok ] - synack[ ok ] - > ack[ ok ] > # 19 subflows limited by server w cookies syn[ ok ] - synack[ ok ] - > ack[ ok ] > # 20 signal address with syn cookies syn[ ok ] - synack[ ok ] - > ack[ ok ] > # add[ ok ] - echo [ ok ] > # 21 subflow and signal w cookies syn[ ok ] - synack[ ok ] - > ack[ ok ] > # add[ ok ] - echo [ ok ] > # 22 subflows and signal w. cookies syn[ ok ] - synack[ ok ] - > ack[ ok ] > # add[ ok ] - echo [ ok ] > not ok 3 selftests: net/mptcp: mptcp_join.sh # exit=1 > > > I don't think it is due to your modification. I didn't have the issue > without the debug kconfig. uhmm... that self-test is failing here, too... ... just because my VM don't have iptables (required by said test:) iptables is dead! long live to nftables!!! :))) /P
diff --git a/net/mptcp/options.c b/net/mptcp/options.c index fc9e114ba1e3..d601dd1ebe5e 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -517,7 +517,7 @@ static bool mptcp_established_options_dss(struct sock *sk, struct sk_buff *skb, return ret; } - if (subflow->use_64bit_ack) { + if (READ_ONCE(msk->use_64bit_ack)) { ack_size = TCPOLEN_MPTCP_DSS_ACK64; opts->ext_copy.data_ack = READ_ONCE(msk->ack_seq); opts->ext_copy.ack64 = 1; diff --git a/net/mptcp/protocol.h b/net/mptcp/protocol.h index c131810adf42..746413ab4f18 100644 --- a/net/mptcp/protocol.h +++ b/net/mptcp/protocol.h @@ -222,6 +222,7 @@ struct mptcp_sock { bool fully_established; bool rcv_data_fin; bool snd_data_fin_enable; + bool use_64bit_ack; /* Set when we received a 64-bit DSN */ spinlock_t join_list_lock; struct work_struct work; struct sk_buff *ooo_last_skb; @@ -352,7 +353,6 @@ struct mptcp_subflow_context { mpc_map : 1, backup : 1, rx_eof : 1, - use_64bit_ack : 1, /* Set when we received a 64-bit DSN */ can_ack : 1, /* only after processing the remote a key */ disposable : 1; /* ctx can be free at ulp release time */ enum mptcp_data_avail data_avail; diff --git a/net/mptcp/subflow.c b/net/mptcp/subflow.c index ae5fa5b3d42b..00040dee323e 100644 --- a/net/mptcp/subflow.c +++ b/net/mptcp/subflow.c @@ -770,12 +770,11 @@ static enum mapping_status get_mapping_status(struct sock *ssk, if (!mpext->dsn64) { map_seq = expand_seq(subflow->map_seq, subflow->map_data_len, mpext->data_seq); - subflow->use_64bit_ack = 0; pr_debug("expanded seq=%llu", subflow->map_seq); } else { map_seq = mpext->data_seq; - subflow->use_64bit_ack = 1; } + WRITE_ONCE(mptcp_sk(subflow->conn)->use_64bit_ack, !!mpext->dsn64); if (subflow->map_valid) { /* Allow replacing only with an identical map */
using packetdrill it's possible to observe the same MPTCP DSN being acked by different subflows with DACK4 and DACK8. This is in contrast with what specified in RFC8684 §3.3.2: if an MPTCP endpoint transmits a 64-bit wide DSN, it MUST be acknowledged with a 64-bit wide DACK. Fix 'use_64bit_ack' variable to make it a property of MPTCP sockets, not TCP subflows. Fixes: a0c1d0eafd1e ("mptcp: Use 32-bit DATA_ACK when possible") Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- net/mptcp/options.c | 2 +- net/mptcp/protocol.h | 2 +- net/mptcp/subflow.c | 3 +-- 3 files changed, 3 insertions(+), 4 deletions(-)