[Patch net v3 0/2] ipv4: relax source validation check for loopback packets

[Patch net v3 0/2] ipv4: relax source validation check for loopback packets

[Cong Wang]

This patchset fixes a corner case when loopback packets get dropped
by rp_filter when we route them from veth to lo. Patch 1 is the fix
and patch 2 provides a simplified test case for this scenario.

Cong Wang (2):
  fib: relax source validation check for loopback packets
  selftests: add a test case for rp_filter

---
v3: use dummy1 instead of dummy0 in the test case
v2: remove a redundant if check and add a test case

 net/ipv4/fib_frontend.c                  |  5 ++++
 tools/testing/selftests/net/fib_tests.sh | 35 +++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

-- 
2.21.0

[Patch net v3 1/2] fib: relax source validation check for loopback packets

[Cong Wang]

In a rare case where we redirect local packets from veth to lo,
these packets fail to pass the source validation when rp_filter
is turned on, as the tracing shows:

  <...>-311708 [040] ..s1 7951180.957825: fib_table_lookup: table 254 oif 0 iif 1 src 10.53.180.130 dst 10.53.180.130 tos 0 scope 0 flags 0
  <...>-311708 [040] ..s1 7951180.957826: fib_table_lookup_nh: nexthop dev eth0 oif 4 src 10.53.180.130

So, the fib table lookup returns eth0 as the nexthop even though
the packets are local and should be routed to loopback nonetheless,
but they can't pass the dev match check in fib_info_nh_uses_dev()
without this patch.

It should be safe to relax this check for this special case, as
normally packets coming out of loopback device still have skb_dst
so they won't even hit this slow path.

Cc: Julian Anastasov <ja@ssi.bg>
Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/ipv4/fib_frontend.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/ipv4/fib_frontend.c b/net/ipv4/fib_frontend.c
index 317339cd7f03..e8bc939b56dd 100644
--- a/net/ipv4/fib_frontend.c
+++ b/net/ipv4/fib_frontend.c
@@ -388,6 +388,11 @@ static int __fib_validate_source(struct sk_buff *skb, __be32 src, __be32 dst,
 	fib_combine_itag(itag, &res);
 
 	dev_match = fib_info_nh_uses_dev(res.fi, dev);
+	/* This is not common, loopback packets retain skb_dst so normally they
+	 * would not even hit this slow path.
+	 */
+	dev_match = dev_match || (res.type == RTN_LOCAL &&
+				  dev == net->loopback_dev);
 	if (dev_match) {
 		ret = FIB_RES_NHC(res)->nhc_scope >= RT_SCOPE_HOST;
 		return ret;
-- 
2.21.0

[Patch net v3 2/2] selftests: add a test case for rp_filter

[Cong Wang]

Add a test case to simulate the loopback packet case fixed
in the previous patch.

This test gets passed after the fix:

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

Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 tools/testing/selftests/net/fib_tests.sh | 35 +++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/fib_tests.sh b/tools/testing/selftests/net/fib_tests.sh
index 9457aaeae092..4465fc2dae14 100755
--- a/tools/testing/selftests/net/fib_tests.sh
+++ b/tools/testing/selftests/net/fib_tests.sh
@@ -9,12 +9,13 @@ ret=0
 ksft_skip=4
 
 # all tests in this script. Can be overridden with -t option
-TESTS="unregister down carrier nexthop ipv6_rt ipv4_rt ipv6_addr_metric ipv4_addr_metric ipv6_route_metrics ipv4_route_metrics ipv4_route_v6_gw"
+TESTS="unregister down carrier nexthop ipv6_rt ipv4_rt ipv6_addr_metric ipv4_addr_metric ipv6_route_metrics ipv4_route_metrics ipv4_route_v6_gw rp_filter"
 
 VERBOSE=0
 PAUSE_ON_FAIL=no
 PAUSE=no
 IP="ip -netns ns1"
+NS_EXEC="ip netns exec ns1"
 
 log_test()
 {
@@ -433,6 +434,37 @@ fib_carrier_test()
 	fib_carrier_unicast_test
 }
 
+fib_rp_filter_test()
+{
+	echo
+	echo "IPv4 rp_filter tests"
+
+	setup
+
+	set -e
+	$IP link set dev lo address 52:54:00:6a:c7:5e
+	$IP link set dummy0 address 52:54:00:6a:c7:5e
+	$IP link add dummy1 type dummy
+	$IP link set dummy1 address 52:54:00:6a:c7:5e
+	$IP link set dev dummy1 up
+	$NS_EXEC sysctl -qw net.ipv4.conf.all.rp_filter=1
+	$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 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"
+	log_test $? 0 "rp_filter passes local packets"
+
+	run_cmd "ip netns exec ns1 ping -I dummy1 -w1 -c1 127.0.0.1"
+	log_test $? 0 "rp_filter passes loopback packets"
+
+	cleanup
+}
+
 ################################################################################
 # Tests on nexthop spec
 
@@ -1557,6 +1589,7 @@ do
 	fib_unreg_test|unregister)	fib_unreg_test;;
 	fib_down_test|down)		fib_down_test;;
 	fib_carrier_test|carrier)	fib_carrier_test;;
