sporadic failure of mptcp_join.sh selftest 13

[dcaratti]

While backporting the support for "echo bit" to make ADD_ADDR "reliable", I noticed that some of the kselftests that check for the value of MPTcpExtEchoAdd often fail: I'm attaching pcap captures of the same test (number 13 in the current list), repeated twice on the same kernel.
In mp_join-01-ns1-0-FZiNdH.pcap the test failed as follows:

Created /tmp/tmp.6ll1l3yxsi (size 2 KB) containing data sent by client
Created /tmp/tmp.zo99WX2idt (size 2 KB) containing data sent by server
Capturing traffic for test 1 into mp_join-01-ns1-0-FZiNdH.pcap
dropped privs to tcpdump
tcpdump: listening on any, link-type LINUX_SLL (Linux cooked v1), capture size 65535 bytes
287 packets captured
290 packets received by filter
0 packets dropped by kernel
01 remove single address                syn[ ok ] - synack[ ok ] - ack[ ok ]
                                        add[ ok ] - echo  [fail] got 0 ADD_ADDR echo[s] expected 1
Server ns stats
MPTcpExtMPCapableSYNRX          1                  0.0
MPTcpExtMPCapableACKRX          1                  0.0
MPTcpExtMPTCPRetrans            14                 0.0
MPTcpExtMPJoinSynRx             1                  0.0
MPTcpExtMPJoinAckRx             1                  0.0
MPTcpExtDuplicateData           16                 0.0
MPTcpExtRmSubflow               1                  0.0
Client ns stats
MPTcpExtMPTCPRetrans            16                 0.0
MPTcpExtMPJoinSynAckRx          1                  0.0
MPTcpExtDuplicateData           14                 0.0
MPTcpExtAddAddr                 1                  0.0
MPTcpExtRmAddr                  1                  0.0
                                        rm [ ok ] - sf    [ ok ]

while in the other trace, namely mp_join-01-ns1-0-M8kYtD.pcap, the test passed.
Comparing the 2 captures, it's clear that the "echoed" ADD_ADDR is received correctly only if the peer transmitted it in a "pure ack" when TCP is in the ESTABLISHED state. However, it might happen that TCP subflows have no chance to send a pure ACK until they receive a FIN packet, like it happens in mp_join-01-ns1-0-FZiNdH.pcap. In this case, all the MPTCP options in the packet are currently discarded (here) by Linux.

This patch proved to fix the tests:

--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -6254,6 +6265,9 @@ int tcp_rcv_state_process(struct sock *sk, struct sk_buff *skb)
                         */
                        inet_csk_reset_keepalive_timer(sk, tmo);
                } else {
+                       if (sk_is_mptcp(sk) && TCP_SKB_CB(skb)->seq == TCP_SKB_CB(skb)->end_seq)
+                               mptcp_incoming_options(sk, skb);
+
                        tcp_time_wait(sk, TCP_FIN_WAIT2, tmo);
                        goto discard;
                }

but I'm unsure if we are allowed to do this (e.g. parsing the MPTCP options after sending FIN, and skipping the test of URG bit).

[dcaratti]

issue112.tar.gz

[dcaratti]

I noticed that this behavior becomes more evident on kernel where the TCP "ping pong threshold" is 1. e.g. they don't have this commit:

commit 4a41f453bedfd5e9cd040bad509d9da49feb3e2c
Author: Wei Wang <weiwan@google.com>
Date:   Fri Jan 25 10:53:20 2019 -0800

    tcp: change pingpong threshold to 3

the issue can also be fixed by temporarily exiting the "pingpong" mode, when the ADD_ADDR is received, e.g. with something like:

--- a/net/mptcp/options.c
+++ b/net/mptcp/options.c
@@ -911,6 +922,8 @@ void mptcp_incoming_options(struct sock *sk, struct sk_buff *skb)
                if (!mp_opt.echo) {
                        mptcp_pm_add_addr_received(msk, &addr);
                        MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ADDADDR);
+                      /* printk("silly-fix\n");*/
+                       inet_csk_exit_pingpong_mode(sk);
                } else {
                        mptcp_pm_del_add_timer(msk, &addr);
                        MPTCP_INC_STATS(sock_net(sk), MPTCP_MIB_ECHOADD)

[dcaratti]

A third option might be to ignore the problem in real-life scenarios, and just fix the tests in a way that subflows are created with TCP_QUICKACK socket option for kselftests where the value of MPTcpExtEchoAdd is required to pass the test (but this would require at least implementation of the setsockopt() in the kernel)

[dcaratti]

I still see sporadic failures in the "flush", but I see them independently from whether I keep commit 4a41f45 reverted or not. Hence, the issue can be considered a dup of #139