netdev - Re: [PATCH net-next] selftests: net: local_termination: annotate the expected failures

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] [day] [month] [year] [list]

Message-ID: <87ikzevyl7.fsf@nvidia.com>
Date: Thu, 16 May 2024 10:42:31 +0200
From: Petr Machata <petrm@...dia.com>
To: Jakub Kicinski <kuba@...nel.org>
CC: Petr Machata <petrm@...dia.com>, <davem@...emloft.net>,
	<netdev@...r.kernel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>,
	<vladimir.oltean@....com>, <shuah@...nel.org>, <liuhangbin@...il.com>,
	<bpoirier@...dia.com>, <linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH net-next] selftests: net: local_termination: annotate
 the expected failures


Jakub Kicinski <kuba@...nel.org> writes:

> On Wed, 15 May 2024 11:02:28 +0200 Petr Machata wrote:
>> >> And then either replace the existing xfail_on_veth's (there are just a
>> >> handful) or convert xfail_on_veth to a wrapper around xfail_on_kind.  
>> >
>> > I think the bridge thing we can workaround by just checking
>> > if ${NETIFS[p1]} is veth, rather than $rcv_if_name.
>> > Since the two behave the same.  
>> 
>> I don't follow. The test has two legs, one creates a VRF and attaches
>> p2, the other creates a bridge and attaches p2. Whether p1 and p2 are
>> veth or HW seems orthogonal to whether $rcv_if_name is a bridge or a
>> veth.
>
> Right, my superficial understanding was that the main distinction is
> whether p2/h2 can do the filtering (or possibly some offload happens).
> So if p1,p2 are veths we know to XFAIL, doesn't matter if we're in 
> the vrf or bridge configuration, cause these construct will not filter
> either.
>
> If I'm not making sense - I'm probably confused, I can code up what you
> suggested, it will work, just more LoC :)

I'm not sure myself, but from the commit message it looks like the issue
is with $rcv_if_name being the bridge.

But the patch that you inline is R-b'd and T-b'd by Vladimir, so I'm
going to assume it's doing the right thing.

> +# Clear internal failure tracking for the next test case
> +begin_test()
> +{
> +    RET=0
> +    FAIL_TO_XFAIL=
> +}
> +
>  check_err()
>  {
>  	local err=$1
> diff --git a/tools/testing/selftests/net/forwarding/local_termination.sh b/tools/testing/selftests/net/forwarding/local_termination.sh
> index c5b0cbc85b3e..a241acc02498 100755
> --- a/tools/testing/selftests/net/forwarding/local_termination.sh
> +++ b/tools/testing/selftests/net/forwarding/local_termination.sh
> @@ -73,9 +73,12 @@ check_rcv()
>  	local pattern=$3
>  	local should_receive=$4
>  	local should_fail=
> +	local xfail_sw=$5
>  
>  	[ $should_receive = true ] && should_fail=0 || should_fail=1
> -	RET=0
> +	begin_test
> +	# check if main interface is veth
> +	[ "$xfail_sw" == true ] && xfail_on_veth $h1

If xfail_on_veth $h1 is all that's needed, then I really don't see a
reason why not just do this:

	check_rcv $rcv_if_name "Unicast IPv4 to primary MAC address" \
		"$smac > $rcv_dmac, ethertype IPv4 (0x0800)" \
		true

	check_rcv $rcv_if_name "Unicast IPv4 to macvlan MAC address" \
		"$smac > $MACVLAN_ADDR, ethertype IPv4 (0x0800)" \
		true

	xfail_on_veth $h1 \
		check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
			"$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
			false

This should work now, in much the same way as this patch, but the intent
is IMHO clearer (vs. passing a mystery true), and FAIL_TO_XFAIL is
cleanly scoped and doesn't run the risk of leaking out of the test.

>  	tcpdump_show $if_name | grep -q "$pattern"
>  
> @@ -157,7 +160,7 @@ run_test()
>  
>  	check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
>  		"$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
> -		false
> +		false true
>  
>  	check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address, promisc" \
>  		"$smac > $UNKNOWN_UC_ADDR2, ethertype IPv4 (0x0800)" \
> @@ -165,7 +168,7 @@ run_test()
>  
>  	check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address, allmulti" \
>  		"$smac > $UNKNOWN_UC_ADDR3, ethertype IPv4 (0x0800)" \
> -		false
> +		false true
>  
>  	check_rcv $rcv_if_name "Multicast IPv4 to joined group" \
>  		"$smac > $JOINED_MACV4_MC_ADDR, ethertype IPv4 (0x0800)" \
> @@ -173,7 +176,7 @@ run_test()
>  
>  	check_rcv $rcv_if_name "Multicast IPv4 to unknown group" \
>  		"$smac > $UNKNOWN_MACV4_MC_ADDR1, ethertype IPv4 (0x0800)" \
> -		false
> +		false true
>  
>  	check_rcv $rcv_if_name "Multicast IPv4 to unknown group, promisc" \
>  		"$smac > $UNKNOWN_MACV4_MC_ADDR2, ethertype IPv4 (0x0800)" \
> @@ -189,7 +192,7 @@ run_test()
>  
>  	check_rcv $rcv_if_name "Multicast IPv6 to unknown group" \
>  		"$smac > $UNKNOWN_MACV6_MC_ADDR1, ethertype IPv6 (0x86dd)" \
> -		false
> +		false true
>  
>  	check_rcv $rcv_if_name "Multicast IPv6 to unknown group, promisc" \
>  		"$smac > $UNKNOWN_MACV6_MC_ADDR2, ethertype IPv6 (0x86dd)" \