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

[netdev-bot+sashiko@kernel.org]

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

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

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] selftests/net: unhandled EPIPE when reading /dev/kmsg causes
  spurious test failures
- [Low] main() in tools/testing/selftests/net/sysctl_net_test.c uses the
  kselftest.h API against its documented contract.
- [Low] The UDP part of the test cannot exercise the documented effect of
  the UDP sysctls it selects, so its 'UDP 16/16' result says nothing…
- [Low] v2 adds a facility filter to check_kernel_log() (`prefix =
  strtoul(record, NULL, 10); if (prefix >> 3 != 0) continue;`, lines…

--

selftests/net: test network sysctl extremes

This adds a new net selftest, sysctl_net_test. It finds the accepted lower
and upper bounds of four per-netns sysctls (tcp_rmem, tcp_wmem,
udp_rmem_min, udp_wmem_min) and runs TCP and UDP loopback traffic at each
bound. While it runs, it watches /dev/kmsg for kernel splats, and it
restores the original values after each probe.

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..6360616132c41
> --- /dev/null
> +++ b/tools/testing/selftests/net/sysctl_net_test.c
> @@ -0,0 +1,925 @@

[ ... ]

> +static bool check_kernel_log(void)
> +{

[ ... ]

> +		record[n] = '\0';
> +		message = strchr(record, ';');
> +		if (!message)
> +			continue;
> +		/* Ignore messages that userspace wrote to /dev/kmsg. */
> +		prefix = strtoul(record, NULL, 10);
> +		if (prefix >> 3 != 0)
> +			continue;
> +		message++;
> +		for (i = 0; i < ARRAY_SIZE(splats); i++) {
> +			if (!strstr(message, splats[i]))
> +				continue;
> +			set_failure("kernel splat detected: %.180s", message);
> +			return false;
> +		}
> +	}
> +}

[Severity: Medium]
What happens in check_kernel_log() when the printk ring buffer wraps
past this reader between two calls?

devkmsg_read() reports that case once as -EPIPE. It then moves the
reader forward to the oldest record that is still available:

kernel/printk/printk.c:devkmsg_read() {
    ...
	if (pmsg.dropped) {
		/* our last seen message is gone, return error and reset */
		atomic64_set(&user->seq, pmsg.seq);
		ret = -EPIPE;
		goto out;
	}
    ...
}

The loop here handles EINTR and EAGAIN/EWOULDBLOCK. Any other errno
goes to set_failure() and returns false.

So on a busy machine where something else logs heavily during the run,
the test fails with "cannot read /dev/kmsg: Broken pipe". Nothing in
the network stack went wrong in that case. probe_value(),
prepare_sysctl() and exercise_value() all turn this into a failure.
main() then stops testing the remaining sysctls. That makes the test
flaky on noisy CI hosts.

The next read() after -EPIPE succeeds, so could this case simply
continue the loop? If you want the output to show that records were
missed, and a splat could therefore have been lost, a "# " diagnostic
line might be enough. Or do you prefer to treat lost records as a
failure?

[Severity: Low]
The commit message says the test passed "also while userspace wrote
WARNING:/BUG: lines to /dev/kmsg". This facility check covers that case,
because devkmsg_write() forces LOG_USER on those records.

Does it keep all user-controlled text out of the match, though? Records
that the kernel itself generates still carry facility 0 even when they
embed user strings. Also, strstr() matches anywhere in the message body.
For example, on x86 show_signal_msg() puts the task comm at the start of
the line:

