Re: [PATCH net-next v3 6/6] selftests: net: test the vxlan vnifilter request limit and dump replay

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] No test rejects a VNI because its START is outside the 24-bit
  space, even though the commit message says the bound is "exercised…
- [Low] In vxlan_vnifilter_dump_replay()
  (test_vxlan_vnifiltering.sh:469-471), two run_cmd calls populate the…
- [Low] vxlan_vnifilter_dump_replay() only compares the source dump with
  the destination dump.
- [Low] Every absence and "installed nothing" check in
  vxlan_vnifilter_api() (lines 389, 393, 399, 403, 428, 432) runs…

--

selftests: net: test the vxlan vnifilter request limit and dump replay

This extends test_vxlan_vnifiltering.sh to cover the vnifilter request-size
limit, the 24-bit VNI range, and symmetric add/delete caps. It adds a new
vxlan_vnifilter_dump_replay() test that builds a run twice the limit,
replays the dumped ranges into a second device, and compares the two dumps.
It also adds veth-host and the test netns to cleanup().

> diff --git a/tools/testing/selftests/net/test_vxlan_vnifiltering.sh b/tools/testing/selftests/net/test_vxlan_vnifiltering.sh
> index 8deacc565afa1..f48bc861bb88e 100755
> --- a/tools/testing/selftests/net/test_vxlan_vnifiltering.sh
> +++ b/tools/testing/selftests/net/test_vxlan_vnifiltering.sh

[ ... ]

> @@ -371,6 +373,116 @@ vxlan_vnifilter_api()

[ ... ]

> +	# The rejected oversized add must install nothing. Assert the range is
> +	# absent first: vxlan_vni_add() folds an already-present VNI into the
> +	# update path and returns 0, so a later probe cannot tell "installed
> +	# nothing" from "installed part" unless it started absent.
> +	run_cmd "bridge -netns $testns vni show dev vxlan-ext1 | grep -qw 20000"
> +	log_test $? 1 "VNI 20000 absent before the oversized add"
> +	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 20000-24096"
> +	log_test $? 255 "Cannot add a request over the maximum VNI count"
> +	run_cmd "bridge -netns $testns vni show dev vxlan-ext1 | grep -qw 20000"
> +	log_test $? 1 "The rejected oversized add installed nothing"

[Severity: Low]
Can these absence checks pass without looking at the device at all?

run_cmd() does this:

	out=$(eval $cmd $stderr)
	rc=$?

That returns the status of the last command in the pipeline, which is
grep. The script does not set pipefail.

Suppose bridge vni show fails or prints nothing, for example because of a
regression in the dump path. grep -qw 20000 then exits 1 on empty input,
and log_test $? 1 reports OK.

The same pattern is used for every absence and "installed nothing" check
in vxlan_vnifilter_api(), for VNIs 20000, 30000 and 55000.

Could the dump status be checked on its own, or pipefail be enabled for
these commands?

[ ... ]

> +	# The VXLAN header carries 24 bits. The bound is on both endpoints, so a
> +	# range whose END alone leaves the space is rejected too.
> +	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 16777215"
> +	log_test $? 0 "Add the highest VNI the header can carry"
> +	run_cmd "bridge -netns $testns vni del dev vxlan-ext1 vni 16777215"
> +	log_test $? 0 "Delete the highest VNI the header can carry"
> +	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 16777215-16777216"
> +	log_test $? 255 "Cannot add a range whose END leaves the 24-bit space"
> +	run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 100-4294967295"
> +	log_test $? 255 "Cannot add a range whose END wraps past the 24-bit space"
> +}

[Severity: Low]
The commit message says:

    and the 24-bit bound exercised through END,
    not only START.

Does any case here reject a VNI because its START is out of range? START
only appears as an accepted value (vni 16777215). Both rejected cases go
out of range through END only.

