| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4f49a99a-bd46-ef77-036c-81b308f2ee92@linux.dev>
Date: Wed, 4 Oct 2023 15:02:45 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Martynas Pumputis <m@...bda.lt>
Cc: Daniel Borkmann <daniel@...earbox.net>, netdev@...r.kernel.org,
Nikolay Aleksandrov <razor@...ckwall.org>, bpf@...r.kernel.org
Subject: Re: [PATCH bpf v2 2/2] selftests/bpf: Add BPF_FIB_LOOKUP_SET_SRC
tests
On 10/3/23 12:10 AM, Martynas Pumputis wrote:
> This patch extends the existing fib_lookup test suite by adding two test
> cases (for each IP family):
>
> * Test source IP selection when default route is used.
It will be helpful to reword "default route". I was looking in the patch for a
new route addition like "default via xxx". I think the test is reusing the
existing prefix route "/24" and "/64". This is to test the address selection
from the dev.
> * Test source IP selection when an IP route has a preferred src IP addr.
>
> Signed-off-by: Martynas Pumputis <m@...bda.lt>
> ---
> .../selftests/bpf/prog_tests/fib_lookup.c | 76 +++++++++++++++++--
> 1 file changed, 70 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/fib_lookup.c b/tools/testing/selftests/bpf/prog_tests/fib_lookup.c
> index 2fd05649bad1..1b0ab1dbd4f1 100644
> --- a/tools/testing/selftests/bpf/prog_tests/fib_lookup.c
> +++ b/tools/testing/selftests/bpf/prog_tests/fib_lookup.c
> @@ -11,9 +11,13 @@
>
> #define NS_TEST "fib_lookup_ns"
> #define IPV6_IFACE_ADDR "face::face"
> +#define IPV6_IFACE_ADDR_SEC "cafe::cafe"
SEC stands for secondary?
> +#define IPV6_ADDR_DST "face::3"
> #define IPV6_NUD_FAILED_ADDR "face::1"
> #define IPV6_NUD_STALE_ADDR "face::2"
> #define IPV4_IFACE_ADDR "10.0.0.254"
> +#define IPV4_IFACE_ADDR_SEC "10.1.0.254"
> +#define IPV4_ADDR_DST "10.2.0.254"
> #define IPV4_NUD_FAILED_ADDR "10.0.0.1"
> #define IPV4_NUD_STALE_ADDR "10.0.0.2"
> #define IPV4_TBID_ADDR "172.0.0.254"
> @@ -31,6 +35,8 @@ struct fib_lookup_test {
> const char *desc;
> const char *daddr;
> int expected_ret;
> + const char *expected_ipv4_src;
> + const char *expected_ipv6_src;
Instead of two members, can it be one "expected_src" member which is v4/v6
agnoastic (similar to the "daddr" above)? The logic needs to be a little smarter
for one time but the future test additions will be easier and less error prone.
> int lookup_flags;
> __u32 tbid;
> __u8 dmac[6];
> @@ -69,6 +75,22 @@ static const struct fib_lookup_test tests[] = {
> .daddr = IPV6_TBID_DST, .expected_ret = BPF_FIB_LKUP_RET_SUCCESS,
> .lookup_flags = BPF_FIB_LOOKUP_DIRECT | BPF_FIB_LOOKUP_TBID, .tbid = 100,
> .dmac = DMAC_INIT2, },
> + { .desc = "IPv4 set src addr",
> + .daddr = IPV4_NUD_FAILED_ADDR, .expected_ret = BPF_FIB_LKUP_RET_SUCCESS,
> + .expected_ipv4_src = IPV4_IFACE_ADDR,
> + .lookup_flags = BPF_FIB_LOOKUP_SET_SRC | BPF_FIB_LOOKUP_SKIP_NEIGH, },
> + { .desc = "IPv6 set src addr",
> + .daddr = IPV6_NUD_FAILED_ADDR, .expected_ret = BPF_FIB_LKUP_RET_SUCCESS,
> + .expected_ipv6_src = IPV6_IFACE_ADDR,
> + .lookup_flags = BPF_FIB_LOOKUP_SET_SRC | BPF_FIB_LOOKUP_SKIP_NEIGH, },
> + { .desc = "IPv4 set prefsrc addr from route",
> + .daddr = IPV4_ADDR_DST, .expected_ret = BPF_FIB_LKUP_RET_SUCCESS,
> + .expected_ipv4_src = IPV4_IFACE_ADDR_SEC,
> + .lookup_flags = BPF_FIB_LOOKUP_SET_SRC | BPF_FIB_LOOKUP_SKIP_NEIGH, },
> + { .desc = "IPv6 set prefsrc addr route",
> + .daddr = IPV6_ADDR_DST, .expected_ret = BPF_FIB_LKUP_RET_SUCCESS,
> + .expected_ipv6_src = IPV6_IFACE_ADDR_SEC,
> + .lookup_flags = BPF_FIB_LOOKUP_SET_SRC | BPF_FIB_LOOKUP_SKIP_NEIGH, },
> };
>
> static int ifindex;
> @@ -97,6 +119,13 @@ static int setup_netns(void)
> SYS(fail, "ip neigh add %s dev veth1 nud failed", IPV4_NUD_FAILED_ADDR);
> SYS(fail, "ip neigh add %s dev veth1 lladdr %s nud stale", IPV4_NUD_STALE_ADDR, DMAC);
>
> + /* Setup for prefsrc IP addr selection */
> + SYS(fail, "ip addr add %s/24 dev veth1", IPV4_IFACE_ADDR_SEC);
> + SYS(fail, "ip route add %s/32 dev veth1 src %s", IPV4_ADDR_DST, IPV4_IFACE_ADDR_SEC);
> +
> + SYS(fail, "ip addr add %s/64 dev veth1 nodad", IPV6_IFACE_ADDR_SEC);
> + SYS(fail, "ip route add %s/128 dev veth1 src %s", IPV6_ADDR_DST, IPV6_IFACE_ADDR_SEC);
> +
> /* Setup for tbid lookup tests */
> SYS(fail, "ip addr add %s/24 dev veth2", IPV4_TBID_ADDR);
> SYS(fail, "ip route del %s/24 dev veth2", IPV4_TBID_NET);
> @@ -133,9 +162,12 @@ static int set_lookup_params(struct bpf_fib_lookup *params, const struct fib_loo
>
> if (inet_pton(AF_INET6, test->daddr, params->ipv6_dst) == 1) {
> params->family = AF_INET6;
> - ret = inet_pton(AF_INET6, IPV6_IFACE_ADDR, params->ipv6_src);
> - if (!ASSERT_EQ(ret, 1, "inet_pton(IPV6_IFACE_ADDR)"))
> - return -1;
> + if (!(test->lookup_flags & BPF_FIB_LOOKUP_SET_SRC)) {
> + ret = inet_pton(AF_INET6, IPV6_IFACE_ADDR, params->ipv6_src);
> + if (!ASSERT_EQ(ret, 1, "inet_pton(IPV6_IFACE_ADDR)"))
> + return -1;
> + }
> +
> return 0;
> }
>
> @@ -143,9 +175,12 @@ static int set_lookup_params(struct bpf_fib_lookup *params, const struct fib_loo
> if (!ASSERT_EQ(ret, 1, "convert IP[46] address"))
> return -1;
> params->family = AF_INET;
> - ret = inet_pton(AF_INET, IPV4_IFACE_ADDR, ¶ms->ipv4_src);
> - if (!ASSERT_EQ(ret, 1, "inet_pton(IPV4_IFACE_ADDR)"))
> - return -1;
> +
> + if (!(test->lookup_flags & BPF_FIB_LOOKUP_SET_SRC)) {
> + ret = inet_pton(AF_INET, IPV4_IFACE_ADDR, ¶ms->ipv4_src);
> + if (!ASSERT_EQ(ret, 1, "inet_pton(IPV4_IFACE_ADDR)"))
> + return -1;
> + }
>
> return 0;
> }
> @@ -207,6 +242,35 @@ void test_fib_lookup(void)
> ASSERT_EQ(skel->bss->fib_lookup_ret, tests[i].expected_ret,
> "fib_lookup_ret");
>
> + if (tests[i].expected_ipv4_src) {
> + __be32 expected_ipv4_src;
> +
> + ret = inet_pton(AF_INET, tests[i].expected_ipv4_src,
> + &expected_ipv4_src);
> + ASSERT_EQ(ret, 1, "inet_pton(expected_ipv4_src)");
> +
> + ASSERT_EQ(fib_params->ipv4_src, expected_ipv4_src,
> + "fib_lookup ipv4 src");
> + }
> + if (tests[i].expected_ipv6_src) {
> + __u32 expected_ipv6_src[4];
> +
> + ret = inet_pton(AF_INET6, tests[i].expected_ipv6_src,
> + expected_ipv6_src);
> + ASSERT_EQ(ret, 1, "inet_pton(expected_ipv6_src)");
> +
> + ret = memcmp(expected_ipv6_src, fib_params->ipv6_src,
> + sizeof(fib_params->ipv6_src));
> + if (!ASSERT_EQ(ret, 0, "fib_lookup ipv6 src")) {
> + char src_ip6[64];
> +
> + inet_ntop(AF_INET6, fib_params->ipv6_src, src_ip6,
> + sizeof(src_ip6));
> + printf("ipv6 expected %s actual %s ",
> + tests[i].expected_ipv6_src, src_ip6);
> + }
> + }
nit. Move the v4/v6 expected_src comparison to a static function, potentially
done in a v4/v6 agnostic way mentioned in the above expected_ipv[46]_src comment.
> +
> ret = memcmp(tests[i].dmac, fib_params->dmac, sizeof(tests[i].dmac));
> if (!ASSERT_EQ(ret, 0, "dmac not match")) {
> char expected[18], actual[18];
Powered by blists - more mailing lists