Re: [PATCH net v2 3/3] selftests: net: add vxlan vnifilter notification test

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://netdev-ai.bots.linux.dev/sashiko/

Simon says:

There is also an AI-generated review of this patch on sashiko.dev.
And AI-generated reviews of other patches of this series.
But I believe what is forwarded here covers what should
be considered in the context of this patch-set.

---
selftests: net: add vxlan vnifilter notification test

This patch adds a selftest that exercises VXLAN vnifilter netlink
notifications by running bridge monitor vni in the background while
adding, updating, deleting and re-adding VNIs, then counting the
matching lines in the captured output.

> diff --git a/tools/testing/selftests/net/test_vxlan_vnifilter_notify.sh b/tools/testing/selftests/net/test_vxlan_vnifilter_notify.sh
> new file mode 100755
> index 000000000000..9d51a9e02ae0
> --- /dev/null
> +++ b/tools/testing/selftests/net/test_vxlan_vnifilter_notify.sh
> @@ -0,0 +1,184 @@

[ ... ]

> +# Run bridge monitor in the background, execute a command, then count
> +# the notification lines.
> +# Usage: vni_notify_check <command> [args...]
> +# Sets: NOTIFY_COUNT with the number of notifications observed.
> +vni_notify_check()
> +{
> +	local tmpf cmd_ret monitor_pid
> +
> +	tmpf=$(mktemp)
> +	defer rm "$tmpf"
> +
> +	defer_scope_push
> +		ip netns exec "$NS1" bridge monitor vni > "$tmpf" 2>/dev/null &
> +		monitor_pid=$!
> +		defer kill_process "$monitor_pid"
> +
> +		sleep 0.5
> +		if [ ! -e "/proc/$monitor_pid" ]; then
> +			RET=$ksft_skip
> +			log_test "iproute2 'bridge monitor vni' not supported"
> +			return "$RET"
> +		fi

[Medium]
Could the skip path here turn a legitimate skip into a failure?

When bridge monitor vni is not supported, the helper sets RET=$ksft_skip,
calls log_test from inside the helper, and then returns ksft_skip (4).
Every caller follows the helper with:

    vni_notify_check \
        bridge -n "$NS1" vni add vni 1000 dev "$VXLAN_DEV"
    check_err $? "Failed to add VNI"

Since ksft_skip is non-zero, check_err treats the return as a failure
and calls ret_set_ksft_status with ksft_fail. Per the merge weights in
ksft_status_merge (pass < xfail < skip < fail), that demotes RET from
skip to fail.

NOTIFY_COUNT is also never assigned because the function returned
before the grep, so the next:

    [ "$NOTIFY_COUNT" -eq 1 ]
    check_err $? "Expected 1 notification for VNI add, got $NOTIFY_COUNT"

also records a failure, and the caller's log_test then prints FAIL for
the real test name in addition to the SKIP printed by the helper. The
script ends up reporting both a SKIP and a FAIL line for the same case
and exits with a failure status.

Two related observations on this same path:

The bare return is issued between defer_scope_push and defer_scope_pop,
so defer_scope_pop is skipped on the skip path. Does that leave
__DEFER__SCOPE_ID off by one and the registered defer kill_process
"$monitor_pid" only unwound through the EXIT trap rather than the
normal scope unwind?

Would it be cleaner to signal skip back to the caller without going
through check_err (for example, set RET plus a flag and have the caller
short-circuit), and either move log_test to the caller or restructure
the skip path so it does not straddle defer_scope_push/defer_scope_pop?
fdb_notify.sh uses a similar pattern without mixing log_test and an
early return inside the scope.

> +
> +		"$@"
> +		cmd_ret=$?
> +		sleep 0.2
> +	defer_scope_pop
> +
> +	NOTIFY_COUNT=$(grep -c "$VXLAN_DEV" "$tmpf")
> +	NOTIFY_COUNT=${NOTIFY_COUNT:-0}
> +	return "$cmd_ret"
> +}

[Low]
Does this counting strategy actually distinguish RTM_NEWTUNNEL from
RTM_DELTUNNEL?

The commit message states:

    verifies that RTM_NEWTUNNEL and RTM_DELTUNNEL are sent correctly
    when VNIs are added, deleted, or updated

but NOTIFY_COUNT comes from:

    NOTIFY_COUNT=$(grep -c "$VXLAN_DEV" "$tmpf")

which matches both add and delete events emitted by bridge monitor vni.
A regression that emitted RTM_NEWTUNNEL on delete (or vice versa) would
still produce NOTIFY_COUNT=1 and pass every positive test. Would
differentiating the two (for instance, by separately counting lines
that begin with "Deleted " versus those that do not) make the test
match what the commit message describes?