The 100-4294967295 case also seems to be rejected by the request cap
whatever the policy says. In vxlan_vnifilter_check_msg():

		vnis += vni_end - vni_start + 1;
		if (vnis > VXLAN_VNI_FILTER_MSG_MAX) {

The span is far over 4096, so this returns -EINVAL whether or not the
NLA_POLICY_FULL_RANGE entries in vni_filter_entry_policy exist. So only
16777215-16777216 tests the 24-bit policy.

The case the series' "reject VNIs outside the 24-bit space" patch is
mainly about, a lone START=0xffffffff, has no test. That request has
vnis == 1, so the cap accepts it. Only the START policy then keeps
vxlan_vni_add_del() out of this endless loop:

	for (v = start_vni; v <= end_vni; v++) {

Would a case such as vni 16777216 expecting 255 be worth adding? It
covers START without risking a hang if it regresses.

> +
> +# A device may hold a contiguous run longer than one request's limit, built
> +# from several requests. The dump coalesces it, so the dump must break the run
> +# into entries each no larger than the limit, or the configuration it reports
> +# cannot be replayed. Install such a run, then feed every range the dump
> +# reports back into a second device and require all to be accepted.
> +vxlan_vnifilter_dump_replay()
> +{

[ ... ]

> +	# 8192 contiguous VNIs sharing the default remote: one run, twice the
> +	# limit, installed in two accepted requests.
> +	run_cmd "bridge -netns $testns vni add dev vxlan-src vni 10000-14095"
> +	run_cmd "bridge -netns $testns vni add dev vxlan-src vni 14096-18191"
> +	log_test $? 0 "dump/replay: populate a run larger than the limit"

[Severity: Low]
Is the exit status of the first add ever checked? The second run_cmd
overwrites $?, so log_test only sees the 14096-18191 add.

If the first add fails and the second succeeds, vxlan-src holds only
14096-18191. That is exactly 4096 VNIs, and it dumps as one entry the
input path accepts, with or without this clamp in
vxlan_vnifilter_dump_dev():

		    vnirange(vbegin, v) < VXLAN_VNI_FILTER_MSG_MAX &&

Replay then succeeds and the two dumps match, so all three dump/replay
checks report OK without testing the clamp.

This also applies when the test runs alone with
-t vxlan_vnifilter_dump_replay. In that case the API test's max-size add
check does not run first.

Could each add get its own log_test?

> +
> +	for range in $(bridge -netns $testns vni show dev vxlan-src | \
> +		       grep -oE '[0-9]+-[0-9]+|[0-9]{2,}'); do
> +		bridge -netns $testns vni add dev vxlan-dst vni "$range" \
> +			2>/dev/null || rc=$?
> +	done
> +	log_test $rc 0 "dump/replay: every dumped entry is accepted on replay"
> +
> +	src_ranges=$(bridge -netns $testns vni show dev vxlan-src | \
> +		     grep -oE '[0-9]+-[0-9]+|[0-9]{2,}' | sort)
> +	dst_ranges=$(bridge -netns $testns vni show dev vxlan-dst | \
> +		     grep -oE '[0-9]+-[0-9]+|[0-9]{2,}' | sort)
> +	[ -n "$src_ranges" ] && [ "$src_ranges" = "$dst_ranges" ]
> +	log_test $? 0 "dump/replay: source and destination dump the same ranges"

[Severity: Low]
Would this comparison catch a dump that drops part of the run? Both sides
come from the same vxlan_vnifilter_dump_dev(), and neither is compared
with the 10000-18191 range that was installed.

Suppose the clamp regressed so the dump stopped at the first limit
boundary and reported only 10000-14095, with no new entry starting at
14096. The replay loop would install 10000-14095 on vxlan-dst, and both
dumps would print 10000-14095.

Both the "every dumped entry is accepted on replay" and "source and
destination dump the same ranges" checks would pass, even though half
the configuration was missing from the dump.

Could src_ranges also be compared against the expected
10000-14095 14096-18191?

>  }
>  
>  # Sanity test vnifilter datapath

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260927215209.2581830-1-alishmery18%40gmail.com