[RFC PATCH net-next] mptcp: drop all sub-options except ADD_ADDR when the echo bit is set

[RFC PATCH net-next] mptcp: drop all sub-options except ADD_ADDR when the echo bit is set

[Davide Caratti]

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(-)

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

Re: [RFC PATCH net-next] mptcp: drop all sub-options except ADD_ADDR when the echo bit is set

[Mat Martineau]

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