+	fib_rp_filter_test|rp_filter)	fib_rp_filter_test;;
 	fib_nexthop_test|nexthop)	fib_nexthop_test;;
 	ipv6_route_test|ipv6_rt)	ipv6_route_test;;
 	ipv4_route_test|ipv4_rt)	ipv4_route_test;;
-- 
2.21.0

Re: [Patch net v3 2/2] selftests: add a test case for rp_filter

[David Ahern]

On 7/17/19 3:41 PM, Cong Wang wrote:
> Add a test case to simulate the loopback packet case fixed
> in the previous patch.
> 
> This test gets passed after the fix:
> 
> IPv4 rp_filter tests
>     TEST: rp_filter passes local packets                                [ OK ]
>     TEST: rp_filter passes loopback packets                             [ OK ]
> 
> Cc: David Ahern <dsahern@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  tools/testing/selftests/net/fib_tests.sh | 35 +++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
> 

Thanks for adding the test

Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [Patch net v3 1/2] fib: relax source validation check for loopback packets

[David Ahern]

On 7/17/19 3:41 PM, Cong Wang wrote:
> In a rare case where we redirect local packets from veth to lo,
> these packets fail to pass the source validation when rp_filter
> is turned on, as the tracing shows:
> 
>   <...>-311708 [040] ..s1 7951180.957825: fib_table_lookup: table 254 oif 0 iif 1 src 10.53.180.130 dst 10.53.180.130 tos 0 scope 0 flags 0
>   <...>-311708 [040] ..s1 7951180.957826: fib_table_lookup_nh: nexthop dev eth0 oif 4 src 10.53.180.130
> 
> So, the fib table lookup returns eth0 as the nexthop even though
> the packets are local and should be routed to loopback nonetheless,
> but they can't pass the dev match check in fib_info_nh_uses_dev()
> without this patch.
> 
> It should be safe to relax this check for this special case, as
> normally packets coming out of loopback device still have skb_dst
> so they won't even hit this slow path.
> 
> Cc: Julian Anastasov <ja@ssi.bg>
> Cc: David Ahern <dsahern@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  net/ipv4/fib_frontend.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 

Seems ok to me.
Reviewed-by: David Ahern <dsahern@gmail.com>

Re: [Patch net v3 0/2] ipv4: relax source validation check for loopback packets

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Wed, 17 Jul 2019 14:41:57 -0700

> This patchset fixes a corner case when loopback packets get dropped
> by rp_filter when we route them from veth to lo. Patch 1 is the fix
> and patch 2 provides a simplified test case for this scenario.

Series applied, thanks Cong.

Re: [Patch net v3 2/2] selftests: add a test case for rp_filter

[Hangbin Liu]

On Wed, Jul 17, 2019 at 02:41:59PM -0700, Cong Wang wrote:
> Add a test case to simulate the loopback packet case fixed
> in the previous patch.
> 
> This test gets passed after the fix:
> 
> IPv4 rp_filter tests
>     TEST: rp_filter passes local packets                                [ OK ]
>     TEST: rp_filter passes loopback packets                             [ OK ]

Hi Wang Cong,

Have you tried this test recently? I got this test failed for a long time.
Do you have any idea?

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

Task result:
https://datawarehouse.cki-project.org/kcidb/tests/1789355
Task log:
https://s3.us-east-1.amazonaws.com/arr-cki-prod-datawarehouse-public/datawarehouse-public/2021/11/04/401644508/build_x86_64_redhat:1746652818/tests/kselftests_upstream_net/10924465_x86_64_2_resultoutputfile.log
Build log:
https://gitlab.com/redhat/red-hat-ci-tools/kernel/cki-internal-pipelines/cki-trusted-contributors/-/jobs/1746652817/artifacts/browse/artifacts/

