| Message ID | aeb4b7f3044e2f869ae026f950eb70c4398dad63.1616620119.git.dcaratti@redhat.com |
|---|---|
| State | Accepted, archived |
| Commit | aa44e0298e25fd6ac95e86250431bcc355fb4fc7 |
| Delegated to: | Matthieu Baerts |
| Headers | show |
| Series | [RFC,net-next] mptcp: drop all sub-options except ADD_ADDR when the echo bit is set | expand |
On Wed, 24 Mar 2021, Davide Caratti wrote: > Current Linux carries echo-ed ADD_ADDR over pure TCP ACKs, so there is no > need to add a DSS element that would fit only ADD_ADDR with IPv4 address. > Drop the DSS from echo-ed ADD_ADDR, regardless of the IP version. > > Signed-off-by: Davide Caratti <dcaratti@redhat.com> > --- > > Notes: > hello, > > maybe this is a known issue, maybe not. In case it is, TLDR: > > while working on packetdrill with REMOVE_ADDR, I noticed that the following > inbound packet: > > +0.0 < . 1:1(0) ack 101 win 256 <nop, nop, TS val 705 ecr 305, add_address addr[saddr] hmac=auto> > > had wrong length TCPOLEN_ADD_ADDR_V4_HMAC (18 Byte) in the IPv6 case, so the > address carried in the option was something that probably can't be used by the > client, because it doesn't belong to the same address family of the source > address used by the (client) kernel socket. > > So, I wrote a fix for packetdrill and now it selects the correct ADD_ADDR length > both for IPv4 and IPv6 inbound packets. > > However, matching the subsequent outbound echo-ed ADD_ADDR became problematic: > the IPv4 echoed ADD_ADDR has a trailing DSS option that's not existent in the > IPv6 equivalent of the test (because it does not fit the max TCP option space). > Is there a reason for not saving the DSS option space in the IPv4 case? If not, > can we harmonize at least echo-ed IPv4 and IPv6 ADD_ADDRs? > > any feedback appreciated, thank you in advance! Only reason I can think of to include the DSS is to carry the MPTCP-level ACK, since a previous MPTCP-level ACK could have been lost if it was carried on a pure TCP ack. I don't think we gain a lot from adding the DSS option to these ADD_ADDR echoes, because: * the MPTCP-level ACK is very likely a duplicate * the protocol will recover from a lost ACK * ADD_ADDR echoes are not sent very often I'm ok with skipping the DSS option in this case. Mat > net/mptcp/options.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/net/mptcp/options.c b/net/mptcp/options.c > index c7eb61d0564c..d51c3ad54d9a 100644 > --- a/net/mptcp/options.c > +++ b/net/mptcp/options.c > @@ -624,7 +624,8 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff * > int len; > > if ((mptcp_pm_should_add_signal_ipv6(msk) || > - mptcp_pm_should_add_signal_port(msk)) && > + mptcp_pm_should_add_signal_port(msk) || > + mptcp_pm_should_add_signal_echo(msk)) && > skb && skb_is_tcp_pure_ack(skb)) { > pr_debug("drop other suboptions"); > opts->suboptions = 0; > -- > 2.30.2 -- Mat Martineau Intel
diff --git a/net/mptcp/options.c b/net/mptcp/options.c index c7eb61d0564c..d51c3ad54d9a 100644 --- a/net/mptcp/options.c +++ b/net/mptcp/options.c @@ -624,7 +624,8 @@ static bool mptcp_established_options_add_addr(struct sock *sk, struct sk_buff * int len; if ((mptcp_pm_should_add_signal_ipv6(msk) || - mptcp_pm_should_add_signal_port(msk)) && + mptcp_pm_should_add_signal_port(msk) || + mptcp_pm_should_add_signal_echo(msk)) && skb && skb_is_tcp_pure_ack(skb)) { pr_debug("drop other suboptions"); opts->suboptions = 0;
Current Linux carries echo-ed ADD_ADDR over pure TCP ACKs, so there is no need to add a DSS element that would fit only ADD_ADDR with IPv4 address. Drop the DSS from echo-ed ADD_ADDR, regardless of the IP version. Signed-off-by: Davide Caratti <dcaratti@redhat.com> --- Notes: hello, maybe this is a known issue, maybe not. In case it is, TLDR: while working on packetdrill with REMOVE_ADDR, I noticed that the following inbound packet: +0.0 < . 1:1(0) ack 101 win 256 <nop, nop, TS val 705 ecr 305, add_address addr[saddr] hmac=auto> had wrong length TCPOLEN_ADD_ADDR_V4_HMAC (18 Byte) in the IPv6 case, so the address carried in the option was something that probably can't be used by the client, because it doesn't belong to the same address family of the source address used by the (client) kernel socket. So, I wrote a fix for packetdrill and now it selects the correct ADD_ADDR length both for IPv4 and IPv6 inbound packets. However, matching the subsequent outbound echo-ed ADD_ADDR became problematic: the IPv4 echoed ADD_ADDR has a trailing DSS option that's not existent in the IPv6 equivalent of the test (because it does not fit the max TCP option space). Is there a reason for not saving the DSS option space in the IPv4 case? If not, can we harmonize at least echo-ed IPv4 and IPv6 ADD_ADDRs? any feedback appreciated, thank you in advance! davide net/mptcp/options.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)