Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.
Critical: 0 · High: 0 · Medium: 1 · Low: 4 (1)
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [Medium] In vxlan_vnifilter_dump_replay()
(tools/testing/selftests/net/test_vxlan_vnifiltering.sh), the exit…
- [Low] Each new 'absent before' / 'installed nothing' probe in
vxlan_vnifilter_api() runs `bridge -netns $testns vni show dev vxlan-…
- [Low] vxlan_vnifilter_dump_replay() replays the dump one range at a
time, each in its own `bridge vni add` (lines 457-460), using ranges…
- [Low] The 24-bit bound cases in vxlan_vnifilter_api() do not isolate
the START half of vni_filter_entry_policy, and one case cannot tell…
- [Low] In vxlan_vnifilter_dump_replay(), the replay result variable `rc`
is overwritten by run_cmd() before the replay loop.
Pre-existing issues:
- [Low] The comment on VXLAN_VNI_FILTER_MSG_MAX in
drivers/net/vxlan/vxlan_vnifilter.c (lines 20-24) still says 'A later…
--
selftests: net: test the vxlan vnifilter request limit and dump replay
This extends test_vxlan_vnifiltering.sh to cover the per-request VNI cap
on add and delete and the 24-bit VNI bound. It also adds a dump/replay
test that builds a contiguous run twice the limit and replays the dumped
ranges into a second device. The cleanup() handler now also removes
veth-host and the test netns.
> diff --git a/tools/testing/selftests/net/test_vxlan_vnifiltering.sh
> b/tools/testing/selftests/net/test_vxlan_vnifiltering.sh
> index 6fb5af013dacf..bb35e89d2d036 100755
> --- a/tools/testing/selftests/net/test_vxlan_vnifiltering.sh
> +++ b/tools/testing/selftests/net/test_vxlan_vnifiltering.sh
[ ... ]
> @@ -347,6 +349,121 @@ vxlan_vnifilter_api()
> # change vxlan vnifilter flag
> run_cmd "ip -netns $testns link set dev vxlan-ext1 type vxlan external
> novnifilter"
> log_test $? 2 "Cannot unset vnifilter flag on a device"
> +
> + # A single request may add at most 4096 VNIs. bridge(8) puts one
> + # VXLAN_VNIFILTER_ENTRY per comma-separated item into a single message,
> + # so both the single-entry and the multi-entry paths are reachable here.
[Severity: Low]
This problem was not introduced by this patch, but the comment on
VXLAN_VNI_FILTER_MSG_MAX in drivers/net/vxlan/vxlan_vnifilter.c still
describes the request cap as future work:
drivers/net/vxlan/vxlan_vnifilter.c:
* dump emits stays within what the input path will accept on replay. A later
* patch bounds a single RTM_NEWTUNNEL/RTM_DELTUNNEL request to the same number.
*/
#define VXLAN_VNI_FILTER_MSG_MAX 4096
That cap is already in the tree. vxlan_vnifilter_process() calls
vxlan_vnifilter_check_msg() before it applies any entry:
err = vxlan_vnifilter_check_msg(nlh, extack);
if (err)
return err;
Could the "A later patch" sentence be updated, since this selftest is
exercising that cap?
> +
> + # The largest accepted add, and one VNI more rejected.
> + run_cmd "bridge -netns $testns vni add dev vxlan-ext1 vni 10000-14095"
> + log_test $? 0 "Add a request of the maximum VNI count"
> +
> + # 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"
[Severity: Low]
Can this probe pass when bridge vni show itself fails?
run_cmd() evaluates the whole pipeline and returns its status:
out=$(eval $cmd $stderr)
rc=$?
That status comes from grep. Neither this script nor lib.sh sets
pipefail. If bridge vni show fails, grep reads no input and exits 1, which
is the value log_test expects.
The same pattern is used for each of the "absent before" and "installed
nothing" probes for 20000, 30000 and 55000. That means these probes cannot
tell a VNI that is absent from a show that failed.
The src_ranges and dst_ranges captures in vxlan_vnifilter_dump_replay() also
drop the exit status of bridge.
Would it make sense to check the exit status of bridge vni show on its own,
or to set pipefail for these probes?
> + 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"
[ ... ]
> + # 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]
Does the 100-4294967295 case depend on the END range policy? Its span is
4294967196 VNIs, so vxlan_vnifilter_check_msg() would reject it with
-EINVAL even if the END bound in vni_filter_entry_policy were removed:
vnis += vni_end - vni_start + 1;
if (vnis > VXLAN_VNI_FILTER_MSG_MAX) {
The test comment ("The bound is on both endpoints") and the commit message
("exercised through END, not only START") suggest that an out-of-range
START is tested. No such case exists here or elsewhere under
tools/testing/selftests.
A single VNI such as 16777216 has no END attribute. That gives a span of
one, so check_msg accepts it, and only the START policy keeps it out of
vxlan_vni_add_del():
[VXLAN_VNIFILTER_ENTRY_START] = NLA_POLICY_FULL_RANGE(NLA_U32,
&vni_filter_vni_range),
If START=END=0xffffffff ever got past that policy, this loop would wrap
under rtnl_lock:
for (v = start_vni; v <= end_vni; v++) {
Could a case like "vni add ... vni 16777216" be added so a regression in
the START bound is caught?
> +}
> +
> +# 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()
> +{
> + local opts="external vnifilter local 172.16.0.1 dev veth-testns"
> + local range rc=0 src_ranges dst_ranges
[ ... ]
> + # 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: Medium]
Is the status of the first populate command lost here? The $? passed to
log_test is only the status of the 14096-18191 add.
If the 10000-14095 add fails, vxlan-src holds exactly 4096 VNIs. In that
case vxlan_vnifilter_dump_dev() never reaches the clamp:
if (!dump_stats && vnirange(vend, v) == 1 &&
vnirange(vbegin, v) < VXLAN_VNI_FILTER_MSG_MAX &&
The dump is then a single limit-sized entry. The non-empty check, the
replay loop and the src/dst comparison all pass, whether or not the dump
clamp exists.
The v4 note says the non-empty check was added so the replay case cannot
pass vacuously, but this path still does.
There is a related gap. src_ranges is never compared against the expected
split (10000-14095 and 14096-18191). Both the replay input and the expected
result come from the same source dump.
A resume regression in vxlan_vnifilter_dump_dev() could drop or repeat VNIs
at the 4096 boundary, or lose the second message:
if (!err && vbegin) {
if (vnis + vnirange(vbegin, vend) + 1 >
VXLAN_VNI_FILTER_MSG_MAX)
err = -EMSGSIZE;
Wouldn't that produce the same wrong ranges on both devices and still
pass?
Could each populate command get its own log_test, and could src_ranges be
checked against the literal expected ranges?
> +
> + # Assert the dump is non-empty before replaying it, so an empty or
> + # failed dump cannot let the replay check below pass without testing
> + # anything.
> + src_ranges=$(bridge -netns $testns vni show dev vxlan-src | \
> + grep -oE '[0-9]+-[0-9]+|[0-9]{2,}' | sort)
> + test -n "$src_ranges"
> + log_test $? 0 "dump/replay: source dump is non-empty"
> +
> + for range in $src_ranges; do
> + bridge -netns $testns vni add dev vxlan-dst vni "$range" \
> + 2>/dev/null || rc=$?
> + done
[Severity: Low]
This loop replays one range per bridge vni add, using ranges parsed from
the text output of bridge vni show. That output loses netlink message
boundaries, so the per-message VNI budget in vxlan_vnifilter_dump_dev() is
never tested:
if (vnis + sz > VXLAN_VNI_FILTER_MSG_MAX) {
err = -EMSGSIZE;
break;
}
vxlan_vnifilter_check_msg() caps the sum across all entries in a request.
Suppose that budget were removed and the merge clamp kept. The 8192-VNI
run would be dumped as 10000-14095 and 14096-18191 in one message, and that
message would be rejected if replayed as a single request.
Each range would still replay on its own here, the dumps would match, and
the test would pass. Is the per-message half of the dump guarantee meant
to be covered by this test?
> + log_test $rc 0 "dump/replay: every dumped entry is accepted on replay"
[Severity: Low]
Can rc be non-zero here even when every replay add succeeded?
run_cmd() assigns rc=$? without declaring it local. Because bash scoping is
dynamic, that assignment writes to the local rc in
vxlan_vnifilter_dump_replay(). After the 14096-18191 populate run_cmd, rc
holds that command's exit status.
The replay loop only assigns rc when an add fails. So if the populate
failed, this line also reports FAIL even when all replays succeeded, and
the rc=0 initializer has no effect.
Would resetting rc=0 just before the loop, or using a separate variable,
avoid this?
> +
> + dst_ranges=$(bridge -netns $testns vni show dev vxlan-dst | \
> + grep -oE '[0-9]+-[0-9]+|[0-9]{2,}' | sort)
> + [ "$src_ranges" = "$dst_ranges" ]
> + log_test $? 0 "dump/replay: source and destination dump the same ranges"
> }
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261014123814.388557-1-alishmery18%40gmail.com
