Re: [RFC PATCH bpf-next 1/2] ftrace: deprecate disabling via ftrace_enabled sysctl

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The livepatch ftrace selftest is broken because it explicitly tests the deprecated `kernel.ftrace_enabled` sysctl behavior.
- [Low] The functions `ftrace_shutdown_sysctl` and `is_permanent_ops_registered` are now completely unused but are left in the codebase.
--

--- Patch [1]: [RFC PATCH bpf-next 1/2] ftrace: deprecate disabling via ftrace_enabled sysctl ---
commit b118500ef9b6245b16b7bddf0a01d982bf5b52ce
Author: Andrey Grodzovsky <[email protected]>

ftrace: deprecate disabling via ftrace_enabled sysctl

This commit deprecates disabling ftrace via the kernel.ftrace_enabled
sysctl. Writing 0 to the sysctl now returns -EOPNOTSUPP and logs a warning
instead of attempting to disable ftrace.

> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index f93e34dd23288..82bb7356ccce5 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -9357,7 +9357,7 @@ static void ftrace_startup_sysctl(void)
>  	}
>  }
>  
> -static void ftrace_shutdown_sysctl(void)
> +static void __maybe_unused ftrace_shutdown_sysctl(void)
[Severity: Low]
Since the only caller in ftrace_enable_sysctl() has been removed, should this
function be deleted entirely instead of keeping it around as dead code with
the __maybe_unused attribute?

[ ... ]

> -static bool is_permanent_ops_registered(void)
> +static bool __maybe_unused is_permanent_ops_registered(void)
[Severity: Low]
Similarly, should this function be removed entirely since it is no longer
called anywhere?

> @@ -9415,15 +9415,12 @@ ftrace_enable_sysctl(const struct ctl_table *table, int write,
>  		ftrace_startup_sysctl();
>  
>  	} else {
> -		if (is_permanent_ops_registered()) {
> -			ftrace_enabled = true;
> -			return -EBUSY;
> -		}
> -
> -		/* stopping ftrace calls (just send to ftrace_stub) */
> -		ftrace_trace_function = ftrace_stub;
> -
> -		ftrace_shutdown_sysctl();
> +		/*
> +		 * Disabling ftrace at runtime via this knob is deprecated.
> +		 */
> +		ftrace_enabled = true;
> +		pr_warn_once("The ftrace_enabled file is deprecated and no longer disables ftrace\n");
> +		return -EOPNOTSUPP;
[Severity: Medium]
Will this break the livepatch selftests?

The script tools/testing/selftests/livepatch/test-ftrace.sh explicitly attempts
to write 0 to the sysctl on line 18:

tools/testing/selftests/livepatch/test-ftrace.sh:
    set_ftrace_enabled 0

Because this is now refused, the helper function in functions.sh detects the
failure and skips:

tools/testing/selftests/livepatch/functions.sh:set_ftrace_enabled() {
    ...
    if [[ $(cat /proc/sys/kernel/ftrace_enabled) != "$1" ]]; then
        skip "failed to set kernel.ftrace_enabled = $1"
    ...
}

This skip call unconditionally exits the script, preventing the remaining tests
in the file from running. Should the selftests be updated to reflect this
change in behavior?

>  	}
>  
>  	last_ftrace_enabled = !!ftrace_enabled;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1