[PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag

[PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag

[Miroslav Benes]

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>
---
v1->v2:
- different logic, proposed by Joe Lawrence

Two things I am not sure about much:

- return codes. I chose EBUSY, because it seemed the least
  inappropriate. I usually pick the wrong one, so suggestions are
  welcome.
- I did not add any pr_* reporting the problem to make it consistent
  with the existing code.

 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..c2cad29dc557 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.
+ * PERMAMENT - 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..d2992ea29fe1 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 = last_ftrace_enabled;
+			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;
-- 
2.23.0

Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag

[Petr Mladek]

On Mon 2019-10-14 12:59:23, Miroslav Benes 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>

Looks fine to me. I finally understand which ftrace_enabled toggle
we are talking about ;-)

Reviewed-by: Petr Mladek <pmladek@suse.com>

> ---
> - return codes. I chose EBUSY, because it seemed the least
>   inappropriate. I usually pick the wrong one, so suggestions are
>   welcome.

-EBUSY is perfectly fine in ftrace_enable_sysctl(). It is not ideal
in __register_ftrace_function(). But it still looks better than
-ENODEV there.

Best Regards,
Petr

Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag

[Steven Rostedt]

On Mon, 14 Oct 2019 12:59:23 +0200
Miroslav Benes <mbenes@suse.cz> wrote:

>  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 = last_ftrace_enabled;

Although this is not incorrect, but may be somewhat confusing.

At this location, last_ftrace_enabled is always true.

I'm thinking this would be better to simply set it to false here.


> +			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:

And move the assignment of last_ftrace_enabled to after the "out:"
label.

-- Steve

>  	mutex_unlock(&ftrace_lock);
>  	return ret;

Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag

[Joe Lawrence]

On Mon, Oct 14, 2019 at 12:59:23PM +0200, Miroslav Benes 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>
> ---
> v1->v2:
> - different logic, proposed by Joe Lawrence
> 
> Two things I am not sure about much:
> 
> - return codes. I chose EBUSY, because it seemed the least
>   inappropriate. I usually pick the wrong one, so suggestions are
>   welcome.
> - I did not add any pr_* reporting the problem to make it consistent
>   with the existing code.
> 
>  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..c2cad29dc557 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.
> + * PERMAMENT - 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..d2992ea29fe1 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 = last_ftrace_enabled;
> +			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;
> -- 
> 2.23.0
>

Hi Miroslav,

Maybe we should add a test to verify this new behavior?  See sample
version below (lightly tested).  We can add to this one, or patch
seperately if you prefer.

-- Joe

-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--

 
From c8c9f22e3816ca4c90ab7e7159d2ce536eaa5fad Mon Sep 17 00:00:00 2001
From: Joe Lawrence <joe.lawrence@redhat.com>
Date: Mon, 14 Oct 2019 18:25:01 -0400
Subject: [PATCH] selftests/livepatch: test interaction with ftrace_enabled

Since livepatching depends upon ftrace handlers to implement "patched"
functionality, verify that the ftrace_enabled sysctl value interacts
with livepatch registration as expected.

Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
---
 tools/testing/selftests/livepatch/Makefile    |  3 +-
 .../testing/selftests/livepatch/functions.sh  | 18 +++++
 .../selftests/livepatch/test-ftrace.sh        | 65 +++++++++++++++++++
 3 files changed, 85 insertions(+), 1 deletion(-)
 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 79b0affd21fb..556252efece0 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -52,6 +52,24 @@ function set_dynamic_debug() {
 		EOF
 }
 
+function push_ftrace_enabled() {
+	FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled)
+}
+function pop_ftrace_enabled() {
+	if [[ -n "$FTRACE_ENABLED" ]]; then
+		sysctl kernel.ftrace_enabled="$FTRACE_ENABLED"
+	fi
+}
+# set_ftrace_enabled() - save the current ftrace_enabled config and tweak
+# 			 it for the self-tests.  Set a script exit trap
+#			 that restores the original value.
+function set_ftrace_enabled() {
+	local sysctl="$1"
+        trap pop_ftrace_enabled EXIT INT TERM HUP
+	result=$(sysctl kernel.ftrace_enabled="$1" 2>&1 | paste --serial --delimiters=' ')
+	echo "livepatch: $result" > /dev/kmsg
+}
+
 # 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-ftrace.sh b/tools/testing/selftests/livepatch/test-ftrace.sh
