diff mbox series

[net,v2] selftests/fib_tests: ping from dummy0 in fib_rp_filter_test()

Message ID 20211130004905.4146-1-yepeilin.cs@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series [net,v2] selftests/fib_tests: ping from dummy0 in fib_rp_filter_test() | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch warning WARNING: Unknown commit id 'f455fee41c07', maybe rebased or not pulled? WARNING: line length of 110 exceeds 80 columns WARNING: line length of 111 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Peilin Ye Nov. 30, 2021, 12:49 a.m. UTC 
From: Peilin Ye <peilin.ye@bytedance.com>

Currently rp_filter tests in fib_tests.sh:fib_rp_filter_test() are
failing.  ping sockets are bound to dummy1 using the "-I" option
(SO_BINDTODEVICE), but socket lookup is failing when receiving ping
replies, since the routing table thinks they belong to dummy0.

For example, suppose ping is using a SOCK_RAW socket for ICMP messages.
When receiving ping replies, in __raw_v4_lookup(), sk->sk_bound_dev_if
is 3 (dummy1), but dif (skb_rtable(skb)->rt_iif) says 2 (dummy0), so the
raw_sk_bound_dev_eq() check fails.  Similar things happen in
ping_lookup() for SOCK_DGRAM sockets.

Fix the tests by binding to dummy0 instead.  Redirect ping requests to
dummy1 before redirecting them again to lo, so that sk->sk_bound_dev_if
agrees with our routing table.

These tests used to pass due to a bug [1] in iputils, where "ping -I"
actually did not bind ICMP message sockets to device.  The bug has been
fixed by iputils commit f455fee41c07 ("ping: also bind the ICMP socket
to the specific device") in 2016, which is why our rp_filter tests
started to fail.  See [2] .

Tested with ping from iputils 20210722-41-gf9fb573:

$ ./fib_tests.sh -t rp_filter

IPv4 rp_filter tests
    TEST: rp_filter passes local packets		[ OK ]
    TEST: rp_filter passes loopback packets		[ OK ]

[1] https://github.com/iputils/iputils/issues/55
[2] https://github.com/iputils/iputils/commit/f455fee41c077d4b700a473b2f5b3487b8febc1d

Reported-by: Hangbin Liu <liuhangbin@gmail.com>
Fixes: adb701d6cfa4 ("selftests: add a test case for rp_filter")
Reviewed-by: Cong Wang <cong.wang@bytedance.com>
Signed-off-by: Peilin Ye <peilin.ye@bytedance.com>
---
Change in v2:
    - s/SOCK_ICMP/SOCK_DGRAM/ in commit message

 tools/testing/selftests/net/fib_tests.sh | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

David Ahern Nov. 30, 2021, 1:16 a.m. UTC | #1 
On 11/29/21 5:49 PM, Peilin Ye wrote:
> diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
> index 5abe92d55b69..b8bceae00f8e 100755
> --- a/tools/testing/selftests/net/fib_tests.sh
> +++ b/tools/testing/selftests/net/fib_tests.sh
> @@ -453,15 +453,19 @@ fib_rp_filter_test()
>  	$NS_EXEC sysctl -qw net.ipv4.conf.all.accept_local=1
>  	$NS_EXEC sysctl -qw net.ipv4.conf.all.route_localnet=1
>  
> +	$NS_EXEC tc qd add dev dummy0 parent root handle 1: fq_codel
> +	$NS_EXEC tc filter add dev dummy0 parent 1: protocol arp basic action mirred egress redirect dev dummy1
> +	$NS_EXEC tc filter add dev dummy0 parent 1: protocol ip basic action mirred egress redirect dev dummy1
> +
>  	$NS_EXEC tc qd add dev dummy1 parent root handle 1: fq_codel
>  	$NS_EXEC tc filter add dev dummy1 parent 1: protocol arp basic action mirred egress redirect dev lo
>  	$NS_EXEC tc filter add dev dummy1 parent 1: protocol ip basic action mirred egress redirect dev lo
>  	set +e
>  
> -	run_cmd "ip netns exec ns1 ping -I dummy1 -w1 -c1 198.51.100.1"
> +	run_cmd "ip netns exec ns1 ping -I dummy0 -w1 -c1 198.51.100.1"
>  	log_test $? 0 "rp_filter passes local packets"
>  
> -	run_cmd "ip netns exec ns1 ping -I dummy1 -w1 -c1 127.0.0.1"
> +	run_cmd "ip netns exec ns1 ping -I dummy0 -w1 -c1 127.0.0.1"
>  	log_test $? 0 "rp_filter passes loopback packets"
>  
>  	cleanup
> 

confused by the point of this test if you are going to change dummy1 to
dummy0. dummy0 has 198.51.100.1 assigned to it, so the ping should
always work.
Peilin Ye Nov. 30, 2021, 5:13 a.m. UTC | #2 
Hi David,

On Mon, Nov 29, 2021 at 06:16:48PM -0700, David Ahern wrote:
> On 11/29/21 5:49 PM, Peilin Ye wrote:
> > -	run_cmd "ip netns exec ns1 ping -I dummy1 -w1 -c1 198.51.100.1"
> > +	run_cmd "ip netns exec ns1 ping -I dummy0 -w1 -c1 198.51.100.1"
> >  	log_test $? 0 "rp_filter passes local packets"
> 
> confused by the point of this test if you are going to change dummy1 to
> dummy0. dummy0 has 198.51.100.1 assigned to it, so the ping should
> always work.

Ah...  Thanks for pointing this out, I'll try to figure out a different
way to test it in v3.

Thanks,
Peilin Ye
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index 5abe92d55b69..b8bceae00f8e 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -453,15 +453,19 @@  fib_rp_filter_test()
 	$NS_EXEC sysctl -qw net.ipv4.conf.all.accept_local=1
 	$NS_EXEC sysctl -qw net.ipv4.conf.all.route_localnet=1
 
+	$NS_EXEC tc qd add dev dummy0 parent root handle 1: fq_codel
+	$NS_EXEC tc filter add dev dummy0 parent 1: protocol arp basic action mirred egress redirect dev dummy1
+	$NS_EXEC tc filter add dev dummy0 parent 1: protocol ip basic action mirred egress redirect dev dummy1
+
 	$NS_EXEC tc qd add dev dummy1 parent root handle 1: fq_codel
 	$NS_EXEC tc filter add dev dummy1 parent 1: protocol arp basic action mirred egress redirect dev lo
 	$NS_EXEC tc filter add dev dummy1 parent 1: protocol ip basic action mirred egress redirect dev lo
 	set +e
 
-	run_cmd "ip netns exec ns1 ping -I dummy1 -w1 -c1 198.51.100.1"
+	run_cmd "ip netns exec ns1 ping -I dummy0 -w1 -c1 198.51.100.1"
 	log_test $? 0 "rp_filter passes local packets"
 
-	run_cmd "ip netns exec ns1 ping -I dummy1 -w1 -c1 127.0.0.1"
+	run_cmd "ip netns exec ns1 ping -I dummy0 -w1 -c1 127.0.0.1"
 	log_test $? 0 "rp_filter passes loopback packets"
 
 	cleanup