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

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 (local)

[ ... ]

[ ... ]

[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?

[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?

[ ... ]

[ ... ]

[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