new file mode 100755
index 000000000000..016576883a33
--- /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
+
+set_dynamic_debug
+
+
+# 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
-- 
2.21.0

Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag

[Petr Mladek]

On Mon 2019-10-14 11:17:19, Steven Rostedt wrote:
> On Mon, 14 Oct 2019 12:59:23 +0200
> Miroslav Benes <mbenes@suse.cz> wrote:
> 
> >  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 = last_ftrace_enabled;
> 
> Although this is not incorrect, but may be somewhat confusing.
> 
> At this location, last_ftrace_enabled is always true.
> 
> I'm thinking this would be better to simply set it to false here.

IMHO, we want to set ftrace_enabled = true here.

It was set to "false" by writing to the sysfs file. But the change
gets rejected. ftrace will stay enabled. So, we should set
the value back to "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:
> 
> And move the assignment of last_ftrace_enabled to after the "out:"
> label.

This change might make sense anyway. But it is not strictly necessary
from my POV.

Best Regards,
Petr

Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag

[Miroslav Benes]

On Tue, 15 Oct 2019, Petr Mladek wrote:

> On Mon 2019-10-14 11:17:19, Steven Rostedt wrote:
> > On Mon, 14 Oct 2019 12:59:23 +0200
> > Miroslav Benes <mbenes@suse.cz> wrote:
> > 
> > >  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 = last_ftrace_enabled;
> > 
> > Although this is not incorrect, but may be somewhat confusing.
> > 
> > At this location, last_ftrace_enabled is always true.
> > 
> > I'm thinking this would be better to simply set it to false here.
> 
> IMHO, we want to set ftrace_enabled = true here.
> 
> It was set to "false" by writing to the sysfs file. But the change
> gets rejected. ftrace will stay enabled. So, we should set
> the value back to "true".

That's correct.

I can make it explicit as proposed. I just thought that 'ftrace_enabled = 
last_ftrace_enabled' was clear enough given Petr's explanation.

> > > +			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:
> > 
> > And move the assignment of last_ftrace_enabled to after the "out:"
> > label.
> 
> This change might make sense anyway. But it is not strictly necessary
> from my POV.

If it stays before "out:" label, last_ftrace_enabled is set if and only if 
it has to be set. I think it is better, but I can, of course, move it in 
v3 if Steven prefers it.

Thanks
Miroslav

Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag

[Miroslav Benes]

> Hi Miroslav,
> 
> Maybe we should add a test to verify this new behavior?  See sample
> version below (lightly tested).  We can add to this one, or patch
> seperately if you prefer.

Thanks a lot, Joe. It looks nice. I'll include it in v3. One question 
below.
 
> -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
> 
>  
> >From c8c9f22e3816ca4c90ab7e7159d2ce536eaa5fad Mon Sep 17 00:00:00 2001
> From: Joe Lawrence <joe.lawrence@redhat.com>
> Date: Mon, 14 Oct 2019 18:25:01 -0400
> Subject: [PATCH] selftests/livepatch: test interaction with ftrace_enabled
> 
> Since livepatching depends upon ftrace handlers to implement "patched"
> functionality, verify that the ftrace_enabled sysctl value interacts
> with livepatch registration as expected.
> 
> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> ---
>  tools/testing/selftests/livepatch/Makefile    |  3 +-
>  .../testing/selftests/livepatch/functions.sh  | 18 +++++
>  .../selftests/livepatch/test-ftrace.sh        | 65 +++++++++++++++++++
>  3 files changed, 85 insertions(+), 1 deletion(-)
>  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 79b0affd21fb..556252efece0 100644
> --- a/tools/testing/selftests/livepatch/functions.sh
> +++ b/tools/testing/selftests/livepatch/functions.sh
> @@ -52,6 +52,24 @@ function set_dynamic_debug() {
>  		EOF
>  }
>  
> +function push_ftrace_enabled() {
> +	FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled)
> +}