Thanks
Hangbin

Re: [Patch net v3 2/2] selftests: add a test case for rp_filter

[Cong Wang]

On Wed, Nov 10, 2021 at 1:18 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>
> On Wed, Jul 17, 2019 at 02:41:59PM -0700, Cong Wang wrote:
> > Add a test case to simulate the loopback packet case fixed
> > in the previous patch.
> >
> > This test gets passed after the fix:
> >
> > IPv4 rp_filter tests
> >     TEST: rp_filter passes local packets                                [ OK ]
> >     TEST: rp_filter passes loopback packets                             [ OK ]
>
> Hi Wang Cong,
>
> Have you tried this test recently? I got this test failed for a long time.
> Do you have any idea?
>
> IPv4 rp_filter tests
>     TEST: rp_filter passes local packets                                [FAIL]
>     TEST: rp_filter passes loopback packets                             [FAIL]

Hm, I think another one also reported this before, IIRC, it is
related to ping version or cmd option. Please look into this if
you can, otherwise I will see if I can reproduce this on my side.

Thanks.

Re: [Patch net v3 2/2] selftests: add a test case for rp_filter

[David Ahern]

On 11/14/21 10:08 PM, Cong Wang wrote:
> On Wed, Nov 10, 2021 at 1:18 AM Hangbin Liu <liuhangbin@gmail.com> wrote:
>>
>> On Wed, Jul 17, 2019 at 02:41:59PM -0700, Cong Wang wrote:
>>> Add a test case to simulate the loopback packet case fixed
>>> in the previous patch.
>>>
>>> This test gets passed after the fix:
>>>
>>> IPv4 rp_filter tests
>>>     TEST: rp_filter passes local packets                                [ OK ]
>>>     TEST: rp_filter passes loopback packets                             [ OK ]
>>
>> Hi Wang Cong,
>>
>> Have you tried this test recently? I got this test failed for a long time.
>> Do you have any idea?
>>
>> IPv4 rp_filter tests
>>     TEST: rp_filter passes local packets                                [FAIL]
>>     TEST: rp_filter passes loopback packets                             [FAIL]
> 
> Hm, I think another one also reported this before, IIRC, it is
> related to ping version or cmd option. Please look into this if
> you can, otherwise I will see if I can reproduce this on my side.
> 

The test does 'ping -I dummy1'. As I recall newer version of ping uses
SO_BINDTODEVICE vs cmsg to specify the device binding. The setsockopt is
stronger and I bet the socket lookup is failing. If that is the case,
the test needs to be fixed because it will never pass again.

Re: [Patch net v3 2/2] selftests: add a test case for rp_filter

[Hangbin Liu]

On Sun, Nov 14, 2021 at 09:08:41PM -0800, Cong Wang wrote:
> > Hi Wang Cong,
> >
> > Have you tried this test recently? I got this test failed for a long time.
> > Do you have any idea?
> >
> > IPv4 rp_filter tests
> >     TEST: rp_filter passes local packets                                [FAIL]
> >     TEST: rp_filter passes loopback packets                             [FAIL]
> 
> Hm, I think another one also reported this before, IIRC, it is
> related to ping version or cmd option. Please look into this if
> you can, otherwise I will see if I can reproduce this on my side.

I tried both iputils-s20180629 and iputils-20210722 on 5.15.0. All tests
failed. Not sure where goes wrong.

Hangbin

Re: [Patch net v3 2/2] selftests: add a test case for rp_filter

[David Ahern]

On 11/16/21 8:19 PM, Hangbin Liu wrote:
> On Sun, Nov 14, 2021 at 09:08:41PM -0800, Cong Wang wrote:
>>> Hi Wang Cong,
>>>
>>> Have you tried this test recently? I got this test failed for a long time.
>>> Do you have any idea?
>>>
>>> IPv4 rp_filter tests
>>>     TEST: rp_filter passes local packets                                [FAIL]
>>>     TEST: rp_filter passes loopback packets                             [FAIL]
>>
>> Hm, I think another one also reported this before, IIRC, it is
>> related to ping version or cmd option. Please look into this if
>> you can, otherwise I will see if I can reproduce this on my side.
> 
> I tried both iputils-s20180629 and iputils-20210722 on 5.15.0. All tests
> failed. Not sure where goes wrong.
> 