arch/x86/mm/fault.c:show_signal_msg() {
    ...
	printk("%s%s[%d]: segfault at %lx ip %px sp %px error %lx",
		loglvl, tsk->comm, task_pid_nr(tsk), address,
    ...
}

Suppose an unprivileged process calls prctl(PR_SET_NAME, "BUG:") and then
segfaults while this test runs. The record "BUG:[1234]: segfault at ..."
passes the prefix >> 3 check and matches "BUG:". The test then reports a
kernel splat and fails.

Would it help to match the markers only at the start of the message
(for example "WARNING: CPU:", "------------[ cut here ]", or "BUG: " at
offset 0)?

[ ... ]

> +static int udp_loopback(void)
> +{

[ ... ]

> +	addr.sin_port = htons(server_port);
> +	n = sendto(client, &data, sizeof(data), 0, (struct sockaddr *)&addr,
> +		   sizeof(addr));

[Severity: Low]
How much does the "UDP 16/16" result say about udp_rmem_min and
udp_wmem_min?

udp_loopback() sends and receives a single byte, which normally stays
below the protocol memory limit. In that case __sk_mem_raise_allocated()
returns before it reads udp_rmem_min:

net/core/sock.c:__sk_mem_raise_allocated() {
    ...
	/* Under limit. */
	if (allocated <= sk_prot_mem_limits(sk, 0)) {
		sk_leave_memory_pressure(sk);
		return 1;
	}
    ...
	if (kind == SK_MEM_RECV) {
		if (atomic_read(&sk->sk_rmem_alloc) < sk_get_rmem0(sk, prot))
			return 1;
    ...
}

Documentation/networking/ip-sysctl.rst also says udp_rmem_min applies
"even if total pages of UDP sockets exceed udp_mem pressure". For
udp_wmem_min it says "this tunable has no effect".

The test never creates UDP memory pressure. Unless the system is already
under pressure, wouldn't both bounds of both UDP sysctls pass whether or
not the minimum buffer handling works?

> +	if (n != sizeof(data)) {
> +		if (n >= 0)
> +			errno = EIO;
> +		goto out;
> +	}

[ ... ]

> +int main(int argc, char **argv)
> +{

[ ... ]

> +	ksft_print_header();
> +	ksft_set_plan(1);
> +
> +	if (unshare(CLONE_NEWNET))
> +		ksft_exit_skip("cannot create network namespace: %s\n",
> +			       strerror(errno));
> +
> +	kmsg_fd = open("/dev/kmsg", O_RDONLY | O_NONBLOCK | O_CLOEXEC);
> +	if (kmsg_fd < 0)
> +		ksft_exit_skip("cannot read /dev/kmsg: %s\n", strerror(errno));
> +	if (lseek(kmsg_fd, 0, SEEK_END) < 0)
> +		ksft_exit_skip("cannot seek /dev/kmsg: %s\n", strerror(errno));
> +	if (set_loopback_up())
> +		ksft_exit_skip("cannot bring loopback up: %s\n", strerror(errno));

[ ... ]

> +	if (!failure[0] && !tested_files)
> +		ksft_exit_skip("no selected writable numeric network sysctls found\n");

[ ... ]

> +	ksft_test_result_pass("TCP %zu/%zu, UDP %zu/%zu deliveries succeeded\n",
> +			      tcp_successes, tcp_attempts,
> +			      udp_successes, udp_attempts);
> +	ksft_finished();
> +	close(kmsg_fd);
> +	return KSFT_PASS;
> +
> +fail:
> +	ksft_test_result_fail("%s\n", failure);
> +	ksft_finished();
> +	close(kmsg_fd);
> +	return KSFT_FAIL;
> +}

[Severity: Low]
Can the close(kmsg_fd) and return statements after ksft_finished() ever
run? ksft_finished() expands to ksft_exit(), and that always calls either
ksft_exit_pass() or ksft_exit_fail(). Both are __noreturn.

So the exit status comes from the kselftest counters, not from the
KSFT_PASS or KSFT_FAIL returned here. The same applies to the fail: path.

Separately, ksft_exit_skip() is called after ksft_set_plan(1). This
happens on the unshare(), /dev/kmsg open and seek, loopback, and "no
selected writable numeric network sysctls" paths. kselftest.h calls this
pattern a misuse:

tools/testing/selftests/kselftest.h:ksft_exit_skip() {
    ...
	/*
	 * FIXME: several tests misuse ksft_exit_skip so produce
	 * something sensible if some tests have already been run
	 * or a plan has been printed.  Those tests should use
	 * ksft_test_result_skip or ksft_exit_fail_msg instead.
	 */
    ...
}

The header works around this today and prints "ok 1 # SKIP", so the TAP
output is still valid. Would ksft_test_result_skip() followed by
ksft_finished() be a better fit on these paths?

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