Shouldn't we call push_ftrace_enabled() somewhere at the beginning of the 
test script? set_dynamic_debug() calls its push_dynamic_debug() directly, 
but set_ftrace_enabled() is different, because it is called more than once 
in the script.

One could argue that ftrace_enabled has to be true at the beginning of 
testing anyway, but I think it would be cleaner. Btw, we should probably 
guarantee that ftrace_enabled is true when livepatch selftests are 
invoked.

> +function pop_ftrace_enabled() {
> +	if [[ -n "$FTRACE_ENABLED" ]]; then
> +		sysctl kernel.ftrace_enabled="$FTRACE_ENABLED"
> +	fi
> +}
> +# set_ftrace_enabled() - save the current ftrace_enabled config and tweak
> +# 			 it for the self-tests.  Set a script exit trap
> +#			 that restores the original value.
> +function set_ftrace_enabled() {
> +	local sysctl="$1"
> +        trap pop_ftrace_enabled EXIT INT TERM HUP
> +	result=$(sysctl kernel.ftrace_enabled="$1" 2>&1 | paste --serial --delimiters=' ')
> +	echo "livepatch: $result" > /dev/kmsg
> +}
> +
>  # 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

Miroslav

Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag

[Joe Lawrence]

On 10/15/19 7:23 AM, Miroslav Benes wrote:
>> Hi Miroslav,
>>
>> Maybe we should add a test to verify this new behavior?  See sample
>> version below (lightly tested).  We can add to this one, or patch
>> seperately if you prefer.
> 
> Thanks a lot, Joe. It looks nice. I'll include it in v3. One question
> below.
>   
>> -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
>>
>>   
>> >From c8c9f22e3816ca4c90ab7e7159d2ce536eaa5fad Mon Sep 17 00:00:00 2001
>> From: Joe Lawrence <joe.lawrence@redhat.com>
>> Date: Mon, 14 Oct 2019 18:25:01 -0400
>> Subject: [PATCH] selftests/livepatch: test interaction with ftrace_enabled
>>
>> Since livepatching depends upon ftrace handlers to implement "patched"
>> functionality, verify that the ftrace_enabled sysctl value interacts
>> with livepatch registration as expected.
>>
>> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
>> ---
>>   tools/testing/selftests/livepatch/Makefile    |  3 +-
>>   .../testing/selftests/livepatch/functions.sh  | 18 +++++
>>   .../selftests/livepatch/test-ftrace.sh        | 65 +++++++++++++++++++
>>   3 files changed, 85 insertions(+), 1 deletion(-)
>>   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 79b0affd21fb..556252efece0 100644
>> --- a/tools/testing/selftests/livepatch/functions.sh
>> +++ b/tools/testing/selftests/livepatch/functions.sh
>> @@ -52,6 +52,24 @@ function set_dynamic_debug() {
>>   		EOF
>>   }
>>   
>> +function push_ftrace_enabled() {
>> +	FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled)
>> +}
> 
> Shouldn't we call push_ftrace_enabled() somewhere at the beginning of the
> test script? set_dynamic_debug() calls its push_dynamic_debug() directly,
> but set_ftrace_enabled() is different, because it is called more than once
> in the script.
> 
> One could argue that ftrace_enabled has to be true at the beginning of
> testing anyway, but I think it would be cleaner. Btw, we should probably
> guarantee that ftrace_enabled is true when livepatch selftests are
> invoked.
> 

Ah yes, that occurred to me while creating that piece of the patch. 
Something like setup_test_config() that pushes both ftrace and the 
debugfs, etc. would be cleaner for all scripts.  If you're onboard with 
that idea, I can make that revision.

