From: Geliang Tang <[email protected]>
[ Upstream commit 8d014eaa9254a9b8e0841df40dd36782b451579a ]
This patch added the test case for retransmitting ADD_ADDR when timeout
occurs. It set NS1's add_addr_timeout to 1 second, and drop NS2's ADD_ADDR
echo packets.
Here we need to slow down the transfer process of all data to let the
ADD_ADDR suboptions can be retransmitted three times. So we added a new
parameter "speed" for do_transfer, it can be set with fast or slow.
We also added three new optional parameters for run_tests, and dropped
run_remove_tests function.
Since we added the netfilter rules in this test case, we need to update
the "config" file.
Suggested-by: Matthieu Baerts <[email protected]>
Suggested-by: Paolo Abeni <[email protected]>
Acked-by: Paolo Abeni <[email protected]>
Reviewed-by: Matthieu Baerts <[email protected]>
Signed-off-by: Geliang Tang <[email protected]>
Signed-off-by: Mat Martineau <[email protected]>
Signed-off-by: Jakub Kicinski <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
---
tools/testing/selftests/net/mptcp/config | 10 ++
.../testing/selftests/net/mptcp/mptcp_join.sh | 94 ++++++++++++++-----
2 files changed, 80 insertions(+), 24 deletions(-)
diff --git a/tools/testing/selftests/net/mptcp/config b/tools/testing/selftests/net/mptcp/config
index 741a1c4f4ae8..0faaccd21447 100644
--- a/tools/testing/selftests/net/mptcp/config
+++ b/tools/testing/selftests/net/mptcp/config
@@ -5,3 +5,13 @@ CONFIG_INET_DIAG=m
CONFIG_INET_MPTCP_DIAG=m
CONFIG_VETH=y
CONFIG_NET_SCH_NETEM=m
+CONFIG_NETFILTER=y
+CONFIG_NETFILTER_ADVANCED=y
+CONFIG_NETFILTER_NETLINK=m
+CONFIG_NF_TABLES=m
+CONFIG_NFT_COUNTER=m
+CONFIG_NFT_COMPAT=m
+CONFIG_NETFILTER_XTABLES=m
+CONFIG_NETFILTER_XT_MATCH_BPF=m
+CONFIG_NF_TABLES_IPV4=y
+CONFIG_NF_TABLES_IPV6=y
diff --git a/tools/testing/selftests/net/mptcp/mptcp_join.sh b/tools/testing/selftests/net/mptcp/mptcp_join.sh
index 08f53d86dedc..0d93b243695f 100755
--- a/tools/testing/selftests/net/mptcp/mptcp_join.sh
+++ b/tools/testing/selftests/net/mptcp/mptcp_join.sh
@@ -13,6 +13,24 @@ capture=0
TEST_COUNT=0
+# generated using "nfbpf_compile '(ip && (ip[54] & 0xf0) == 0x30) ||
+# (ip6 && (ip6[74] & 0xf0) == 0x30)'"
+CBPF_MPTCP_SUBOPTION_ADD_ADDR="14,
+ 48 0 0 0,
+ 84 0 0 240,
+ 21 0 3 64,
+ 48 0 0 54,
+ 84 0 0 240,
+ 21 6 7 48,
+ 48 0 0 0,
+ 84 0 0 240,
+ 21 0 4 96,
+ 48 0 0 74,
+ 84 0 0 240,
+ 21 0 1 48,
+ 6 0 0 65535,
+ 6 0 0 0"
+
init()
{
capout=$(mktemp)
@@ -82,6 +100,26 @@ reset_with_cookies()
done
}
+reset_with_add_addr_timeout()
+{
+ local ip="${1:-4}"
+ local tables
+
+ tables="iptables"
+ if [ $ip -eq 6 ]; then
+ tables="ip6tables"
+ fi
+
+ reset
+
+ ip netns exec $ns1 sysctl -q net.mptcp.add_addr_timeout=1
+ ip netns exec $ns2 $tables -A OUTPUT -p tcp \
+ -m tcp --tcp-option 30 \
+ -m bpf --bytecode \
+ "$CBPF_MPTCP_SUBOPTION_ADD_ADDR" \
+ -j DROP
+}
+
for arg in "$@"; do
if [ "$arg" = "-c" ]; then
capture=1
@@ -94,6 +132,17 @@ if [ $? -ne 0 ];then
exit $ksft_skip
fi
+iptables -V > /dev/null 2>&1
+if [ $? -ne 0 ];then
+ echo "SKIP: Could not run all tests without iptables tool"
+ exit $ksft_skip
+fi
+
+ip6tables -V > /dev/null 2>&1
+if [ $? -ne 0 ];then
+ echo "SKIP: Could not run all tests without ip6tables tool"
+ exit $ksft_skip
+fi
check_transfer()
{
@@ -135,6 +184,7 @@ do_transfer()
connect_addr="$5"
rm_nr_ns1="$6"
rm_nr_ns2="$7"
+ speed="$8"
port=$((10000+$TEST_COUNT))
TEST_COUNT=$((TEST_COUNT+1))
@@ -159,7 +209,7 @@ do_transfer()
sleep 1
fi
- if [[ $rm_nr_ns1 -eq 0 && $rm_nr_ns2 -eq 0 ]]; then
+ if [ $speed = "fast" ]; then
mptcp_connect="./mptcp_connect -j"
else
mptcp_connect="./mptcp_connect -r"
@@ -250,26 +300,13 @@ run_tests()
listener_ns="$1"
connector_ns="$2"
connect_addr="$3"
+ rm_nr_ns1="${4:-0}"
+ rm_nr_ns2="${5:-0}"
+ speed="${6:-fast}"
lret=0
- do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP ${connect_addr} 0 0
- lret=$?
- if [ $lret -ne 0 ]; then
- ret=$lret
- return
- fi
-}
-
-run_remove_tests()
-{
- listener_ns="$1"
- connector_ns="$2"
- connect_addr="$3"
- rm_nr_ns1="$4"
- rm_nr_ns2="$5"
- lret=0
-
- do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP ${connect_addr} ${rm_nr_ns1} ${rm_nr_ns2}
+ do_transfer ${listener_ns} ${connector_ns} MPTCP MPTCP ${connect_addr} \
+ ${rm_nr_ns1} ${rm_nr_ns2} ${speed}
lret=$?
if [ $lret -ne 0 ]; then
ret=$lret
@@ -491,12 +528,21 @@ run_tests $ns1 $ns2 10.0.1.1
chk_join_nr "multiple subflows and signal" 3 3 3
chk_add_nr 1 1
+# add_addr timeout
+reset_with_add_addr_timeout
+ip netns exec $ns1 ./pm_nl_ctl limits 0 1
+ip netns exec $ns2 ./pm_nl_ctl limits 1 1
+ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
+run_tests $ns1 $ns2 10.0.1.1 0 0 slow
+chk_join_nr "signal address, ADD_ADDR timeout" 1 1 1
+chk_add_nr 4 0
+
# single subflow, remove
reset
ip netns exec $ns1 ./pm_nl_ctl limits 0 1
ip netns exec $ns2 ./pm_nl_ctl limits 0 1
ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
-run_remove_tests $ns1 $ns2 10.0.1.1 0 1
+run_tests $ns1 $ns2 10.0.1.1 0 1 slow
chk_join_nr "remove single subflow" 1 1 1
chk_rm_nr 1 1
@@ -506,7 +552,7 @@ ip netns exec $ns1 ./pm_nl_ctl limits 0 2
ip netns exec $ns2 ./pm_nl_ctl limits 0 2
ip netns exec $ns2 ./pm_nl_ctl add 10.0.2.2 flags subflow
ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
-run_remove_tests $ns1 $ns2 10.0.1.1 0 2
+run_tests $ns1 $ns2 10.0.1.1 0 2 slow
chk_join_nr "remove multiple subflows" 2 2 2
chk_rm_nr 2 2
@@ -515,7 +561,7 @@ reset
ip netns exec $ns1 ./pm_nl_ctl limits 0 1
ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
ip netns exec $ns2 ./pm_nl_ctl limits 1 1
-run_remove_tests $ns1 $ns2 10.0.1.1 1 0
+run_tests $ns1 $ns2 10.0.1.1 1 0 slow
chk_join_nr "remove single address" 1 1 1
chk_add_nr 1 1
chk_rm_nr 0 0
@@ -526,7 +572,7 @@ ip netns exec $ns1 ./pm_nl_ctl limits 0 2
ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
ip netns exec $ns2 ./pm_nl_ctl limits 1 2
ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
-run_remove_tests $ns1 $ns2 10.0.1.1 1 1
+run_tests $ns1 $ns2 10.0.1.1 1 1 slow
chk_join_nr "remove subflow and signal" 2 2 2
chk_add_nr 1 1
chk_rm_nr 1 1
@@ -538,7 +584,7 @@ ip netns exec $ns1 ./pm_nl_ctl add 10.0.2.1 flags signal
ip netns exec $ns2 ./pm_nl_ctl limits 1 3
ip netns exec $ns2 ./pm_nl_ctl add 10.0.3.2 flags subflow
ip netns exec $ns2 ./pm_nl_ctl add 10.0.4.2 flags subflow
-run_remove_tests $ns1 $ns2 10.0.1.1 1 2
+run_tests $ns1 $ns2 10.0.1.1 1 2 slow
chk_join_nr "remove subflows and signal" 3 3 3
chk_add_nr 1 1
chk_rm_nr 2 2
--
2.35.1
Hi Greg, Sasha,
(+ MPTCP upstream ML)
First, thank you again for maintaining the stable branches!
On 05/07/2022 13:58, Greg Kroah-Hartman wrote:
> From: Geliang Tang <[email protected]>
>
> [ Upstream commit 8d014eaa9254a9b8e0841df40dd36782b451579a ]
>
> This patch added the test case for retransmitting ADD_ADDR when timeout
> occurs. It set NS1's add_addr_timeout to 1 second, and drop NS2's ADD_ADDR
> echo packets.
TL;DR: Could it be possible to drop all selftests MPTCP patches from
v5.10 queue please?
I was initially reacting on this patch because it looks like it depends on:
93f323b9cccc ("mptcp: add a new sysctl add_addr_timeout")
and indirectly to:
9ce7deff92e8 ("docs: networking: mptcp: Add MPTCP sysctl entries")
to have "net.mptcp.add_addr_timeout" sysctl knob needed for this new
selftest.
But then I tried to understand why this current patch ("selftests:
mptcp: add ADD_ADDR timeout test case") has been selected for 5.10. I
guess it was to ease the backport of another one, right?
Looking at the 'series' file in 5.10 queue, it seems the new
"selftests-mptcp-more-stable-diag-tests" patch requires 5 other patches:
-> selftests-mptcp-more-stable-diag-tests.patch
-> selftests-mptcp-fix-diag-instability.patch
-> selftests-mptcp-launch-mptcp_connect-with-timeout.patch
-> selftests-mptcp-add-add_addr-ipv6-test-cases.patch
-> selftests-mptcp-add-link-failure-test-case.patch
-> selftests-mptcp-add-add_addr-timeout-test-case.patch
When looking at these patches in more detail, it looks like "selftests:
mptcp: add ADD_ADDR IPv6 test cases" depends on a new feature only
available from v5.11: ADD_ADDR for IPv6.
Could it be possible to drop all these patches from v5.10 then please?
The two recent fixes for the "diag" selftest mainly helps on slow / busy
CI. I think it is not worth backporting them to v5.10.
(Note that if we want "selftests: mptcp: fix diag instability" patch, we
also need 2e580a63b5c2 ("selftests: mptcp: add cfg_do_w for cfg_remove")
and the top part of 8da6229b9524 ("selftests: mptcp: timeout testcases
for multi addresses"): the list starts to be long.)
One last thing: it looks like when Sasha adds patches to a stable queue,
a notification is sent to less people than when Greg adds patches. For
example here, I have not been notified for this patch when added to the
queue while I was one of the reviewers. I already got notifications from
Greg when I was a reviewer on other patches.
Is it normal? Do you only cc people who signed off on the patch?
It looks like you don't cc maintainers from the MAINTAINERS file but
that's probably on purpose. I didn't get cc for all MPTCP patches of the
series here but I guess I can always subscribe to 'stable' ML for that.
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
http://www.tessares.net
On Tue, Jul 05, 2022 at 05:59:22PM +0200, Matthieu Baerts wrote:
> Hi Greg, Sasha,
>
> (+ MPTCP upstream ML)
>
> First, thank you again for maintaining the stable branches!
>
> On 05/07/2022 13:58, Greg Kroah-Hartman wrote:
> > From: Geliang Tang <[email protected]>
> >
> > [ Upstream commit 8d014eaa9254a9b8e0841df40dd36782b451579a ]
> >
> > This patch added the test case for retransmitting ADD_ADDR when timeout
> > occurs. It set NS1's add_addr_timeout to 1 second, and drop NS2's ADD_ADDR
> > echo packets.
> TL;DR: Could it be possible to drop all selftests MPTCP patches from
> v5.10 queue please?
>
>
> I was initially reacting on this patch because it looks like it depends on:
>
> 93f323b9cccc ("mptcp: add a new sysctl add_addr_timeout")
>
> and indirectly to:
>
> 9ce7deff92e8 ("docs: networking: mptcp: Add MPTCP sysctl entries")
>
> to have "net.mptcp.add_addr_timeout" sysctl knob needed for this new
> selftest.
>
> But then I tried to understand why this current patch ("selftests:
> mptcp: add ADD_ADDR timeout test case") has been selected for 5.10. I
> guess it was to ease the backport of another one, right?
> Looking at the 'series' file in 5.10 queue, it seems the new
> "selftests-mptcp-more-stable-diag-tests" patch requires 5 other patches:
>
> -> selftests-mptcp-more-stable-diag-tests.patch
> -> selftests-mptcp-fix-diag-instability.patch
> -> selftests-mptcp-launch-mptcp_connect-with-timeout.patch
> -> selftests-mptcp-add-add_addr-ipv6-test-cases.patch
> -> selftests-mptcp-add-link-failure-test-case.patch
> -> selftests-mptcp-add-add_addr-timeout-test-case.patch
>
>
> When looking at these patches in more detail, it looks like "selftests:
> mptcp: add ADD_ADDR IPv6 test cases" depends on a new feature only
> available from v5.11: ADD_ADDR for IPv6.
>
>
> Could it be possible to drop all these patches from v5.10 then please?
Sure, but leave them in for 5.15.y and 5.18.y?
> The two recent fixes for the "diag" selftest mainly helps on slow / busy
> CI. I think it is not worth backporting them to v5.10.
>
>
> (Note that if we want "selftests: mptcp: fix diag instability" patch, we
> also need 2e580a63b5c2 ("selftests: mptcp: add cfg_do_w for cfg_remove")
> and the top part of 8da6229b9524 ("selftests: mptcp: timeout testcases
> for multi addresses"): the list starts to be long.)
>
>
> One last thing: it looks like when Sasha adds patches to a stable queue,
> a notification is sent to less people than when Greg adds patches. For
> example here, I have not been notified for this patch when added to the
> queue while I was one of the reviewers. I already got notifications from
> Greg when I was a reviewer on other patches.
> Is it normal? Do you only cc people who signed off on the patch?
I cc: everyone on the commit, Sasha should also do that but sometimes
his script acts up.
> It looks like you don't cc maintainers from the MAINTAINERS file but
> that's probably on purpose. I didn't get cc for all MPTCP patches of the
> series here but I guess I can always subscribe to 'stable' ML for that.
No, we don't use the MAINTAINERS file, that would just be too noisy as
ideally who ever the MAINTAINER was, they already saw this as the commit
is already in Linus's tree.
thanks,
greg k-h
On Tue, 5 Jul 2022, Greg Kroah-Hartman wrote:
> On Tue, Jul 05, 2022 at 05:59:22PM +0200, Matthieu Baerts wrote:
>> Hi Greg, Sasha,
>>
>> (+ MPTCP upstream ML)
>>
>> First, thank you again for maintaining the stable branches!
>>
>> On 05/07/2022 13:58, Greg Kroah-Hartman wrote:
>>> From: Geliang Tang <[email protected]>
>>>
>>> [ Upstream commit 8d014eaa9254a9b8e0841df40dd36782b451579a ]
>>>
>>> This patch added the test case for retransmitting ADD_ADDR when timeout
>>> occurs. It set NS1's add_addr_timeout to 1 second, and drop NS2's ADD_ADDR
>>> echo packets.
>> TL;DR: Could it be possible to drop all selftests MPTCP patches from
>> v5.10 queue please?
>>
>>
>> I was initially reacting on this patch because it looks like it depends on:
>>
>> 93f323b9cccc ("mptcp: add a new sysctl add_addr_timeout")
>>
>> and indirectly to:
>>
>> 9ce7deff92e8 ("docs: networking: mptcp: Add MPTCP sysctl entries")
>>
>> to have "net.mptcp.add_addr_timeout" sysctl knob needed for this new
>> selftest.
>>
>> But then I tried to understand why this current patch ("selftests:
>> mptcp: add ADD_ADDR timeout test case") has been selected for 5.10. I
>> guess it was to ease the backport of another one, right?
>> Looking at the 'series' file in 5.10 queue, it seems the new
>> "selftests-mptcp-more-stable-diag-tests" patch requires 5 other patches:
>>
>> -> selftests-mptcp-more-stable-diag-tests.patch
>> -> selftests-mptcp-fix-diag-instability.patch
>> -> selftests-mptcp-launch-mptcp_connect-with-timeout.patch
>> -> selftests-mptcp-add-add_addr-ipv6-test-cases.patch
>> -> selftests-mptcp-add-link-failure-test-case.patch
>> -> selftests-mptcp-add-add_addr-timeout-test-case.patch
>>
>>
>> When looking at these patches in more detail, it looks like "selftests:
>> mptcp: add ADD_ADDR IPv6 test cases" depends on a new feature only
>> available from v5.11: ADD_ADDR for IPv6.
>>
>>
>> Could it be possible to drop all these patches from v5.10 then please?
>
> Sure, but leave them in for 5.15.y and 5.18.y?
>
Hi Greg -
I'm the other MPTCP maintainer, jumping in here due to Matt's time zone.
Yes: leave selftests-mptcp-more-stable-diag-tests.patch in 5.15.y and
5.18.y
--
Mat Martineau
Intel
Hi Greg,
On 05/07/2022 18:31, Greg Kroah-Hartman wrote:
> On Tue, Jul 05, 2022 at 05:59:22PM +0200, Matthieu Baerts wrote:
>> Hi Greg, Sasha,
>>
>> (+ MPTCP upstream ML)
>>
>> First, thank you again for maintaining the stable branches!
>>
>> On 05/07/2022 13:58, Greg Kroah-Hartman wrote:
>>> From: Geliang Tang <[email protected]>
>>>
>>> [ Upstream commit 8d014eaa9254a9b8e0841df40dd36782b451579a ]
>>>
>>> This patch added the test case for retransmitting ADD_ADDR when timeout
>>> occurs. It set NS1's add_addr_timeout to 1 second, and drop NS2's ADD_ADDR
>>> echo packets.
>> TL;DR: Could it be possible to drop all selftests MPTCP patches from
>> v5.10 queue please?
>>
>>
>> I was initially reacting on this patch because it looks like it depends on:
>>
>> 93f323b9cccc ("mptcp: add a new sysctl add_addr_timeout")
>>
>> and indirectly to:
>>
>> 9ce7deff92e8 ("docs: networking: mptcp: Add MPTCP sysctl entries")
>>
>> to have "net.mptcp.add_addr_timeout" sysctl knob needed for this new
>> selftest.
>>
>> But then I tried to understand why this current patch ("selftests:
>> mptcp: add ADD_ADDR timeout test case") has been selected for 5.10. I
>> guess it was to ease the backport of another one, right?
>> Looking at the 'series' file in 5.10 queue, it seems the new
>> "selftests-mptcp-more-stable-diag-tests" patch requires 5 other patches:
>>
>> -> selftests-mptcp-more-stable-diag-tests.patch
>> -> selftests-mptcp-fix-diag-instability.patch
>> -> selftests-mptcp-launch-mptcp_connect-with-timeout.patch
>> -> selftests-mptcp-add-add_addr-ipv6-test-cases.patch
>> -> selftests-mptcp-add-link-failure-test-case.patch
>> -> selftests-mptcp-add-add_addr-timeout-test-case.patch
>>
>>
>> When looking at these patches in more detail, it looks like "selftests:
>> mptcp: add ADD_ADDR IPv6 test cases" depends on a new feature only
>> available from v5.11: ADD_ADDR for IPv6.
>>
>>
>> Could it be possible to drop all these patches from v5.10 then please?
>
> Sure, but leave them in for 5.15.y and 5.18.y?
(@Mat: Thank you for having replied to this part: yes, please leave them
there)
>> The two recent fixes for the "diag" selftest mainly helps on slow / busy
>> CI. I think it is not worth backporting them to v5.10.
>>
>>
>> (Note that if we want "selftests: mptcp: fix diag instability" patch, we
>> also need 2e580a63b5c2 ("selftests: mptcp: add cfg_do_w for cfg_remove")
>> and the top part of 8da6229b9524 ("selftests: mptcp: timeout testcases
>> for multi addresses"): the list starts to be long.)
>>
>>
>> One last thing: it looks like when Sasha adds patches to a stable queue,
>> a notification is sent to less people than when Greg adds patches. For
>> example here, I have not been notified for this patch when added to the
>> queue while I was one of the reviewers. I already got notifications from
>> Greg when I was a reviewer on other patches.
>> Is it normal? Do you only cc people who signed off on the patch?
>
> I cc: everyone on the commit, Sasha should also do that but sometimes
> his script acts up.
All good, thank you!
>> It looks like you don't cc maintainers from the MAINTAINERS file but
>> that's probably on purpose. I didn't get cc for all MPTCP patches of the
>> series here but I guess I can always subscribe to 'stable' ML for that.
>
> No, we don't use the MAINTAINERS file, that would just be too noisy as
> ideally who ever the MAINTAINER was, they already saw this as the commit
> is already in Linus's tree.
I understand, thank you for the explanation.
Cheers,
Matt
--
Tessares | Belgium | Hybrid Access Solutions
http://www.tessares.net