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/[email protected] > 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, ¤t) && > + values_equal(&expected, ¤t)) > + *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