-- Joe

Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag

[Steven Rostedt]

On Tue, 15 Oct 2019 12:50:59 +0200 (CEST)
Miroslav Benes <mbenes@suse.cz> wrote:

> > > > @@ -6752,12 +6764,19 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
> > > >  		ftrace_startup_sysctl();
> > > >  
> > > >  	} else {
> > > > +		if (is_permanent_ops_registered()) {
> > > > +			ftrace_enabled = last_ftrace_enabled;  
> > > 
> > > Although this is not incorrect, but may be somewhat confusing.
> > > 
> > > At this location, last_ftrace_enabled is always true.
> > > 
> > > I'm thinking this would be better to simply set it to false here.  
> > 
> > IMHO, we want to set ftrace_enabled = true here.

Yes, I meant true (don't know why I said false :-/ )

> > 
> > It was set to "false" by writing to the sysfs file. But the change
> > gets rejected. ftrace will stay enabled. So, we should set
> > the value back to "true".  
> 
> That's correct.
> 
> I can make it explicit as proposed. I just thought that 'ftrace_enabled = 
> last_ftrace_enabled' was clear enough given Petr's explanation.
> 
> > > > +			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:  
> > > 
> > > And move the assignment of last_ftrace_enabled to after the "out:"
> > > label.  
> > 
> > This change might make sense anyway. But it is not strictly necessary
> > from my POV.  
> 
> If it stays before "out:" label, last_ftrace_enabled is set if and only if 
> it has to be set. I think it is better, but I can, of course, move it in 
> v3 if Steven prefers it.

I don't have any strong feelings here. If you want to keep it like
this, I wont argue.

-- Steve

Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag

[Miroslav Benes]

On Tue, 15 Oct 2019, Joe Lawrence wrote:

> On 10/15/19 7:23 AM, Miroslav Benes wrote:
> >> Hi Miroslav,
> >>
> >> Maybe we should add a test to verify this new behavior?  See sample
> >> version below (lightly tested).  We can add to this one, or patch
> >> seperately if you prefer.
> > 
> > Thanks a lot, Joe. It looks nice. I'll include it in v3. One question
> > below.
> >   
> >> -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--
> >>
> >>   
> >> >From c8c9f22e3816ca4c90ab7e7159d2ce536eaa5fad Mon Sep 17 00:00:00 2001
> >> From: Joe Lawrence <joe.lawrence@redhat.com>
> >> Date: Mon, 14 Oct 2019 18:25:01 -0400
> >> Subject: [PATCH] selftests/livepatch: test interaction with ftrace_enabled
> >>
> >> Since livepatching depends upon ftrace handlers to implement "patched"
> >> functionality, verify that the ftrace_enabled sysctl value interacts
> >> with livepatch registration as expected.
> >>
> >> Signed-off-by: Joe Lawrence <joe.lawrence@redhat.com>
> >> ---
> >>   tools/testing/selftests/livepatch/Makefile    |  3 +-
> >>   .../testing/selftests/livepatch/functions.sh  | 18 +++++
> >>   .../selftests/livepatch/test-ftrace.sh        | 65 +++++++++++++++++++
> >>   3 files changed, 85 insertions(+), 1 deletion(-)
> >>   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 79b0affd21fb..556252efece0 100644
> >> --- a/tools/testing/selftests/livepatch/functions.sh
> >> +++ b/tools/testing/selftests/livepatch/functions.sh
> >> @@ -52,6 +52,24 @@ function set_dynamic_debug() {
> >>   		EOF
> >>   }
> >>   
> >> +function push_ftrace_enabled() {
> >> +	FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled)
> >> +}
> > 
> > Shouldn't we call push_ftrace_enabled() somewhere at the beginning of the
> > test script? set_dynamic_debug() calls its push_dynamic_debug() directly,
> > but set_ftrace_enabled() is different, because it is called more than once
> > in the script.
> > 
> > One could argue that ftrace_enabled has to be true at the beginning of
> > testing anyway, but I think it would be cleaner. Btw, we should probably
> > guarantee that ftrace_enabled is true when livepatch selftests are
> > invoked.
> > 
> 
> Ah yes, that occurred to me while creating that piece of the patch. Something
> like setup_test_config() that pushes both ftrace and the debugfs, etc. would
> be cleaner for all scripts.  If you're onboard with that idea, I can make that
> revision.

Yes, that would be perfect. Thanks.

Miroslav

Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag

[Joe Lawrence]

On Tue, Oct 15, 2019 at 01:23:10PM +0200, Miroslav Benes wrote:
> > Hi Miroslav,
> > 
> > Maybe we should add a test to verify this new behavior?  See sample
> > version below (lightly tested).  We can add to this one, or patch
> > seperately if you prefer.
> 
> Thanks a lot, Joe. It looks nice. I'll include it in v3. One question 
> below.
>  
> [ ... snip ... ]
> 
> Shouldn't we call push_ftrace_enabled() somewhere at the beginning of the 
> test script? set_dynamic_debug() calls its push_dynamic_debug() directly, 
> but set_ftrace_enabled() is different, because it is called more than once 
> in the script.
> 
> One could argue that ftrace_enabled has to be true at the beginning of 
> testing anyway, but I think it would be cleaner. Btw, we should probably 
> guarantee that ftrace_enabled is true when livepatch selftests are 
> invoked.
> 

Here's a new version that combines the ftrace_enabled configuration with
the debugfs stuff (so its only pushed/pop once per test) and updates all
the tests accordingly.  Feel free to any tweaks or flourishes when
adding to v3.

Thanks,

-- Joe

-->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8-- -->8--

From a22ca55b3f429b7c9ceed6be87a571f77520994c Mon Sep 17 00:00:00 2001
From: Joe Lawrence <joe.lawrence@redhat.com>
Date: Tue, 15 Oct 2019 10:33:18 -0400
Subject: [PATCH] selftests/livepatch: test interaction with ftrace_enabled

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>
---
 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 +-
 6 files changed, 95 insertions(+), 13 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 79b0affd21fb..31eb09e38729 100644
--- a/tools/testing/selftests/livepatch/functions.sh
+++ b/tools/testing/selftests/livepatch/functions.sh
@@ -29,29 +29,45 @@ 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}')
+	FTRACE_ENABLED=$(sysctl --values kernel.ftrace_enabled)
 }
 
