Re: [PATCH v6 net-next 15/15] selftest: net: Specify netns for ip ntable in test_neigh.sh.

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 0

- [Medium] The series namespacifies the neighbour tables (commit
  "neighbour: Namespacify neigh_tables."), and this patch is the…

--

selftest: net: Specify netns for ip ntable in test_neigh.sh.

Now that the neighbour tables are per netns, this patch converts the
"ip ntable" invocations in test_neigh.sh to "ip -n $ns1 ntable" so the
thresholds and base_reachable_time are set in the namespace the test
actually uses, and drops the comments that described those knobs as
global resources.

> diff --git a/tools/testing/selftests/net/test_neigh.sh b/tools/testing/selftests/net/test_neigh.sh
> index 447423713266..e70e31edf633 100755
> --- a/tools/testing/selftests/net/test_neigh.sh
> +++ b/tools/testing/selftests/net/test_neigh.sh
> @@ -240,9 +240,6 @@ extern_valid_common()
>  	# Check that an "extern_valid" entry survives a forced garbage
>  	# collection. Add an entry, wait 5 seconds and add more entries than
>  	# "thresh3" so that forced garbage collection will run.
> -	#
> -	# Note that the garbage collection thresholds are global resources and
> -	# that changes in the initial namespace affect all the namespaces.
>  	local forced_gc_runs_t0
>  	local forced_gc_runs_t1
>  	local orig_thresh1
> @@ -250,18 +247,23 @@ extern_valid_common()
>  	local orig_thresh3
>  
>  	run_cmd "ip -n $ns1 neigh flush dev veth0"
> -	orig_thresh1=$(ip -j ntable show name "$tbl_name" | jq '.[] | select(has("thresh1")) | .["thresh1"]')
> -	orig_thresh2=$(ip -j ntable show name "$tbl_name" | jq '.[] | select(has("thresh2")) | .["thresh2"]')
> -	orig_thresh3=$(ip -j ntable show name "$tbl_name" | jq '.[] | select(has("thresh3")) | .["thresh3"]')
> -	run_cmd "ip ntable change name $tbl_name thresh3 10 thresh2 9 thresh1 8"
> +	orig_thresh1=$(ip -n "$ns1" -j ntable show name "$tbl_name" | \
> +			jq '.[] | select(has("thresh1")) | .["thresh1"]')
> +	orig_thresh2=$(ip -n "$ns1" -j ntable show name "$tbl_name" | \
> +			jq '.[] | select(has("thresh2")) | .["thresh2"]')
> +	orig_thresh3=$(ip -n "$ns1" -j ntable show name "$tbl_name" | \
> +			jq '.[] | select(has("thresh3")) | .["thresh3"]')
> +	run_cmd "ip -n $ns1 ntable change name $tbl_name thresh3 10 thresh2 9 thresh1 8"

[Severity: Medium]
This hunk moves test_neigh.sh over to the per-netns form, but should
tools/testing/selftests/net/netfilter/nft_nat_zones.sh get the same
treatment in this series?  That script tunes the same knobs from the root
netns:

tools/testing/selftests/net/netfilter/nft_nat_zones.sh {
	...
	setup_ns gw srv
	...
	sysctl -q net.ipv6.neigh.default.gc_thresh1=512  2>/dev/null
	sysctl -q net.ipv6.neigh.default.gc_thresh2=1024 2>/dev/null
	sysctl -q net.ipv6.neigh.default.gc_thresh3=4096 2>/dev/null
	sysctl -q net.ipv4.neigh.default.gc_thresh1=512  2>/dev/null
	sysctl -q net.ipv4.neigh.default.gc_thresh2=1024 2>/dev/null
	sysctl -q net.ipv4.neigh.default.gc_thresh3=4096 2>/dev/null
	...
}

With the namespacified tables, neigh_sysctl_register() points those ctl
entries at the per-netns table and registers them in that netns:

net/core/neighbour.c:neigh_sysctl_register() {
	...
		t->neigh_vars[NEIGH_VAR_GC_THRESH1].data = &tbl->gc_thresh1;
		t->neigh_vars[NEIGH_VAR_GC_THRESH2].data = &tbl->gc_thresh2;
		t->neigh_vars[NEIGH_VAR_GC_THRESH3].data = &tbl->gc_thresh3;
	...
}

and the only inheritance from init_net is a one-time snapshot taken when
the namespace's table is registered:

net/core/neighbour.c:neigh_table_inherit() {
	...
	tbl->gc_thresh1 = READ_ONCE(init_tbl->gc_thresh1);
	tbl->gc_thresh2 = READ_ONCE(init_tbl->gc_thresh2);
	tbl->gc_thresh3 = READ_ONCE(init_tbl->gc_thresh3);
	...
}

Since nft_nat_zones.sh runs "setup_ns gw srv" before writing the raised
values, do the gw and srv namespaces still end up with the compiled-in
defaults instead of 512/1024/4096?  If so, the raised thresholds are a
no-op for the gw namespace that actually holds the per-veth neighbour
entries, and the gate in neigh_periodic_work():

net/core/neighbour.c:neigh_periodic_work() {
	...
	if (neigh_table_entries(tbl) < READ_ONCE(tbl->gc_thresh1))
		goto out;
	...
}

applies with the unraised limit, which can make the nft counter checks
come up short once the entry count grows (the script's own comment
suggests raising maxclients to 2000).

The cleanup() function in the same script restores only the root-netns
values, which would likewise no longer affect the namespaces that ran the
test.

Would moving the sysctl writes above "setup_ns gw srv", or issuing them
with "ip netns exec \"$gw\" sysctl ...", be the intended conversion here?

[ ... ]

> @@ -284,10 +287,6 @@ extern_valid_common()
>  	# collection. Add an "extern_valid" entry, add more than "thresh1"
>  	# regular entries, wait "base_reachable" (longer than "gc_stale")
>  	# seconds and check that the "extern_valid" entry was not deleted.
> -	#
> -	# Note that the garbage collection thresholds and "base_reachable" are
> -	# global resources and that changes in the initial namespace affect all
> -	# the namespaces.
>  	local periodic_gc_runs_t0
>  	local periodic_gc_runs_t1
>  	local orig_base_reachable

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260902203722.926528-1-kuniyu%40google.com