Re: [PATCH bpf-next v7 8/9] selftests/bpf: move bpf_jit_harden helper into testing_helpers

[bot+bpf-ci]

> diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_insn_array.c b/tools/testing/selftests/bpf/prog_tests/bpf_insn_array.c
> index 0222a9a5d076..815f3e04540f 100644
> --- a/tools/testing/selftests/bpf/prog_tests/bpf_insn_array.c
> +++ b/tools/testing/selftests/bpf/prog_tests/bpf_insn_array.c

[ ... ]

> @@ -272,7 +236,7 @@ static void check_blindness(void)
>  		BPF_MOV64_IMM(BPF_REG_0, 1),
>  		BPF_EXIT_INSN(),
>  	};
> -	int prog_fd = -1, map_fd;
> +	int prog_fd = -1, map_fd, ret;
>  	struct bpf_insn_array_value val = {};
>  	char bpf_jit_harden = '@'; /* non-exizsting value */
>  	int i;
> @@ -291,7 +255,8 @@ static void check_blindness(void)
>  		goto cleanup;
>
>  	bpf_jit_harden = '2';
> -	if (set_bpf_jit_harden(&bpf_jit_harden)) {
> +	ret = set_bpf_jit_harden(&bpf_jit_harden);
> +	if (!ASSERT_OK(ret, "set bpf_jit_harden")) {

This isn't a bug, but could the first call just be
`if (!ASSERT_OK(set_bpf_jit_harden(&bpf_jit_harden), "set bpf_jit_harden"))`
instead?  It would match the restore call below and drop the extra `ret`
local:

>  cleanup:
>  	/* restore the old one */
>  	if (bpf_jit_harden != '@')
> -		set_bpf_jit_harden(&bpf_jit_harden);
> +		ASSERT_OK(set_bpf_jit_harden(&bpf_jit_harden),
> +			  "restore hardening configuration");

> diff --git a/tools/testing/selftests/bpf/testing_helpers.c b/tools/testing/selftests/bpf/testing_helpers.c
> index c970e7793dfc..737f668b35e2 100644
> --- a/tools/testing/selftests/bpf/testing_helpers.c
> +++ b/tools/testing/selftests/bpf/testing_helpers.c
> @@ -519,6 +519,38 @@ bool is_jit_enabled(void)
>  	return enabled;
>  }
>
> +int set_bpf_jit_harden(char *level)
> +{
> +	char old_level;
> +	int err = -1;
> +	int fd = -1;
> +
> +	fd = open("/proc/sys/net/core/bpf_jit_harden", O_RDWR | O_NONBLOCK);
> +	if (fd < 0)
> +		return -1;
> +
> +	err = read(fd, &old_level, 1);
> +	if (err != 1) {
> +		err = -1;
> +		goto end;
> +	}
> +
> +	lseek(fd, 0, SEEK_SET);
> +
> +	err = write(fd, level, 1);
> +	if (err != 1) {
> +		err = -1;
> +		goto end;
> +	}
> +
> +	err = 0;
> +	*level = old_level;
> +end:
> +	if (fd >= 0)
> +		close(fd);
> +	return err;
> +}

tools/testing/selftests/bpf/ already has a shared sysctl helper with these
semantics.  Is there a reason to add a second sysctl read-modify-write
implementation instead of wrapping the existing one?

sysctl_helpers.h (same directory) exports:

    int sysctl_set(const char *sysctl_path, char *old_val,
                   const char *new_val);
    int sysctl_set_or_fail(const char *sysctl_path, char *old_val,
                           const char *new_val);

and sysctl_helpers.c implements 'open the sysctl, save the previous value
into old_val, write new_val, return -errno on failure' - the same contract
the new set_bpf_jit_harden() open-codes with
open()/read()/lseek()/write()/close(). sysctl_set_or_fail() additionally
emits the PRINT_FAIL diagnostic that the moved helper drops.

sysctl_helpers.c is already linked into test_progs (Makefile
TRUNNER_EXTRA_SOURCES) and is already used by three prog_tests
(test_sysctl.c, token.c, unpriv_bpf_disabled.c). The only consumers of
set_bpf_jit_harden() are test_progs subtests (prog_tests/bpf_insn_array.c
here, and prog_tests/kasan.c added by the follow-up commit 7af63bad7fc6), so
sysctl_set() is reachable from every call site.

Since this patch's stated purpose is precisely to provide a shared way to
change the hardening configuration, landing a second sysctl
set-and-restore implementation means a corner-case fix in one (short
read/write handling, values longer than one byte, error reporting) will not
propagate to the other.

The immediately preceding function in the same file, is_jit_enabled() at
testing_helpers.c:505, already hand-opens /proc/sys/net/core/bpf_jit_enable
with open()/read()/close(), so the new code matches its neighbour.  Also,
the patch is a pure code motion - the body is byte-identical to the version
removed from bpf_insn_array.c apart from the deleted ASSERT_FAIL calls - so
it does not newly introduce the hand-rolled I/O, it only relocates it.
sysctl_set() reads the old value with an unbounded fscanf(fp, "%s",
old_val), which would require the caller to pass a sized buffer rather than
the single char the callers use today, so a straight substitution is not
free.

Worth asking whether set_bpf_jit_harden() should be a thin wrapper over
sysctl_set("/proc/sys/net/core/bpf_jit_harden", ...) instead?

---

This isn't a bug, but the patch is not a pure move: the three ASSERT_FAIL()
calls that reported which of open/read/write failed (with errno) are dropped
from the helper, and two ASSERT_OK() checks are added at the call sites
instead.

The reason that rework is required is not obvious from the diff -
testing_helpers.c contains no ASSERT_*/PRINT_FAIL uses at all, and
testing_helpers.o is linked into standalone binaries (test_maps, test_tag,
test_sockmap, bench) that do not provide the test_progs failure machinery,
which is why network_helpers.c's users have to define stub test__fail() in
xdp_features.c, xskxceiver.c and xdp_hw_metadata.c.

A reader also loses the ability to tell open/read/write apart, while the
retained comment at the call site still says "open, read or write failed =>
no write was done".

Would it be worth a sentence noting that the ASSERT_FAIL() diagnostics had
to move to the callers because testing_helpers.c is linked into binaries
without the test_progs assertion machinery?


---
AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

CI run summary: https://github.com/kernel-patches/bpf/actions/runs/32534828209