-function pop_dynamic_debug() {
+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
 }
 
-# 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
 }
 
+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 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
+}
+
 # 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-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
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
-- 
2.21.0

Re: [PATCH v2] ftrace: Introduce PERMANENT ftrace_ops flag

[Kamalesh Babulal]

On 10/14/19 4:29 PM, Miroslav Benes 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>

The patch looks good to me. A minor typo in flag description below.

Reviewed-by: Kamalesh Babulal <kamalesh@linux.vnet.ibm.com>

[...]
> diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
> index 8a8cb3c401b2..c2cad29dc557 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.
> + * PERMAMENT - Set when the ops is permanent and should not be affected by
> + *             ftrace_enabled.
>   */

s/PERMAMENT/PERMANENT

> diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
> index 62a50bf399d6..d2992ea29fe1 100644
> --- a/kernel/trace/ftrace.c
> +++ b/kernel/trace/ftrace.c
> @@ -6752,12 +6764,19 @@ ftrace_enable_sysctl(struct ctl_table *table, int write,
>  		ftrace_startup_sysctl();
> 
>  	} else {
> +		if (is_permanent_ops_registered()) {
> +			ftrace_enabled = last_ftrace_enabled;
> +			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;

No strong feelings on last_ftrace_enabled placement, leaving it to
your preference. 

>   out:
>  	mutex_unlock(&ftrace_lock);
>  	return ret;
> 


-- 
Kamalesh