Livepatch uses ftrace for redirection to new patched functions. It means that if ftrace is disabled, all live patched functions are disabled as well. Toggling global 'ftrace_enabled' sysctl thus affect it directly. It is not a problem per se, because only administrator can set sysctl values, but it still may be surprising.
Introduce PERMANENT ftrace_ops flag to amend this. If the FTRACE_OPS_FL_PERMANENT is set on any ftrace ops, the tracing cannot be disabled by disabling ftrace_enabled. Equally, a callback with the flag set cannot be registered if ftrace_enabled is disabled.
v2->v3: - ftrace_enabled explicitly set to true - selftest from Joe Lawrence (I just split it to two patches) - typo fix
v1->v2: - different logic, proposed by Joe Lawrence
Joe Lawrence (2): selftests/livepatch: Make dynamic debug setup and restore generic selftests/livepatch: Test interaction with ftrace_enabled
Miroslav Benes (1): ftrace: Introduce PERMANENT ftrace_ops flag
Documentation/trace/ftrace-uses.rst | 8 +++ Documentation/trace/ftrace.rst | 4 +- include/linux/ftrace.h | 3 + kernel/livepatch/patch.c | 3 +- kernel/trace/ftrace.c | 23 ++++++- tools/testing/selftests/livepatch/Makefile | 3 +- .../testing/selftests/livepatch/functions.sh | 34 +++++++--- .../selftests/livepatch/test-callbacks.sh | 2 +- .../selftests/livepatch/test-ftrace.sh | 65 +++++++++++++++++++ .../selftests/livepatch/test-livepatch.sh | 2 +- .../selftests/livepatch/test-shadow-vars.sh | 2 +- 11 files changed, 132 insertions(+), 17 deletions(-) create mode 100755 tools/testing/selftests/livepatch/test-ftrace.sh
Livepatch uses ftrace for redirection to new patched functions. It means that if ftrace is disabled, all live patched functions are disabled as well. Toggling global 'ftrace_enabled' sysctl thus affect it directly. It is not a problem per se, because only administrator can set sysctl values, but it still may be surprising.
Introduce PERMANENT ftrace_ops flag to amend this. If the FTRACE_OPS_FL_PERMANENT is set on any ftrace ops, the tracing cannot be disabled by disabling ftrace_enabled. Equally, a callback with the flag set cannot be registered if ftrace_enabled is disabled.
Signed-off-by: Miroslav Benes mbenes@suse.cz Reviewed-by: Petr Mladek pmladek@suse.com Reviewed-by: Kamalesh Babulal kamalesh@linux.vnet.ibm.com --- Documentation/trace/ftrace-uses.rst | 8 ++++++++ Documentation/trace/ftrace.rst | 4 +++- include/linux/ftrace.h | 3 +++ kernel/livepatch/patch.c | 3 ++- kernel/trace/ftrace.c | 23 +++++++++++++++++++++-- 5 files changed, 37 insertions(+), 4 deletions(-)
diff --git a/Documentation/trace/ftrace-uses.rst b/Documentation/trace/ftrace-uses.rst index 1fbc69894eed..740bd0224d35 100644 --- a/Documentation/trace/ftrace-uses.rst +++ b/Documentation/trace/ftrace-uses.rst @@ -170,6 +170,14 @@ FTRACE_OPS_FL_RCU a callback may be executed and RCU synchronization will not protect it.
+FTRACE_OPS_FL_PERMANENT + If this is set on any ftrace ops, then the tracing cannot disabled by + writing 0 to the proc sysctl ftrace_enabled. Equally, a callback with + the flag set cannot be registered if ftrace_enabled is 0. + + Livepatch uses it not to lose the function redirection, so the system + stays protected. +
Filtering which functions to trace ================================== diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst index e3060eedb22d..d2b5657ed33e 100644 --- a/Documentation/trace/ftrace.rst +++ b/Documentation/trace/ftrace.rst @@ -2976,7 +2976,9 @@ Note, the proc sysctl ftrace_enable is a big on/off switch for the function tracer. By default it is enabled (when function tracing is enabled in the kernel). If it is disabled, all function tracing is disabled. This includes not only the function tracers for ftrace, but -also for any other uses (perf, kprobes, stack tracing, profiling, etc). +also for any other uses (perf, kprobes, stack tracing, profiling, etc). It +cannot be disabled if there is a callback with FTRACE_OPS_FL_PERMANENT set +registered.
Please disable this with care.
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 8a8cb3c401b2..8385cafe4f9f 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -142,6 +142,8 @@ ftrace_func_t ftrace_ops_get_func(struct ftrace_ops *ops); * PID - Is affected by set_ftrace_pid (allows filtering on those pids) * RCU - Set when the ops can only be called when RCU is watching. * TRACE_ARRAY - The ops->private points to a trace_array descriptor. + * PERMANENT - Set when the ops is permanent and should not be affected by + * ftrace_enabled. */ enum { FTRACE_OPS_FL_ENABLED = 1 << 0, @@ -160,6 +162,7 @@ enum { FTRACE_OPS_FL_PID = 1 << 13, FTRACE_OPS_FL_RCU = 1 << 14, FTRACE_OPS_FL_TRACE_ARRAY = 1 << 15, + FTRACE_OPS_FL_PERMANENT = 1 << 16, };
#ifdef CONFIG_DYNAMIC_FTRACE diff --git a/kernel/livepatch/patch.c b/kernel/livepatch/patch.c index bd43537702bd..b552cf2d85f8 100644 --- a/kernel/livepatch/patch.c +++ b/kernel/livepatch/patch.c @@ -196,7 +196,8 @@ static int klp_patch_func(struct klp_func *func) ops->fops.func = klp_ftrace_handler; ops->fops.flags = FTRACE_OPS_FL_SAVE_REGS | FTRACE_OPS_FL_DYNAMIC | - FTRACE_OPS_FL_IPMODIFY; + FTRACE_OPS_FL_IPMODIFY | + FTRACE_OPS_FL_PERMANENT;
list_add(&ops->node, &klp_ops);
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index 62a50bf399d6..5c8ad14f313a 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -325,6 +325,8 @@ int __register_ftrace_function(struct ftrace_ops *ops) if (ops->flags & FTRACE_OPS_FL_SAVE_REGS_IF_SUPPORTED) ops->flags |= FTRACE_OPS_FL_SAVE_REGS; #endif + if (!ftrace_enabled && (ops->flags & FTRACE_OPS_FL_PERMANENT)) + return -EBUSY;
if (!core_kernel_data((unsigned long)ops)) ops->flags |= FTRACE_OPS_FL_DYNAMIC; @@ -6723,6 +6725,18 @@ int unregister_ftrace_function(struct ftrace_ops *ops) } EXPORT_SYMBOL_GPL(unregister_ftrace_function);
+static bool is_permanent_ops_registered(void) +{ + struct ftrace_ops *op; + + do_for_each_ftrace_op(op, ftrace_ops_list) { + if (op->flags & FTRACE_OPS_FL_PERMANENT) + return true; + } while_for_each_ftrace_op(op); + + return false; +} + int ftrace_enable_sysctl(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, @@ -6740,8 +6754,6 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, if (ret || !write || (last_ftrace_enabled == !!ftrace_enabled)) goto out;
- last_ftrace_enabled = !!ftrace_enabled; - if (ftrace_enabled) {
/* we are starting ftrace again */ @@ -6752,12 +6764,19 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, ftrace_startup_sysctl();
} else { + if (is_permanent_ops_registered()) { + ftrace_enabled = true; + ret = -EBUSY; + goto out; + } + /* stopping ftrace calls (just send to ftrace_stub) */ ftrace_trace_function = ftrace_stub;
ftrace_shutdown_sysctl(); }
+ last_ftrace_enabled = !!ftrace_enabled; out: mutex_unlock(&ftrace_lock); return ret;
On Wed, 16 Oct 2019 13:33:13 +0200 Miroslav Benes mbenes@suse.cz wrote:
Livepatch uses ftrace for redirection to new patched functions. It means that if ftrace is disabled, all live patched functions are disabled as well. Toggling global 'ftrace_enabled' sysctl thus affect it directly. It is not a problem per se, because only administrator can set sysctl values, but it still may be surprising.
Introduce PERMANENT ftrace_ops flag to amend this. If the FTRACE_OPS_FL_PERMANENT is set on any ftrace ops, the tracing cannot be disabled by disabling ftrace_enabled. Equally, a callback with the flag set cannot be registered if ftrace_enabled is disabled.
Signed-off-by: Miroslav Benes mbenes@suse.cz Reviewed-by: Petr Mladek pmladek@suse.com Reviewed-by: Kamalesh Babulal kamalesh@linux.vnet.ibm.com
I pulled in this patch as is, but then I realized we have a race. This race has been there before this patch series, and actually, been there since the dawn of ftrace.
I realized that the user can modify ftrace_enabled out of lock context. That is, the ftrace_enabled is modified directly from the sysctl code, without taking the ftrace_lock mutex. Which means if the user was starting and stopping function tracing while playing with the ftrace_enabled switch, it could potentially cause an accounting failure.
I'm going to apply this patch on top of yours. It reverses the role of how ftrace_enabled is set in the sysctl handler. Instead of having it directly modify ftrace_enabled, I have it modify a new variable called sysctl_ftrace_enabled. I no longer need the last_ftrace_enabled. This way we only need to set or disable ftrace_enabled on a change and if all conditions are met.
Thoughts?
-- Steve
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h index 8385cafe4f9f..aa2e2c7cef9e 100644 --- a/include/linux/ftrace.h +++ b/include/linux/ftrace.h @@ -79,6 +79,7 @@ static inline int ftrace_mod_get_kallsym(unsigned int symnum, unsigned long *val #ifdef CONFIG_FUNCTION_TRACER
extern int ftrace_enabled; +extern int sysctl_ftrace_enabled; extern int ftrace_enable_sysctl(struct ctl_table *table, int write, void __user *buffer, size_t *lenp, @@ -638,6 +639,7 @@ static inline void tracer_disable(void) { #ifdef CONFIG_FUNCTION_TRACER ftrace_enabled = 0; + sysctl_ftrace_enabled = 0; #endif }
@@ -651,6 +653,7 @@ static inline int __ftrace_enabled_save(void) #ifdef CONFIG_FUNCTION_TRACER int saved_ftrace_enabled = ftrace_enabled; ftrace_enabled = 0; + sysctl_ftrace_enabled = 0; return saved_ftrace_enabled; #else return 0; @@ -661,6 +664,7 @@ static inline void __ftrace_enabled_restore(int enabled) { #ifdef CONFIG_FUNCTION_TRACER ftrace_enabled = enabled; + sysctl_ftrace_enabled = enabled; #endif }
diff --git a/kernel/sysctl.c b/kernel/sysctl.c index 00fcea236eba..773fdfc6c645 100644 --- a/kernel/sysctl.c +++ b/kernel/sysctl.c @@ -648,7 +648,7 @@ static struct ctl_table kern_table[] = { #ifdef CONFIG_FUNCTION_TRACER { .procname = "ftrace_enabled", - .data = &ftrace_enabled, + .data = &sysctl_ftrace_enabled, .maxlen = sizeof(int), .mode = 0644, .proc_handler = ftrace_enable_sysctl, diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c index dacb8b50a263..b55c9a4e2b5b 100644 --- a/kernel/trace/ftrace.c +++ b/kernel/trace/ftrace.c @@ -88,7 +88,7 @@ struct ftrace_ops ftrace_list_end __read_mostly = {
/* ftrace_enabled is a method to turn ftrace on or off */ int ftrace_enabled __read_mostly; -static int last_ftrace_enabled; +int sysctl_ftrace_enabled __read_mostly;
/* Current function tracing op */ struct ftrace_ops *function_trace_op __read_mostly = &ftrace_list_end; @@ -6221,7 +6221,7 @@ void __init ftrace_init(void) pr_info("ftrace: allocating %ld entries in %ld pages\n", count, count / ENTRIES_PER_PAGE + 1);
- last_ftrace_enabled = ftrace_enabled = 1; + sysctl_ftrace_enabled = ftrace_enabled = 1;
ret = ftrace_process_locs(NULL, __start_mcount_loc, @@ -6265,6 +6265,7 @@ struct ftrace_ops global_ops = { static int __init ftrace_nodyn_init(void) { ftrace_enabled = 1; + sysctl_ftrace_enabled = 1; return 0; } core_initcall(ftrace_nodyn_init); @@ -6714,6 +6715,7 @@ void ftrace_kill(void) { ftrace_disabled = 1; ftrace_enabled = 0; + sysctl_ftrace_enabled = 0; ftrace_trace_function = ftrace_stub; }
@@ -6796,10 +6798,12 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
ret = proc_dointvec(table, write, buffer, lenp, ppos);
- if (ret || !write || (last_ftrace_enabled == !!ftrace_enabled)) + if (ret || !write || (ftrace_enabled == !!sysctl_ftrace_enabled)) goto out;
- if (ftrace_enabled) { + if (sysctl_ftrace_enabled) { + + ftrace_enabled = true;
/* we are starting ftrace again */ if (rcu_dereference_protected(ftrace_ops_list, @@ -6810,19 +6814,21 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
} else { if (is_permanent_ops_registered()) { - ftrace_enabled = true; ret = -EBUSY; goto out; }
+ ftrace_enabled = false; + /* stopping ftrace calls (just send to ftrace_stub) */ ftrace_trace_function = ftrace_stub;
ftrace_shutdown_sysctl(); }
- last_ftrace_enabled = !!ftrace_enabled; out: + sysctl_ftrace_enabled = ftrace_enabled; + mutex_unlock(&ftrace_lock); return ret; }
On Wed, 16 Oct 2019 09:48:53 -0400 Steven Rostedt rostedt@goodmis.org wrote:
@@ -6796,10 +6798,12 @@ ftrace_enable_sysctl(struct ctl_table *table, int write, ret = proc_dointvec(table, write, buffer, lenp, ppos);
As you just stated on IRC, the update to ftrace_enabled gets updated in the above routine.
I forgot about this :-/ (Senior moment)
I guess there's nothing to worry about here.
-- Steve
- if (ret || !write || (last_ftrace_enabled == !!ftrace_enabled))
- if (ret || !write || (ftrace_enabled == !!sysctl_ftrace_enabled)) goto out;
From: Joe Lawrence joe.lawrence@redhat.com
Livepatch selftests currently save the current dynamic debug config and tweak it for the selftests. The config is restored at the end. Make the infrastructure generic, so that more variables can be saved and restored.
Signed-off-by: Joe Lawrence joe.lawrence@redhat.com Signed-off-by: Miroslav Benes mbenes@suse.cz --- .../testing/selftests/livepatch/functions.sh | 22 +++++++++++-------- .../selftests/livepatch/test-callbacks.sh | 2 +- .../selftests/livepatch/test-livepatch.sh | 2 +- .../selftests/livepatch/test-shadow-vars.sh | 2 +- 4 files changed, 16 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh index 79b0affd21fb..b7e5a67ae434 100644 --- a/tools/testing/selftests/livepatch/functions.sh +++ b/tools/testing/selftests/livepatch/functions.sh @@ -29,29 +29,33 @@ function die() { exit 1 }
-function push_dynamic_debug() { - DYNAMIC_DEBUG=$(grep '^kernel/livepatch' /sys/kernel/debug/dynamic_debug/control | \ - awk -F'[: ]' '{print "file " $1 " line " $2 " " $4}') +function push_config() { + DYNAMIC_DEBUG=$(grep '^kernel/livepatch' /sys/kernel/debug/dynamic_debug/control | \ + awk -F'[: ]' '{print "file " $1 " line " $2 " " $4}') }
-function pop_dynamic_debug() { +function pop_config() { if [[ -n "$DYNAMIC_DEBUG" ]]; then echo -n "$DYNAMIC_DEBUG" > /sys/kernel/debug/dynamic_debug/control fi }
-# set_dynamic_debug() - save the current dynamic debug config and tweak -# it for the self-tests. Set a script exit trap -# that restores the original config. function set_dynamic_debug() { - push_dynamic_debug - trap pop_dynamic_debug EXIT INT TERM HUP cat <<-EOF > /sys/kernel/debug/dynamic_debug/control file kernel/livepatch/* +p func klp_try_switch_task -p EOF }
+# setup_config - save the current config and set a script exit trap that +# restores the original config. Setup the dynamic debug +# for verbose livepatching output. +function setup_config() { + push_config + set_dynamic_debug + trap pop_config EXIT INT TERM HUP +} + # loop_until(cmd) - loop a command until it is successful or $MAX_RETRIES, # sleep $RETRY_INTERVAL between attempts # cmd - command and its arguments to run diff --git a/tools/testing/selftests/livepatch/test-callbacks.sh b/tools/testing/selftests/livepatch/test-callbacks.sh index e97a9dcb73c7..a35289b13c9c 100755 --- a/tools/testing/selftests/livepatch/test-callbacks.sh +++ b/tools/testing/selftests/livepatch/test-callbacks.sh @@ -9,7 +9,7 @@ MOD_LIVEPATCH2=test_klp_callbacks_demo2 MOD_TARGET=test_klp_callbacks_mod MOD_TARGET_BUSY=test_klp_callbacks_busy
-set_dynamic_debug +setup_config
# TEST: target module before livepatch diff --git a/tools/testing/selftests/livepatch/test-livepatch.sh b/tools/testing/selftests/livepatch/test-livepatch.sh index f05268aea859..493e3df415a1 100755 --- a/tools/testing/selftests/livepatch/test-livepatch.sh +++ b/tools/testing/selftests/livepatch/test-livepatch.sh @@ -7,7 +7,7 @@ MOD_LIVEPATCH=test_klp_livepatch MOD_REPLACE=test_klp_atomic_replace
-set_dynamic_debug +setup_config
# TEST: basic function patching diff --git a/tools/testing/selftests/livepatch/test-shadow-vars.sh b/tools/testing/selftests/livepatch/test-shadow-vars.sh index 04a37831e204..1aae73299114 100755 --- a/tools/testing/selftests/livepatch/test-shadow-vars.sh +++ b/tools/testing/selftests/livepatch/test-shadow-vars.sh @@ -6,7 +6,7 @@
MOD_TEST=test_klp_shadow_vars
-set_dynamic_debug +setup_config
# TEST: basic shadow variable API
On Wed 2019-10-16 13:33:14, Miroslav Benes wrote:
From: Joe Lawrence joe.lawrence@redhat.com
Livepatch selftests currently save the current dynamic debug config and tweak it for the selftests. The config is restored at the end. Make the infrastructure generic, so that more variables can be saved and restored.
Signed-off-by: Joe Lawrence joe.lawrence@redhat.com Signed-off-by: Miroslav Benes mbenes@suse.cz
Reviewed-by: Petr Mladek pmladek@suse.com
Best Regards, Petr
On 10/16/19 5:03 PM, Miroslav Benes wrote:
From: Joe Lawrence joe.lawrence@redhat.com
Livepatch selftests currently save the current dynamic debug config and tweak it for the selftests. The config is restored at the end. Make the infrastructure generic, so that more variables can be saved and restored.
Signed-off-by: Joe Lawrence joe.lawrence@redhat.com Signed-off-by: Miroslav Benes mbenes@suse.cz
.../testing/selftests/livepatch/functions.sh | 22 +++++++++++-------- .../selftests/livepatch/test-callbacks.sh | 2 +- .../selftests/livepatch/test-livepatch.sh | 2 +- .../selftests/livepatch/test-shadow-vars.sh | 2 +-
A minor nit pick, should the README also updated with the setup_config()?
On 10/16/19 1:10 PM, Kamalesh Babulal wrote:
On 10/16/19 5:03 PM, Miroslav Benes wrote:
From: Joe Lawrence joe.lawrence@redhat.com
Livepatch selftests currently save the current dynamic debug config and tweak it for the selftests. The config is restored at the end. Make the infrastructure generic, so that more variables can be saved and restored.
Signed-off-by: Joe Lawrence joe.lawrence@redhat.com Signed-off-by: Miroslav Benes mbenes@suse.cz
.../testing/selftests/livepatch/functions.sh | 22 +++++++++++-------- .../selftests/livepatch/test-callbacks.sh | 2 +- .../selftests/livepatch/test-livepatch.sh | 2 +- .../selftests/livepatch/test-shadow-vars.sh | 2 +-
A minor nit pick, should the README also updated with the setup_config()?
Yup, good eye. I think it should be a simple matter of s/set_dynamic_debug/setup_config/g
-- Joe
From: Joe Lawrence joe.lawrence@redhat.com
Since livepatching depends upon ftrace handlers to implement "patched" code functionality, verify that the ftrace_enabled sysctl value interacts with livepatch registration as expected. At the same time, ensure that ftrace_enabled is set and part of the test environment configuration that is saved and restored when running the selftests.
Signed-off-by: Joe Lawrence joe.lawrence@redhat.com Signed-off-by: Miroslav Benes mbenes@suse.cz --- tools/testing/selftests/livepatch/Makefile | 3 +- .../testing/selftests/livepatch/functions.sh | 14 +++- .../selftests/livepatch/test-ftrace.sh | 65 +++++++++++++++++++ 3 files changed, 80 insertions(+), 2 deletions(-) create mode 100755 tools/testing/selftests/livepatch/test-ftrace.sh
diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile index fd405402c3ff..1886d9d94b88 100644 --- a/tools/testing/selftests/livepatch/Makefile +++ b/tools/testing/selftests/livepatch/Makefile @@ -4,6 +4,7 @@ TEST_PROGS_EXTENDED := functions.sh TEST_PROGS := \ test-livepatch.sh \ test-callbacks.sh \ - test-shadow-vars.sh + test-shadow-vars.sh \ + test-ftrace.sh
include ../lib.mk diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh index b7e5a67ae434..31eb09e38729 100644 --- a/tools/testing/selftests/livepatch/functions.sh +++ b/tools/testing/selftests/livepatch/functions.sh @@ -32,12 +32,16 @@ function die() { function push_config() { DYNAMIC_DEBUG=$(grep '^kernel/livepatch' /sys/kernel/debug/dynamic_debug/control | \ awk -F'[: ]' '{print "file " $1 " line " $2 " " $4}') + FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled) }
function pop_config() { if [[ -n "$DYNAMIC_DEBUG" ]]; then echo -n "$DYNAMIC_DEBUG" > /sys/kernel/debug/dynamic_debug/control fi + if [[ -n "$FTRACE_ENABLED" ]]; then + sysctl kernel.ftrace_enabled="$FTRACE_ENABLED" &> /dev/null + fi }
function set_dynamic_debug() { @@ -47,12 +51,20 @@ function set_dynamic_debug() { EOF }
+function set_ftrace_enabled() { + local sysctl="$1" + result=$(sysctl kernel.ftrace_enabled="$1" 2>&1 | paste --serial --delimiters=' ') + echo "livepatch: $result" > /dev/kmsg +} + # setup_config - save the current config and set a script exit trap that # restores the original config. Setup the dynamic debug -# for verbose livepatching output. +# for verbose livepatching output and turn on +# the ftrace_enabled sysctl. function setup_config() { push_config set_dynamic_debug + set_ftrace_enabled 1 trap pop_config EXIT INT TERM HUP }
diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh b/tools/testing/selftests/livepatch/test-ftrace.sh new file mode 100755 index 000000000000..e2a76887f40a --- /dev/null +++ b/tools/testing/selftests/livepatch/test-ftrace.sh @@ -0,0 +1,65 @@ +#!/bin/bash +# SPDX-License-Identifier: GPL-2.0 +# Copyright (C) 2019 Joe Lawrence joe.lawrence@redhat.com + +. $(dirname $0)/functions.sh + +MOD_LIVEPATCH=test_klp_livepatch + +setup_config + + +# TEST: livepatch interaction with ftrace_enabled sysctl +# - turn ftrace_enabled OFF and verify livepatches can't load +# - turn ftrace_enabled ON and verify livepatch can load +# - verify that ftrace_enabled can't be turned OFF while a livepatch is loaded + +echo -n "TEST: livepatch interaction with ftrace_enabled sysctl ... " +dmesg -C + +set_ftrace_enabled 0 +load_failing_mod $MOD_LIVEPATCH + +set_ftrace_enabled 1 +load_lp $MOD_LIVEPATCH +if [[ "$(cat /proc/cmdline)" != "$MOD_LIVEPATCH: this has been live patched" ]] ; then + echo -e "FAIL\n\n" + die "livepatch kselftest(s) failed" +fi + +set_ftrace_enabled 0 +if [[ "$(cat /proc/cmdline)" != "$MOD_LIVEPATCH: this has been live patched" ]] ; then + echo -e "FAIL\n\n" + die "livepatch kselftest(s) failed" +fi +disable_lp $MOD_LIVEPATCH +unload_lp $MOD_LIVEPATCH + +check_result "livepatch: kernel.ftrace_enabled = 0 +% modprobe $MOD_LIVEPATCH +livepatch: enabling patch '$MOD_LIVEPATCH' +livepatch: '$MOD_LIVEPATCH': initializing patching transition +livepatch: failed to register ftrace handler for function 'cmdline_proc_show' (-16) +livepatch: failed to patch object 'vmlinux' +livepatch: failed to enable patch '$MOD_LIVEPATCH' +livepatch: '$MOD_LIVEPATCH': canceling patching transition, going to unpatch +livepatch: '$MOD_LIVEPATCH': completing unpatching transition +livepatch: '$MOD_LIVEPATCH': unpatching complete +modprobe: ERROR: could not insert '$MOD_LIVEPATCH': Device or resource busy +livepatch: kernel.ftrace_enabled = 1 +% modprobe $MOD_LIVEPATCH +livepatch: enabling patch '$MOD_LIVEPATCH' +livepatch: '$MOD_LIVEPATCH': initializing patching transition +livepatch: '$MOD_LIVEPATCH': starting patching transition +livepatch: '$MOD_LIVEPATCH': completing patching transition +livepatch: '$MOD_LIVEPATCH': patching complete +livepatch: sysctl: setting key "kernel.ftrace_enabled": Device or resource busy kernel.ftrace_enabled = 0 +% echo 0 > /sys/kernel/livepatch/$MOD_LIVEPATCH/enabled +livepatch: '$MOD_LIVEPATCH': initializing unpatching transition +livepatch: '$MOD_LIVEPATCH': starting unpatching transition +livepatch: '$MOD_LIVEPATCH': completing unpatching transition +livepatch: '$MOD_LIVEPATCH': unpatching complete +% rmmod $MOD_LIVEPATCH" + + +exit 0
On Wed 2019-10-16 13:33:15, Miroslav Benes wrote:
From: Joe Lawrence joe.lawrence@redhat.com
Since livepatching depends upon ftrace handlers to implement "patched" code functionality, verify that the ftrace_enabled sysctl value interacts with livepatch registration as expected. At the same time, ensure that ftrace_enabled is set and part of the test environment configuration that is saved and restored when running the selftests.
Signed-off-by: Joe Lawrence joe.lawrence@redhat.com Signed-off-by: Miroslav Benes mbenes@suse.cz
tools/testing/selftests/livepatch/Makefile | 3 +- .../testing/selftests/livepatch/functions.sh | 14 +++- .../selftests/livepatch/test-ftrace.sh | 65 +++++++++++++++++++ 3 files changed, 80 insertions(+), 2 deletions(-) create mode 100755 tools/testing/selftests/livepatch/test-ftrace.sh
diff --git a/tools/testing/selftests/livepatch/Makefile b/tools/testing/selftests/livepatch/Makefile index fd405402c3ff..1886d9d94b88 100644 --- a/tools/testing/selftests/livepatch/Makefile +++ b/tools/testing/selftests/livepatch/Makefile @@ -4,6 +4,7 @@ TEST_PROGS_EXTENDED := functions.sh TEST_PROGS := \ test-livepatch.sh \ test-callbacks.sh \
- test-shadow-vars.sh
- test-shadow-vars.sh \
- test-ftrace.sh
include ../lib.mk diff --git a/tools/testing/selftests/livepatch/functions.sh b/tools/testing/selftests/livepatch/functions.sh index b7e5a67ae434..31eb09e38729 100644 --- a/tools/testing/selftests/livepatch/functions.sh +++ b/tools/testing/selftests/livepatch/functions.sh @@ -32,12 +32,16 @@ function die() { function push_config() { DYNAMIC_DEBUG=$(grep '^kernel/livepatch' /sys/kernel/debug/dynamic_debug/control | \ awk -F'[: ]' '{print "file " $1 " line " $2 " " $4}')
- FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled)
} function pop_config() { if [[ -n "$DYNAMIC_DEBUG" ]]; then echo -n "$DYNAMIC_DEBUG" > /sys/kernel/debug/dynamic_debug/control fi
- if [[ -n "$FTRACE_ENABLED" ]]; then
sysctl kernel.ftrace_enabled="$FTRACE_ENABLED" &> /dev/null- fi
} function set_dynamic_debug() { @@ -47,12 +51,20 @@ function set_dynamic_debug() { EOF } +function set_ftrace_enabled() {
- local sysctl="$1"
The variable is not later used.
- result=$(sysctl kernel.ftrace_enabled="$1" 2>&1 | paste --serial --delimiters=' ')
- echo "livepatch: $result" > /dev/kmsg
+}
Otherwise, the patch looks good to me. After removing or using the variable:
Reviewed-by: Petr Mladek pmladek@suse.com
Best Regards, Petr
On 10/16/19 5:03 PM, Miroslav Benes wrote:
From: Joe Lawrence joe.lawrence@redhat.com
Since livepatching depends upon ftrace handlers to implement "patched" code functionality, verify that the ftrace_enabled sysctl value interacts with livepatch registration as expected. At the same time, ensure that ftrace_enabled is set and part of the test environment configuration that is saved and restored when running the selftests.
Signed-off-by: Joe Lawrence joe.lawrence@redhat.com Signed-off-by: Miroslav Benes mbenes@suse.cz
[...]
diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh b/tools/testing/selftests/livepatch/test-ftrace.sh new file mode 100755 index 000000000000..e2a76887f40a --- /dev/null +++ b/tools/testing/selftests/livepatch/test-ftrace.sh
This test fails due to wrong file permissions, with the warning:
# Warning: file test-ftrace.sh is not executable, correct this. not ok 4 selftests: livepatch: test-ftrace.sh
On 10/16/19 1:06 PM, Kamalesh Babulal wrote:
On 10/16/19 5:03 PM, Miroslav Benes wrote:
From: Joe Lawrence joe.lawrence@redhat.com
Since livepatching depends upon ftrace handlers to implement "patched" code functionality, verify that the ftrace_enabled sysctl value interacts with livepatch registration as expected. At the same time, ensure that ftrace_enabled is set and part of the test environment configuration that is saved and restored when running the selftests.
Signed-off-by: Joe Lawrence joe.lawrence@redhat.com Signed-off-by: Miroslav Benes mbenes@suse.cz
[...]
diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh b/tools/testing/selftests/livepatch/test-ftrace.sh new file mode 100755 index 000000000000..e2a76887f40a --- /dev/null +++ b/tools/testing/selftests/livepatch/test-ftrace.sh
This test fails due to wrong file permissions, with the warning:
# Warning: file test-ftrace.sh is not executable, correct this. not ok 4 selftests: livepatch: test-ftrace.sh
Hi Kamalesh,
Thanks for testing. This error is weird as the git log says new file mode: 100755. 'git am' of Miroslav's patchset gives me a new test-ftrace.sh with "Access: (0775/-rwxrwxr-x)" Did you apply the mbox directly or.. ?
-- Joe
On 10/17/19 3:17 AM, Joe Lawrence wrote:
On 10/16/19 1:06 PM, Kamalesh Babulal wrote:
On 10/16/19 5:03 PM, Miroslav Benes wrote:
From: Joe Lawrence joe.lawrence@redhat.com
Since livepatching depends upon ftrace handlers to implement "patched" code functionality, verify that the ftrace_enabled sysctl value interacts with livepatch registration as expected. At the same time, ensure that ftrace_enabled is set and part of the test environment configuration that is saved and restored when running the selftests.
Signed-off-by: Joe Lawrence joe.lawrence@redhat.com Signed-off-by: Miroslav Benes mbenes@suse.cz
[...]
diff --git a/tools/testing/selftests/livepatch/test-ftrace.sh b/tools/testing/selftests/livepatch/test-ftrace.sh new file mode 100755 index 000000000000..e2a76887f40a --- /dev/null +++ b/tools/testing/selftests/livepatch/test-ftrace.sh
This test fails due to wrong file permissions, with the warning:
# Warning: file test-ftrace.sh is not executable, correct this. not ok 4 selftests: livepatch: test-ftrace.sh
Hi Kamalesh,
Thanks for testing. This error is weird as the git log says new file mode: 100755. 'git am' of Miroslav's patchset gives me a new test-ftrace.sh with "Access: (0775/-rwxrwxr-x)" Did you apply the mbox directly or.. ?
Hi Joe,
Thanks, I was using patch command to apply the patches and using git am helped. Sorry for the noise. The test cases passes now, without the issue I previously reported:
[...] # TEST: livepatch interaction with ftrace_enabled sysctl ... ok ok 4 selftests: livepatch: test-ftrace.sh
Long story is that the patch command version installed on the test machine doesn't understand new file mode permission from the git diff, updating the patch version creates the patch with 755 mode.
On Wed, 16 Oct 2019 13:33:12 +0200 Miroslav Benes mbenes@suse.cz wrote:
Livepatch uses ftrace for redirection to new patched functions. It means that if ftrace is disabled, all live patched functions are disabled as well. Toggling global 'ftrace_enabled' sysctl thus affect it directly. It is not a problem per se, because only administrator can set sysctl values, but it still may be surprising.
Introduce PERMANENT ftrace_ops flag to amend this. If the FTRACE_OPS_FL_PERMANENT is set on any ftrace ops, the tracing cannot be disabled by disabling ftrace_enabled. Equally, a callback with the flag set cannot be registered if ftrace_enabled is disabled.
v2->v3:
- ftrace_enabled explicitly set to true
- selftest from Joe Lawrence (I just split it to two patches)
- typo fix
v1->v2:
- different logic, proposed by Joe Lawrence
Joe Lawrence (2): selftests/livepatch: Make dynamic debug setup and restore generic selftests/livepatch: Test interaction with ftrace_enabled
Miroslav Benes (1): ftrace: Introduce PERMANENT ftrace_ops flag
Would you like me to take all three patches through my tree?
-- Steve
On Wed, 16 Oct 2019, Steven Rostedt wrote:
On Wed, 16 Oct 2019 13:33:12 +0200 Miroslav Benes mbenes@suse.cz wrote:
Livepatch uses ftrace for redirection to new patched functions. It means that if ftrace is disabled, all live patched functions are disabled as well. Toggling global 'ftrace_enabled' sysctl thus affect it directly. It is not a problem per se, because only administrator can set sysctl values, but it still may be surprising.
Introduce PERMANENT ftrace_ops flag to amend this. If the FTRACE_OPS_FL_PERMANENT is set on any ftrace ops, the tracing cannot be disabled by disabling ftrace_enabled. Equally, a callback with the flag set cannot be registered if ftrace_enabled is disabled.
v2->v3:
- ftrace_enabled explicitly set to true
- selftest from Joe Lawrence (I just split it to two patches)
- typo fix
v1->v2:
- different logic, proposed by Joe Lawrence
Joe Lawrence (2): selftests/livepatch: Make dynamic debug setup and restore generic selftests/livepatch: Test interaction with ftrace_enabled
Miroslav Benes (1): ftrace: Introduce PERMANENT ftrace_ops flag
Would you like me to take all three patches through my tree?
I think that would be the easiest, yes.
Thanks Miroslav
linux-kselftest-mirror@lists.linaro.org
-
Joe Lawrence -
Kamalesh Babulal -
Miroslav Benes -
Petr Mladek -
Steven Rostedt