no idea. If you have the time can you verify that indeed the failure is
due to socket lookup ... ie., no raw socket found because of the bind to
device setting. Relax that and it should work which is indicative of the
cmsg bind works but SO_BINDTODEVICE does not.

Re: [Patch net v3 2/2] selftests: add a test case for rp_filter

[Cong Wang]

On Tue, Nov 16, 2021 at 8:15 PM David Ahern <dsahern@gmail.com> wrote:
>
> On 11/16/21 8:19 PM, Hangbin Liu wrote:
> > On Sun, Nov 14, 2021 at 09:08:41PM -0800, Cong Wang wrote:
> >>> Hi Wang Cong,
> >>>
> >>> Have you tried this test recently? I got this test failed for a long time.
> >>> Do you have any idea?
> >>>
> >>> IPv4 rp_filter tests
> >>>     TEST: rp_filter passes local packets                                [FAIL]
> >>>     TEST: rp_filter passes loopback packets                             [FAIL]
> >>
> >> Hm, I think another one also reported this before, IIRC, it is
> >> related to ping version or cmd option. Please look into this if
> >> you can, otherwise I will see if I can reproduce this on my side.
> >
> > I tried both iputils-s20180629 and iputils-20210722 on 5.15.0. All tests
> > failed. Not sure where goes wrong.
> >
>
> no idea. If you have the time can you verify that indeed the failure is
> due to socket lookup ... ie., no raw socket found because of the bind to
> device setting. Relax that and it should work which is indicative of the
> cmsg bind works but SO_BINDTODEVICE does not.

My colleague Peilin is now looking into this.

Thanks!

Re: [Patch net v3 2/2] selftests: add a test case for rp_filter

[Hangbin Liu]

On Tue, Nov 23, 2021 at 05:05:14PM -0800, Cong Wang wrote:
> > > On Sun, Nov 14, 2021 at 09:08:41PM -0800, Cong Wang wrote:
> > >>> Hi Wang Cong,
> > >>>
> > >>> Have you tried this test recently? I got this test failed for a long time.
> > >>> Do you have any idea?
> > >>>
> > >>> IPv4 rp_filter tests
> > >>>     TEST: rp_filter passes local packets                                [FAIL]
> > >>>     TEST: rp_filter passes loopback packets                             [FAIL]
> > >>
> > >> Hm, I think another one also reported this before, IIRC, it is
> > >> related to ping version or cmd option. Please look into this if
> > >> you can, otherwise I will see if I can reproduce this on my side.
> > >
> > > I tried both iputils-s20180629 and iputils-20210722 on 5.15.0. All tests
> > > failed. Not sure where goes wrong.
> > >
> >
> > no idea. If you have the time can you verify that indeed the failure is
> > due to socket lookup ... ie., no raw socket found because of the bind to
> > device setting. Relax that and it should work which is indicative of the
> > cmsg bind works but SO_BINDTODEVICE does not.
> 
> My colleague Peilin is now looking into this.

Thanks Wang Cong and Peilin. Sorry I didn't get time to check on this
issue.

Thanks
Hangbin

Re: [Patch net v3 2/2] selftests: add a test case for rp_filter

[Peilin Ye]

Hi all,

On Wed, Nov 24, 2021 at 09:43:08AM +0800, Hangbin Liu wrote:
> On Tue, Nov 23, 2021 at 05:05:14PM -0800, Cong Wang wrote:
> > My colleague Peilin is now looking into this.
> 
> Thanks Wang Cong and Peilin. Sorry I didn't get time to check on this
> issue.

I think I figured out the cause:

There was a bug [1] in ping's -I option, which has been fixed by iputils
commit f455fee41c07 ("ping: also bind the ICMP socket to the specific
device") [2] in 2016.

Before the fix, "ping -I" actually did _not_ bind the ICMP message
socket to device.  It only bound the "probe socket"; see "probe_fd" in
ping4_run().

Now, "ping -I" binds both sockets to device using SO_BINDTODEVICE, and
socket lookup is failing (when receiving ICMP_ECHOREPLY messages) for
our rp_filter test here, as David mentioned earlier.

I'm still thinking about how should we fix the test.  Any ideas?

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

Thanks,
Peilin Ye