Re: [RFC PATCH bpf-next v3 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 `kernel.ftrace_enabled` sysctl now returns `-EOPNOTSUPP` on writes of 0, but the livepatch kselftest was not updated to reflect this, causing the test suite to unconditionally abort.
- [Low] The `FTRACE_DISABLE_CALLS` macro and its handling branch are now dead code due to the removal of `ftrace_shutdown_sysctl()`.
--

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

Writing 0 to kernel.ftrace_enabled is now deprecated and returns -EOPNOTSUPP,
as it has not reliably disabled ftrace for years. The patch updates the
sysctl handler to refuse the write, log a warning, and leaves reads and
enabling unaffected.

> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -9357,38 +9357,10 @@ static void ftrace_startup_sysctl(void)
>  	}
>  }
>  
> -static void ftrace_shutdown_sysctl(void)
> -{
> -	int command;
> -
> -	if (unlikely(ftrace_disabled))
> -		return;
> -
> -	/* ftrace_start_up is true if ftrace is running */
> -	if (ftrace_start_up) {
> -		command = FTRACE_DISABLE_CALLS;
> -		if (ftrace_graph_active)
> -			command |= FTRACE_STOP_FUNC_RET;
> -		ftrace_run_update_code(command);
> -	}
> -}

[Severity: Low]
Is FTRACE_DISABLE_CALLS now unused dead code?

The removed ftrace_shutdown_sysctl() function appears to be the only caller
that passes FTRACE_DISABLE_CALLS to ftrace_run_update_code().

Without it, the FTRACE_DISABLE_CALLS macro in include/linux/ftrace.h and
its corresponding handling branch in kernel/trace/ftrace.c might never be
executed:

kernel/trace/ftrace.c:ftrace_modify_all_code() {
    ...
    else if (command & FTRACE_DISABLE_CALLS)
        ftrace_replace_code(mod_flags);
    ...
}

Could we also clean up this macro and the dead branch?

[ ... ]

> @@ -9415,15 +9387,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 cause the livepatch kselftests to unconditionally fail?

The test script tools/testing/selftests/livepatch/test-ftrace.sh calls
set_ftrace_enabled() to disable ftrace:

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

Since writing 0 now returns -EOPNOTSUPP, the helper in functions.sh catches
the failure and aborts the entire test script:

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

Does the kselftest suite need to be updated alongside this change to avoid
breaking the tests?

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