Re: [PATCH nf-next v3 3/3] selftests: netfilter: nft_flowtable.sh: check the priority a flow carries

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] selftests: netfilter: flaky priority test due to race in counter reset
--

commit fb5120bafad5d9a740fa81fe7c9587aaad2d26eb
Author: Julius Bairaktaris <julius@bairaktaris.de>

selftests: netfilter: nft_flowtable.sh: check the priority a flow carries

Count the forwarded TCP packets that leave with priority 0:3 and those
that leave with none, first without a priority and then with "meta
priority set 0:3" ahead of "flow add". With the priority set, every
packet must carry it, including those the flowtable forwards. Runs for
IPv4 and IPv6.

> diff --git a/tools/testing/selftests/net/netfilter/nft_flowtable.sh b/tools/testing/selftests/net/netfilter/nft_flowtable.sh
> index 449c518bd947d..73c7b1ec2208b 100755
> --- a/tools/testing/selftests/net/netfilter/nft_flowtable.sh
> +++ b/tools/testing/selftests/net/netfilter/nft_flowtable.sh

[ ... ]

> @@ -458,6 +458,106 @@ fi
>  	check_dscp "dscp_fwd" "$pmtu"
>  }
>  
> +check_priority()
> +{

[ ... ]

> +test_tcp_forwarding_set_priority()
> +{
> +	local pmtu="$3"
> +	local proto="$4"
> +	local dstip="$5"
> +	local dstport="$6"
> +	local lret=0
> +
> +ip netns exec "$nsr1" nft -f - <<EOF
> +table netdev priocheck {
> +   counter prio0 { }
> +   counter prio3 { }
> +
> +   chain egress0 {
> +      type filter hook egress device "veth0" priority 0; policy accept
> +      meta l4proto tcp meta priority 0:3 counter name "prio3"
> +      meta l4proto tcp meta priority none counter name "prio0"
> +   }
> +
> +   chain egress1 {
> +      type filter hook egress device "veth1" priority 0; policy accept
> +      meta l4proto tcp meta priority 0:3 counter name "prio3"
> +      meta l4proto tcp meta priority none counter name "prio0"
> +   }
> +}
> +EOF
> +	if [ $? -ne 0 ]; then
> +		echo "SKIP: Could not load netdev:egress for veth0 and veth1"
> +		return 0
> +	fi
> +
> +	if ! test_tcp_forwarding_ip "$1" "$2" "$pmtu" "$proto" "$dstip" "$dstport"; then
> +		lret=1
> +	fi
> +	check_priority "prio_none" "$pmtu"

[Severity: Medium]
Could this introduce test flakiness due to a race condition?

The test_tcp_forwarding_ip() function runs a socat TCP transfer and returns
as soon as the socat processes exit. However, final TCP packets (like FIN or
ACK) may still be in flight in the network namespaces.

When check_priority "prio_none" reads and resets the netfilter counters
immediately after the transfer completes, any delayed packet from this first
unprioritized run that arrives after the reset will increment the prio0
counter.

> +
> +	# The flow stores the priority set before it is added, so the packets
> +	# the flowtable forwards leave with it too, in both directions.
> +ip netns exec "$nsr1" nft -f - <<EOF
> +table inet prioset {
> +   chain forward {
> +      type filter hook forward priority -1; policy accept
> +      meta priority set 0:3
> +   }
> +}
> +EOF
> +	if ! test_tcp_forwarding_ip "$1" "$2" "$pmtu" "$proto" "$dstip" "$dstport"; then
> +		lret=1
> +	fi
> +	check_priority "prio_fwd" "$pmtu"

[Severity: Medium]
If a delayed packet from the first run incremented prio0 after the reset,
will it cause this check to fail spuriously?

The prio_fwd check expects prio0 to remain exactly 0. If delayed packets
from the previous run are still trickling through, this might lead to
random test failures in kselftests.

> +
> +	ip netns exec "$nsr1" nft delete table inet prioset
> +	ip netns exec "$nsr1" nft delete table netdev priocheck
> +
> +	return $lret
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004171636.3545085-1-julius@bairaktaris.de?part=3