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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <203643c3-4ea5-40a8-bd74-d4bde8deba2d@app.fastmail.com>
Date: Thu, 05 Oct 2023 22:19:48 +0200
From: Martynas <m@...bda.lt>
To: "Martin KaFai Lau" <martin.lau@...ux.dev>
Cc: "Daniel Borkmann" <daniel@...earbox.net>, netdev <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 Thu, Oct 5, 2023, at 12:02 AM, Martin KaFai Lau wrote:
> 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.
>

Yep, I will change to that.

>> * 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?
>

Yep, for secondary. IPV6_IFACE_ADDR_SECONDARY felt a bit too long.

>> +#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.
>

SGTM.

>>   	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, &params->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, &params->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.
>
>

SGTM.

>> +
>>   		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