Re: [PATCH net] selftests/net: test network sysctl extremes

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 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] find_bound() assumes every integer inside the accepted range
  reads back unchanged, and treats the first candidate that does not…
- [Medium] In --all mode, the bound search assumes that if a sysctl
  accepts a write, writing the original value back will also succeed.
- [Low] check_kernel_log() throws away the '<pri/facility>,seq,...;'
  record prefix (`message = strchr(record, ';')`, line 307) and searches…

--

selftests/net: test network sysctl extremes

This adds a selftest that probes the lower and upper bounds accepted by
writable network sysctls inside a private network namespace. It then runs
TCP and UDP loopback traffic at each bound. By default it covers four
representative TCP/UDP sysctls. The -s option selects from that set, and
--all scans every writable numeric sysctl under /proc/sys/net.

Link: https://lore.kernel.org/r/20261007054508.1059927-1-sahaj123.sc@gmail.com

> diff --git a/tools/testing/selftests/net/sysctl_net_test.c b/tools/testing/selftests/net/sysctl_net_test.c
> new file mode 100644
> index 0000000000000..cb2f0570d3372
> --- /dev/null
> +++ b/tools/testing/selftests/net/sysctl_net_test.c
> @@ -0,0 +1,947 @@

[ ... ]

> +static bool check_kernel_log(void)
> +{
> +	static const char * const splats[] = {
> +		"WARNING:",
> +		"BUG:",

[ ... ]

> +		record[n] = '\0';
> +		message = strchr(record, ';');
> +		message = message ? message + 1 : record;
> +		for (i = 0; i < ARRAY_SIZE(splats); i++) {
> +			if (!strstr(message, splats[i]))
> +				continue;
> +			set_failure("kernel splat detected: %.180s", message);
> +			return false;
> +		}
> +	}
> +}

[Severity: Low]
Could check_kernel_log() report a userspace message as a kernel splat?

It drops the record prefix before the ';' without checking the facility.
Any record that contains "WARNING:" or "BUG:" will match. That includes
records that init or a logging daemon writes to /dev/kmsg while the test
is running.

Documentation/ABI/testing/dev-kmsg says:

    If no prefix is given, the priority number is the default kernel
    log priority and the facility number is set to LOG_USER (1). It
    is not possible to inject messages from userspace with the
    facility number LOG_KERN (0)

Should the leading priority value be parsed, and records skipped when
their facility is not LOG_KERN?

> +static int probe_value(const char *path, const struct sysctl_values *original,
> +		       size_t index, struct sysctl_number candidate,
> +		       bool *accepted)
> +{
> +	struct sysctl_values expected = *original;
> +	struct sysctl_values current;
> +	char text[32];
> +
> +	expected.value[index] = candidate;
> +	*accepted = false;
> +
> +	if (write_values(path, &expected) &&
> +	    read_values(path, &current) &&
> +	    values_equal(&expected, &current))
> +		*accepted = true;
> +
> +	if (!restore_values(path, original)) {
> +		format_number(candidate, text, sizeof(text));
> +		set_failure("failed to restore %s after probing %s", path, text);
> +		return -1;
> +	}

[Severity: Medium]
Is it safe to assume that writing the original value back will undo any
value the kernel accepts?

In --all mode this is not true for /proc/sys/net/netfilter/nf_hooks_lwtunnel.
nf_lwtunnel_net_init() registers it with mode 0644 in every netns, so the
test's private netns can write it. Its handler sets a global static key
that can only be turned on:

net/netfilter/nf_hooks_lwtunnel.c:nf_hooks_lwtunnel_set() {
	if (static_branch_unlikely(&nf_hooks_lwtunnel_enabled)) {
		if (!enable)
			return -EBUSY;
	} else if (enable) {
		static_branch_enable(&nf_hooks_lwtunnel_enabled);
	}
    ...
}

With CONFIG_LWTUNNEL=y and the key off at the start, the sequence would
be:

main()->nftw()->scan_sysctl()->test_sysctl()
    find_lower_bound()   probes -1, rejected by extra1 = SYSCTL_ZERO
    find_upper_bound()->find_bound()->probe_value()
        write 1           succeeds, calls static_branch_enable()
        restore_values()  write 0 fails with -EBUSY
        set_failure("failed to restore %s after probing %s", ...)

The run then stops with a FAIL. The netns does not isolate the static
key. Does this also leave netfilter hooks enabled on the lwtunnel/seg6
input and output paths in every namespace, including init_net, until
reboot?

Should the --all scan skip sysctls like this one?

[ ... ]

> +static int find_bound(const char *path,
> +		      const struct sysctl_values *original, size_t index,
> +		      bool lower, bool unsigned_domain,
> +		      struct sysctl_number *bound)
> +{

[ ... ]

> +		if (accepted) {
> +			valid = candidate;
> +			step = next_step(step);
> +			continue;
> +		}
> +
> +		invalid = candidate;
> +		while (lower ? valid - invalid > 1 : invalid - valid > 1) {

[Severity: Medium]
Does this search handle sysctls that accept a value but round it?

probe_value() sets *accepted only when the value read back is exactly
the value written. A rounded value is therefore treated like a rejected
one.

For example, the neighbour sysctls retrans_time_ms and
base_reachable_time_ms are defined with
NEIGH_SYSCTL_MS_JIFFIES_REUSED_ENTRY in net/core/neighbour.c. They use
proc_dointvec_ms_jiffies(), which converts through msecs_to_jiffies()
and jiffies_to_msecs(). With HZ=250 and base_reachable_time_ms=30000:

    probe 29999 -> reads back 30000 -> not accepted
    probe 30001 -> reads back 30004 -> not accepted

Each rejected candidate is one unit from valid, so this loop never runs.
find_bound() then returns 30000 as both the lower and the upper bound.
But 29996 and 30004 both read back unchanged, so 30000 is not a bound.

test_sysctl() then sees that lower equals upper:

		if (exercise_value(path, original, i, lower) ||
		    (!numbers_equal(upper, lower) &&
		     exercise_value(path, original, i, upper)))
			return -1;
		tested_values++;

exercise_value() runs once at the original value. restore is false
there, so nothing is written, but tested_values is still incremented.

In --all mode on a kernel with HZ != 1000, would these sysctls be counted
as bound-tested even though no bound was actually exercised?

[ ... ]

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