Re: [RFC PATCH bpf-next v2 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 patch breaks livepatch kselftests by deprecating and rejecting writes to kernel.ftrace_enabled without updating the corresponding tests.
- [Low] Unused static functions `ftrace_shutdown_sysctl` and `is_permanent_ops_registered` were retained and marked with `__maybe_unused` instead of being properly removed.
--

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

ftrace: deprecate disabling via ftrace_enabled sysctl

This commit deprecates disabling ftrace via the kernel.ftrace_enabled
sysctl. It refuses writes of 0 by returning -EOPNOTSUPP and logging a
warning, rather than leaving the system in an inconsistent state.

> 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 of ftrace_shutdown_sysctl() has been removed by this
patch, should this function be deleted entirely rather than kept around with
a __maybe_unused annotation?

>  {
>  	int command;

[ ... ]

> @@ -9377,7 +9377,7 @@ static void ftrace_shutdown_sysctl(void)
>  # define ftrace_shutdown_sysctl()      do { } while (0)
>  #endif /* CONFIG_DYNAMIC_FTRACE */
>  
> -static bool is_permanent_ops_registered(void)
> +static bool __maybe_unused is_permanent_ops_registered(void)

[Severity: Low]
Similarly, is_permanent_ops_registered() appears to be completely dead code
now. Can this be safely removed instead of marking it __maybe_unused?

>  {
>  	struct ftrace_ops *op;

[ ... ]

> @@ -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 rejecting writes of 0 here break the livepatch kselftests?

Looking at tools/testing/selftests/livepatch/test-ftrace.sh, the scripts
explicitly toggle this sysctl and expect either success or an -EBUSY error
when a livepatch is active:

tools/testing/selftests/livepatch/test-ftrace.sh:
    set_ftrace_enabled 0
    load_failing_mod $MOD_LIVEPATCH
    ...
    # Check that ftrace could not get disabled when a livepatch is enabled
    set_ftrace_enabled --fail 0

Now that this returns -EOPNOTSUPP unconditionally, it looks like these
tests will fail or be skipped. Do the livepatch kselftests need to be
updated alongside this change?

>  	}
>  
>  	last_ftrace_enabled = !!ftrace_enabled;

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