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

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

On 11/30/21 5:47 PM, Peilin Ye wrote:
> 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.
> 
> 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] .
> 
> Fixing the tests while keeping everything in one netns turns out to be
> nontrivial.  Rework the tests and build the following topology:
> 
>  ┌─────────────────────────────┐    ┌─────────────────────────────┐
>  │  network namespace 1 (ns1)  │    │  network namespace 2 (ns2)  │
>  │                             │    │                             │
>  │  ┌────┐     ┌─────┐         │    │  ┌─────┐            ┌────┐  │
>  │  │ lo │<───>│veth1│<────────┼────┼─>│veth2│<──────────>│ lo │  │
>  │  └────┘     ├─────┴──────┐  │    │  ├─────┴──────┐     └────┘  │
>  │             │192.0.2.1/24│  │    │  │192.0.2.1/24│             │
>  │             └────────────┘  │    │  └────────────┘             │
>  └─────────────────────────────┘    └─────────────────────────────┘
> 

if the intention of the tests is to validate that rp_filter = 1 works as
designed, then I suggest a simpler test. 2 namespaces, 2 veth pairs.
Request goes through one interface, and the response comes in the other
via routing in ns2. ns1 would see the response coming in the 'wrong'
interface and drops it.

Hi David,

On Wed, Dec 01, 2021 at 11:00:26AM -0700, David Ahern wrote:
> On 11/30/21 5:47 PM, Peilin Ye wrote:
> >  ┌─────────────────────────────┐    ┌─────────────────────────────┐
> >  │  network namespace 1 (ns1)  │    │  network namespace 2 (ns2)  │
> >  │                             │    │                             │
> >  │  ┌────┐     ┌─────┐         │    │  ┌─────┐            ┌────┐  │
> >  │  │ lo │<───>│veth1│<────────┼────┼─>│veth2│<──────────>│ lo │  │
> >  │  └────┘     ├─────┴──────┐  │    │  ├─────┴──────┐     └────┘  │
> >  │             │192.0.2.1/24│  │    │  │192.0.2.1/24│             │
> >  │             └────────────┘  │    │  └────────────┘             │
> >  └─────────────────────────────┘    └─────────────────────────────┘
> 
> if the intention of the tests is to validate that rp_filter = 1 works as
> designed, then I suggest a simpler test. 2 namespaces, 2 veth pairs.
> Request goes through one interface, and the response comes in the other
> via routing in ns2. ns1 would see the response coming in the 'wrong'
> interface and drops it.

Quite the opposite - the goal is to make sure that commit 66f8209547cc
("fib: relax source validation check for loopback packets") _prevents_
packets from being dropped when rp_filter = 1 in this corner case, as I
mentioned in the commit message.

In order to test this corner case, I need a packet that:

  1. was received on lo;
  2. has a local source IP address (other than lo's 127.0.0.1/8, which
     is 192.0.2.1 in this case);
  3. has no dst attached to it (in this case since it was redirected
     from veth).

See __fib_validate_source():

+       dev_match = dev_match || (res.type == RTN_LOCAL &&
+                                 dev == net->loopback_dev);
					      ^^^^^^^^^^^^
This relaxed check only applies to lo, and I do need to redirect packets
from veth ingress to lo ingress in order to trigger this.

Thanks,
Peilin Ye