Re: [RFC PATCH bpf-next 3/3] selftests/bpf: add ftrace_permanent test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] The test dynamically alters a global system-wide setting (`kernel.ftrace_enabled`) but is not marked as a serial test, which will cause flaky test failures during default parallel execution.
- [Medium] The test does not explicitly ensure `kernel.ftrace_enabled = 1` after reading its original state, causing guaranteed test failures if the testing environment starts with ftrace disabled.
- [Low] A multi-line comment violates the BPF subsystem's mandatory comment style.
--

--- Patch [3]: [RFC PATCH bpf-next 3/3] selftests/bpf: add ftrace_permanent test ---
commit 16e4b9c3d2d63207f5f4817fc1dedd224a76749e
Author: Andrey Grodzovsky <andrey.grodzovsky@crowdstrike.com>

selftests/bpf: add ftrace_permanent test

This patch introduces a selftest to verify that kernel.ftrace_enabled=0 is 
refused with EBUSY when fentry, fexit, kprobe, or kretprobe BPF programs are 
attached. It confirms that the sysctl is allowed again once the programs are 
detached, and skips kprobes testing on architectures without 
CONFIG_KPROBES_ON_FTRACE.

> diff --git a/tools/testing/selftests/bpf/prog_tests/ftrace_permanent.c b/tools/testing/selftests/bpf/prog_tests/ftrace_permanent.c
> index 0000000000000..dbe78009a2913
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/prog_tests/ftrace_permanent.c

[ ... ]

> +void test_ftrace_permanent(void)
> +{

[Severity: Medium]
Since this test dynamically alters a global system-wide setting 
(kernel.ftrace_enabled), could this cause flaky failures for other concurrently 
running BPF selftests? 

Should this function be named serial_test_ftrace_permanent to ensure the test 
runner executes it exclusively?

> +	struct ftrace_permanent *skel;
> +	bool kprobes_on_ftrace;
> +	int orig = 1;
> +
> +	/* Save and always restore ftrace_enabled. */
> +	if (read_ftrace_enabled(&orig)) {
> +		test__skip();
> +		return;
> +	}
> +

[Severity: Medium]
If the testing environment happens to start with ftrace disabled (orig is 0), 
will check_blocks_disable() unconditionally fail when it tries to attach 
programs? 

Does the test need to explicitly set kernel.ftrace_enabled = 1 here before 
proceeding?

> +	skel = ftrace_permanent__open_and_load();

[ ... ]

> diff --git a/tools/testing/selftests/bpf/progs/ftrace_permanent.c b/tools/testing/selftests/bpf/progs/ftrace_permanent.c
> index 0000000000000..ca706d8edc23b
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/ftrace_permanent.c

[ ... ]

> +extern bool CONFIG_KPROBES_ON_FTRACE __kconfig __weak;
> +
> +/* This function is here to have CONFIG_KPROBES_ON_FTRACE used and
> + * added to object BTF, so the userspace side can read it back via
> + * skel->kconfig->CONFIG_KPROBES_ON_FTRACE.
> + */

[Severity: Low]
This isn't a bug, but this multi-line comment has text on the same line as the 
opening /*, which conflicts with the BPF subsystem's required comment style.

> +int unused(void)
> +{
> +	return CONFIG_KPROBES_ON_FTRACE ? 0 : 1;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729005959.3853865-1-andrey.grodzovsky@crowdstrike.com?part=3