[PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag

[PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag

[Miroslav Benes]

Livepatch uses ftrace for redirection to new patched functions. It is
thus directly affected by ftrace sysctl knobs such as ftrace_enabled.
Setting ftrace_enabled to 0 also disables all live patched functions. 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, the tracing of the function is not
disabled. Such ftrace_ops can still be unregistered in a standard way.

The patch set passes ftrace and livepatch kselftests.

Miroslav Benes (3):
  ftrace: Make test_rec_ops_needs_regs() generic
  ftrace: Introduce PERMANENT ftrace_ops flag
  livepatch: Use FTRACE_OPS_FL_PERMANENT

 Documentation/trace/ftrace-uses.rst |  6 ++++
 Documentation/trace/ftrace.rst      |  2 ++
 include/linux/ftrace.h              |  8 +++--
 kernel/livepatch/patch.c            |  3 +-
 kernel/trace/ftrace.c               | 47 ++++++++++++++++++++++++-----
 5 files changed, 55 insertions(+), 11 deletions(-)

-- 
2.23.0

[PATCH 1/3] ftrace: Make test_rec_ops_needs_regs() generic

[Miroslav Benes]

Function test_rec_ops_needs_regs() tests whether ftrace_ops registered
on a record needs saved regs. That is, it tests for
FTRACE_OPS_FL_SAVE_REGS being set.

The same logic will be reused for newly introduced
FTRACE_OPS_FL_PERMANENT flag, so make the function generic.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 kernel/trace/ftrace.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index 62a50bf399d6..a37c1127599c 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1602,24 +1602,24 @@ int ftrace_text_reserved(const void *start, const void *end)
 	return (int)!!ret;
 }
 
-/* Test if ops registered to this rec needs regs */
-static bool test_rec_ops_needs_regs(struct dyn_ftrace *rec)
+/* Test if ops registered to this rec needs to have a specified flag set */
+static bool test_rec_ops_needs_flag(struct dyn_ftrace *rec, int flag)
 {
 	struct ftrace_ops *ops;
-	bool keep_regs = false;
+	bool keep_flag = false;
 
 	for (ops = ftrace_ops_list;
 	     ops != &ftrace_list_end; ops = ops->next) {
 		/* pass rec in as regs to have non-NULL val */
 		if (ftrace_ops_test(ops, rec->ip, rec)) {
-			if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
-				keep_regs = true;
+			if (ops->flags & flag) {
+				keep_flag = true;
 				break;
 			}
 		}
 	}
 
-	return  keep_regs;
+	return keep_flag;
 }
 
 static struct ftrace_ops *
@@ -1750,7 +1750,8 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
 			if (ftrace_rec_count(rec) > 0 &&
 			    rec->flags & FTRACE_FL_REGS &&
 			    ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
-				if (!test_rec_ops_needs_regs(rec))
+				if (!test_rec_ops_needs_flag(rec,
+						FTRACE_OPS_FL_SAVE_REGS))
 					rec->flags &= ~FTRACE_FL_REGS;
 			}
 
-- 
2.23.0

[PATCH 2/3] 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, the tracing of the function is not
disabled. Such ftrace_ops can still be unregistered in a standard way.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 Documentation/trace/ftrace-uses.rst |  6 ++++++
 Documentation/trace/ftrace.rst      |  2 ++
 include/linux/ftrace.h              |  8 ++++++--
 kernel/trace/ftrace.c               | 32 ++++++++++++++++++++++++++++-
 4 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/Documentation/trace/ftrace-uses.rst b/Documentation/trace/ftrace-uses.rst
index 1fbc69894eed..e97b52de403f 100644
--- a/Documentation/trace/ftrace-uses.rst
+++ b/Documentation/trace/ftrace-uses.rst
@@ -170,6 +170,12 @@ FTRACE_OPS_FL_RCU
 	a callback may be executed and RCU synchronization will not protect
 	it.
 
+FTRACE_OPS_FL_PERMANENT
+        If this is set, then the tracing of the function is not disabled when
+        the proc sysctl ftrace_enabled is switched off. Livepatch uses it not
+        to lose the function redirection, so the system stays protected. The
+        callbacks with the flag set can still be unregistered.
+
 
 Filtering which functions to trace
 ==================================
diff --git a/Documentation/trace/ftrace.rst b/Documentation/trace/ftrace.rst
index e3060eedb22d..e93987a79477 100644
--- a/Documentation/trace/ftrace.rst
+++ b/Documentation/trace/ftrace.rst
@@ -2977,6 +2977,8 @@ 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).
+Functions, whose ftrace_ops are registered with FTRACE_OPS_FL_PERMANENT set,
+are the only exceptions. Tracing stays enabled there.
 
 Please disable this with care.
 
diff --git a/include/linux/ftrace.h b/include/linux/ftrace.h
index 8a8cb3c401b2..55f074f248b2 100644
--- a/include/linux/ftrace.h
+++ b/include/linux/ftrace.h
@@ -142,6 +142,7 @@ 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 removed.
  */
 enum {
 	FTRACE_OPS_FL_ENABLED			= 1 << 0,
@@ -160,6 +161,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
@@ -330,6 +332,7 @@ bool is_ftrace_trampoline(unsigned long addr);
  *  REGS_EN - the function is set up to save regs.
  *  IPMODIFY - the record allows for the IP address to be changed.
  *  DISABLED - the record is not ready to be touched yet
+ *  PERMANENT - the record is permanent, do not remove it.
  *
  * When a new ftrace_ops is registered and wants a function to save
  * pt_regs, the rec->flag REGS is set. When the function has been
@@ -345,10 +348,11 @@ enum {
 	FTRACE_FL_TRAMP_EN	= (1UL << 27),
 	FTRACE_FL_IPMODIFY	= (1UL << 26),
 	FTRACE_FL_DISABLED	= (1UL << 25),
+	FTRACE_FL_PERMANENT	= (1UL << 24),
 };
 
-#define FTRACE_REF_MAX_SHIFT	25
-#define FTRACE_FL_BITS		7
+#define FTRACE_REF_MAX_SHIFT	24
+#define FTRACE_FL_BITS		8
 #define FTRACE_FL_MASKED_BITS	((1UL << FTRACE_FL_BITS) - 1)
 #define FTRACE_FL_MASK		(FTRACE_FL_MASKED_BITS << FTRACE_REF_MAX_SHIFT)
 #define FTRACE_REF_MAX		((1UL << FTRACE_REF_MAX_SHIFT) - 1)
diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a37c1127599c..790a7c2dd0b4 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -1736,6 +1736,13 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
 			 */
 			if (ops->flags & FTRACE_OPS_FL_SAVE_REGS)
 				rec->flags |= FTRACE_FL_REGS;
+
+			/*
+			 * If any ops is permanent for this function, set it
+			 * for the record.
+			 */
+			if (ops->flags & FTRACE_OPS_FL_PERMANENT)
+				rec->flags |= FTRACE_FL_PERMANENT;
 		} else {
 			if (FTRACE_WARN_ON(ftrace_rec_count(rec) == 0))
 				return false;
@@ -1755,6 +1762,20 @@ static bool __ftrace_hash_rec_update(struct ftrace_ops *ops,
 					rec->flags &= ~FTRACE_FL_REGS;
 			}
 
+			/*
+			 * If the rec had PERMANENT enabled and the ops that is
+			 * being removed had PERMANENT set, then see if there
+			 * is still any ops for this record that wants to be
+			 * permanent.  If not, we can stop recording them.
+			 */
+			if (ftrace_rec_count(rec) > 0 &&
+			    rec->flags & FTRACE_FL_PERMANENT &&
+			    ops->flags & FTRACE_OPS_FL_PERMANENT) {
+				if (!test_rec_ops_needs_flag(rec,
+						FTRACE_OPS_FL_PERMANENT))
+					rec->flags &= ~FTRACE_FL_PERMANENT;
+			}
+
 			/*
 			 * The TRAMP needs to be set only if rec count
 			 * is decremented to one, and the ops that is
@@ -2032,6 +2053,8 @@ void ftrace_bug(int failed, struct dyn_ftrace *rec)
 		pr_info("ftrace record flags: %lx\n", rec->flags);
 		pr_cont(" (%ld)%s", ftrace_rec_count(rec),
 			rec->flags & FTRACE_FL_REGS ? " R" : "  ");
+		pr_cont(" (%ld)%s", ftrace_rec_count(rec),
+			rec->flags & FTRACE_FL_PERMANENT ? " P" : "  ");
 		if (rec->flags & FTRACE_FL_TRAMP_EN) {
 			ops = ftrace_find_tramp_ops_any(rec);
 			if (ops) {
@@ -2129,6 +2152,12 @@ static int ftrace_check_record(struct dyn_ftrace *rec, bool enable, bool update)
 		return FTRACE_UPDATE_MODIFY_CALL;
 	}
 
+	/* Do not disable the permanent record */
+	if (ftrace_rec_count(rec) &&
+	   (rec->flags & FTRACE_FL_PERMANENT)) {
+		return FTRACE_UPDATE_IGNORE;
+	}
+
 	if (update) {
 		/* If there's no more users, clear all flags */
 		if (!ftrace_rec_count(rec))
@@ -3450,9 +3479,10 @@ static int t_show(struct seq_file *m, void *v)
 	if (iter->flags & FTRACE_ITER_ENABLED) {
 		struct ftrace_ops *ops;
 
-		seq_printf(m, " (%ld)%s%s",
+		seq_printf(m, " (%ld)%s%s%s",
 			   ftrace_rec_count(rec),
 			   rec->flags & FTRACE_FL_REGS ? " R" : "  ",
+			   rec->flags & FTRACE_FL_PERMANENT ? " P" : "  ",
 			   rec->flags & FTRACE_FL_IPMODIFY ? " I" : "  ");
 		if (rec->flags & FTRACE_FL_TRAMP_EN) {
 			ops = ftrace_find_tramp_ops_any(rec);
-- 
2.23.0

[PATCH 3/3] livepatch: Use FTRACE_OPS_FL_PERMANENT

[Miroslav Benes]

Use FTRACE_OPS_FL_PERMANENT flag to be immune to toggling the
'ftrace_enabled' sysctl knob.

Signed-off-by: Miroslav Benes <mbenes@suse.cz>
---
 kernel/livepatch/patch.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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);
 
-- 
2.23.0

Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag

[Joe Lawrence]

On Mon, Oct 07, 2019 at 10:17:11AM +0200, Miroslav Benes wrote:
> Livepatch uses ftrace for redirection to new patched functions. It is
> thus directly affected by ftrace sysctl knobs such as ftrace_enabled.
> Setting ftrace_enabled to 0 also disables all live patched functions. 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, the tracing of the function is not
> disabled. Such ftrace_ops can still be unregistered in a standard way.
> 
> The patch set passes ftrace and livepatch kselftests.
> 
> Miroslav Benes (3):
>   ftrace: Make test_rec_ops_needs_regs() generic
>   ftrace: Introduce PERMANENT ftrace_ops flag
>   livepatch: Use FTRACE_OPS_FL_PERMANENT
> 
>  Documentation/trace/ftrace-uses.rst |  6 ++++
>  Documentation/trace/ftrace.rst      |  2 ++
>  include/linux/ftrace.h              |  8 +++--
>  kernel/livepatch/patch.c            |  3 +-
>  kernel/trace/ftrace.c               | 47 ++++++++++++++++++++++++-----
>  5 files changed, 55 insertions(+), 11 deletions(-)
> 
> -- 
> 2.23.0
> 

Hi Miroslav,

I wonder if the opposite would be more intuitive: when ftrace_enabled is
not set, don't allow livepatches to register ftrace filters and
likewise, don't allow ftrace_enabled to be unset if any livepatches are
already registered.  I guess you could make an argument either way, but
just offering another option.  Perhaps livepatches should follow similar
behavior of other ftrace clients (like perf probes?)

As for the approach in this patchset, is it consistent that livepatches
loaded after setting ftrace_enabled to 0 will successfully load, but not
execute their new code...  but then when ftrace_enabled is toggled, the
new livepatch code remains on?

For example:

1 - Turn ftrace_enabled off and load the /proc/cmdline livepatch test
    case, note that it reports a success patching transition, but
    doesn't run new its code:

  % dmesg -C
  % sysctl kernel.ftrace_enabled=0
  kernel.ftrace_enabled = 0
  % insmod lib/livepatch/test_klp_livepatch.ko 
  % echo $?
  0
  % dmesg
  [  450.579980] livepatch: enabling patch 'test_klp_livepatch'
  [  450.581243] livepatch: 'test_klp_livepatch': starting patching transition
  [  451.942971] livepatch: 'test_klp_livepatch': patching complete
  % cat /proc/cmdline 
  BOOT_IMAGE=(hd0,msdos1)/boot/vmlinuz-5.4.0-rc2+ root=UUID=c42bb089-b5c1-4e17-82bd-132f55bee54c ro console=ttyS0 console=ttyS0,115200n8 no_timer_check net.ifnames=0 crashkernel=auto

2 - Turn ftrace_enabled on and see that the livepatch now works:

  % sysctl kernel.ftrace_enabled=1
  kernel.ftrace_enabled = 1
  % cat /proc/cmdline 
  test_klp_livepatch: this has been live patched

3 - Turn ftrace_enabled off and see that it's still enabled:

  % sysctl kernel.ftrace_enabled=0
  kernel.ftrace_enabled = 0
  % cat /proc/cmdline 
  test_klp_livepatch: this has been live patched

Steps 2 and 3 match the behavior described by the patchset, but I was
particularly wondering what you thought about step 1.

IMHO, I would expect step 1 to fully enable the livepatch, or at the
very least, not report a patch transition (though that may confuse
userspace tools waiting for that report).

Thanks,

-- Joe

Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag

[Steven Rostedt]

On Tue, 8 Oct 2019 15:35:34 -0400
Joe Lawrence <joe.lawrence@redhat.com> wrote:

> 
> I wonder if the opposite would be more intuitive: when ftrace_enabled is
> not set, don't allow livepatches to register ftrace filters and
> likewise, don't allow ftrace_enabled to be unset if any livepatches are
> already registered.  I guess you could make an argument either way, but
> just offering another option.  Perhaps livepatches should follow similar
> behavior of other ftrace clients (like perf probes?)

I believe I suggested the "PERMANENT" flag, but disabling ftrace_enable
may be another approach. Might be much easier to maintain.

> 
> As for the approach in this patchset, is it consistent that livepatches
> loaded after setting ftrace_enabled to 0 will successfully load, but not
> execute their new code...  but then when ftrace_enabled is toggled, the
> new livepatch code remains on?
> 
> For example:
> 
> 1 - Turn ftrace_enabled off and load the /proc/cmdline livepatch test
>     case, note that it reports a success patching transition, but
>     doesn't run new its code:
> 
>   % dmesg -C
>   % sysctl kernel.ftrace_enabled=0
>   kernel.ftrace_enabled = 0
>   % insmod lib/livepatch/test_klp_livepatch.ko 
>   % echo $?
>   0
>   % dmesg
>   [  450.579980] livepatch: enabling patch 'test_klp_livepatch'
>   [  450.581243] livepatch: 'test_klp_livepatch': starting patching transition
>   [  451.942971] livepatch: 'test_klp_livepatch': patching complete
>   % cat /proc/cmdline 
>   BOOT_IMAGE=(hd0,msdos1)/boot/vmlinuz-5.4.0-rc2+ root=UUID=c42bb089-b5c1-4e17-82bd-132f55bee54c ro console=ttyS0 console=ttyS0,115200n8 no_timer_check net.ifnames=0 crashkernel=auto
> 
> 2 - Turn ftrace_enabled on and see that the livepatch now works:
> 
>   % sysctl kernel.ftrace_enabled=1
>   kernel.ftrace_enabled = 1
>   % cat /proc/cmdline 
>   test_klp_livepatch: this has been live patched
> 
> 3 - Turn ftrace_enabled off and see that it's still enabled:
> 
>   % sysctl kernel.ftrace_enabled=0
>   kernel.ftrace_enabled = 0
>   % cat /proc/cmdline 
>   test_klp_livepatch: this has been live patched
> 
> Steps 2 and 3 match the behavior described by the patchset, but I was
> particularly wondering what you thought about step 1.
> 
> IMHO, I would expect step 1 to fully enable the livepatch, or at the
> very least, not report a patch transition (though that may confuse
> userspace tools waiting for that report).
> 

I think I like your idea better. To prevent ftrace_enable from being
disabled if a "permanent" option is set. Then we only need to have a
permanent flag for the ftrace_ops, that will disable the ftrace_enable
from being cleared. We can also prevent the ftrace_ops from being
loaded if ftrace_enable is not set and the ftrace_ops has the PERMANENT
flag set.

-- Steve

Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag

[Miroslav Benes]

On Tue, 8 Oct 2019, Steven Rostedt wrote:

> On Tue, 8 Oct 2019 15:35:34 -0400
> Joe Lawrence <joe.lawrence@redhat.com> wrote:
> 
> > 
> > I wonder if the opposite would be more intuitive: when ftrace_enabled is
> > not set, don't allow livepatches to register ftrace filters and
> > likewise, don't allow ftrace_enabled to be unset if any livepatches are
> > already registered.  I guess you could make an argument either way, but
> > just offering another option.  Perhaps livepatches should follow similar
> > behavior of other ftrace clients (like perf probes?)
> 
> I believe I suggested the "PERMANENT" flag, but disabling ftrace_enable
> may be another approach. Might be much easier to maintain.

You did.
 
> > 
> > As for the approach in this patchset, is it consistent that livepatches
> > loaded after setting ftrace_enabled to 0 will successfully load, but not
> > execute their new code...  but then when ftrace_enabled is toggled, the
> > new livepatch code remains on?

No, it is not consistent and was not intended.

> > For example:
> > 
> > 1 - Turn ftrace_enabled off and load the /proc/cmdline livepatch test
> >     case, note that it reports a success patching transition, but
> >     doesn't run new its code:
> > 
> >   % dmesg -C
> >   % sysctl kernel.ftrace_enabled=0
> >   kernel.ftrace_enabled = 0
> >   % insmod lib/livepatch/test_klp_livepatch.ko 
> >   % echo $?
> >   0
> >   % dmesg
> >   [  450.579980] livepatch: enabling patch 'test_klp_livepatch'
> >   [  450.581243] livepatch: 'test_klp_livepatch': starting patching transition
> >   [  451.942971] livepatch: 'test_klp_livepatch': patching complete
> >   % cat /proc/cmdline 
> >   BOOT_IMAGE=(hd0,msdos1)/boot/vmlinuz-5.4.0-rc2+ root=UUID=c42bb089-b5c1-4e17-82bd-132f55bee54c ro console=ttyS0 console=ttyS0,115200n8 no_timer_check net.ifnames=0 crashkernel=auto
> > 
> > 2 - Turn ftrace_enabled on and see that the livepatch now works:
> > 
> >   % sysctl kernel.ftrace_enabled=1
> >   kernel.ftrace_enabled = 1
> >   % cat /proc/cmdline 
> >   test_klp_livepatch: this has been live patched
> > 
> > 3 - Turn ftrace_enabled off and see that it's still enabled:
> > 
> >   % sysctl kernel.ftrace_enabled=0
> >   kernel.ftrace_enabled = 0
> >   % cat /proc/cmdline 
> >   test_klp_livepatch: this has been live patched
> > 
> > Steps 2 and 3 match the behavior described by the patchset, but I was
> > particularly wondering what you thought about step 1.
> > 
> > IMHO, I would expect step 1 to fully enable the livepatch, or at the
> > very least, not report a patch transition (though that may confuse
> > userspace tools waiting for that report).

Yes.
 
> 
> I think I like your idea better. To prevent ftrace_enable from being
> disabled if a "permanent" option is set. Then we only need to have a
> permanent flag for the ftrace_ops, that will disable the ftrace_enable
> from being cleared. We can also prevent the ftrace_ops from being
> loaded if ftrace_enable is not set and the ftrace_ops has the PERMANENT
> flag set.

Agreed. Joe's approach is better. Let me prepare v2.

Thanks
Miroslav

Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag

[Petr Mladek]

On Tue 2019-10-08 15:35:34, Joe Lawrence wrote:
> On Mon, Oct 07, 2019 at 10:17:11AM +0200, Miroslav Benes wrote:
> > Livepatch uses ftrace for redirection to new patched functions. It is
> > thus directly affected by ftrace sysctl knobs such as ftrace_enabled.
> > Setting ftrace_enabled to 0 also disables all live patched functions. 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, the tracing of the function is not
> > disabled. Such ftrace_ops can still be unregistered in a standard way.
> > 
> > The patch set passes ftrace and livepatch kselftests.
> > 
> > Miroslav Benes (3):
> >   ftrace: Make test_rec_ops_needs_regs() generic
> >   ftrace: Introduce PERMANENT ftrace_ops flag
> >   livepatch: Use FTRACE_OPS_FL_PERMANENT
> > 
> >  Documentation/trace/ftrace-uses.rst |  6 ++++
> >  Documentation/trace/ftrace.rst      |  2 ++
> >  include/linux/ftrace.h              |  8 +++--
> >  kernel/livepatch/patch.c            |  3 +-
> >  kernel/trace/ftrace.c               | 47 ++++++++++++++++++++++++-----
> >  5 files changed, 55 insertions(+), 11 deletions(-)
> > 
> > -- 
> > 2.23.0
> > 
> 
> Hi Miroslav,
> 
> I wonder if the opposite would be more intuitive: when ftrace_enabled is
> not set, don't allow livepatches to register ftrace filters and
> likewise, don't allow ftrace_enabled to be unset if any livepatches are
> already registered.  I guess you could make an argument either way, but
> just offering another option.  Perhaps livepatches should follow similar
> behavior of other ftrace clients (like perf probes?)

I am not sure that I understand it correctly.

ftrace_enables is a global flag. My expectation is that it can be
manipulated at any time. But it should affect only ftrace handlers
without FTRACE_OPS_FL_PERMANENT flag.

By other words, the handlers with FTRACE_OPS_FL_PERMANENT flag and
only these handlers should ignore the global flag.

To be even more precise. If a function has registered more ftrace
handlers then the global ftrace_enable setting shold affect only
the handlers without the flag.

Is this the plan, please?

Best Regards,
Petr

Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag

[Joe Lawrence]

On Wed, Oct 09, 2019 at 01:22:34PM +0200, Petr Mladek wrote:
> On Tue 2019-10-08 15:35:34, Joe Lawrence wrote:
> > On Mon, Oct 07, 2019 at 10:17:11AM +0200, Miroslav Benes wrote:
> > > Livepatch uses ftrace for redirection to new patched functions. It is
> > > thus directly affected by ftrace sysctl knobs such as ftrace_enabled.
> > > Setting ftrace_enabled to 0 also disables all live patched functions. 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, the tracing of the function is not
> > > disabled. Such ftrace_ops can still be unregistered in a standard way.
> > > 
> > > The patch set passes ftrace and livepatch kselftests.
> > > 
> > > Miroslav Benes (3):
> > >   ftrace: Make test_rec_ops_needs_regs() generic
> > >   ftrace: Introduce PERMANENT ftrace_ops flag
> > >   livepatch: Use FTRACE_OPS_FL_PERMANENT
> > > 
> > >  Documentation/trace/ftrace-uses.rst |  6 ++++
> > >  Documentation/trace/ftrace.rst      |  2 ++
> > >  include/linux/ftrace.h              |  8 +++--
> > >  kernel/livepatch/patch.c            |  3 +-
> > >  kernel/trace/ftrace.c               | 47 ++++++++++++++++++++++++-----
> > >  5 files changed, 55 insertions(+), 11 deletions(-)
> > > 
> > > -- 
> > > 2.23.0
> > > 
> > 
> > Hi Miroslav,
> > 
> > I wonder if the opposite would be more intuitive: when ftrace_enabled is
> > not set, don't allow livepatches to register ftrace filters and
> > likewise, don't allow ftrace_enabled to be unset if any livepatches are
> > already registered.  I guess you could make an argument either way, but
> > just offering another option.  Perhaps livepatches should follow similar
> > behavior of other ftrace clients (like perf probes?)
> 
> I am not sure that I understand it correctly.
> 
> ftrace_enables is a global flag. My expectation is that it can be
> manipulated at any time. But it should affect only ftrace handlers
> without FTRACE_OPS_FL_PERMANENT flag.
> 
> By other words, the handlers with FTRACE_OPS_FL_PERMANENT flag and
> only these handlers should ignore the global flag.
> 
> To be even more precise. If a function has registered more ftrace
> handlers then the global ftrace_enable setting shold affect only
> the handlers without the flag.
> 

Petr,

I believe this is more or less what the patchset implemented.  I
pointed out a small inconsistency in that livepatches loaded after
ftrace_enable=0 successfully transitioned despite the ftrace handlers
not being enabled for that livepatch.  Toggling ftrace_enable would have
the side effect of enabling those handlers.

From the POV of ftrace I suppose this may be expected behavior and
nobody should be mucking with sysctl's that they don't fully understand.
However if I put on my sysadmin hat, I think I would find this slightly
confusing.  At the very least, the livepatch load should make some
mention that its replacement functions aren't actually live.

Shoring up this quirk so that the FTRACE_OPS_FL_PERMANENT always
registers and fires might be simple enough solution...


On the other hand, I suggested that the presence of
FTRACE_OPS_FL_PERMANENT flags could prevent turning off ftrace_enable
and vice versa.  That would offer the user immediate feedback without
introducing potentially unexpected (and silent) behavior.

I'm happy with either solution as long as it's consistent for the user
and easy to maintain :)

-- Joe

Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag

[Steven Rostedt]

On Wed, 9 Oct 2019 13:22:34 +0200
Petr Mladek <pmladek@suse.com> wrote:

> > Hi Miroslav,
> > 
> > I wonder if the opposite would be more intuitive: when ftrace_enabled is
> > not set, don't allow livepatches to register ftrace filters and
> > likewise, don't allow ftrace_enabled to be unset if any livepatches are
> > already registered.  I guess you could make an argument either way, but
> > just offering another option.  Perhaps livepatches should follow similar
> > behavior of other ftrace clients (like perf probes?)  
> 
> I am not sure that I understand it correctly.
> 
> ftrace_enables is a global flag. My expectation is that it can be
> manipulated at any time. But it should affect only ftrace handlers
> without FTRACE_OPS_FL_PERMANENT flag.

No, it should affect *all* ftrace_ops (which it currently does). The
addition of the "PERMANENT" flag was going to change that to what you
are saying here. But thinking about this more, I believe that is the
wrong approach.

> 
> By other words, the handlers with FTRACE_OPS_FL_PERMANENT flag and
> only these handlers should ignore the global flag.
> 
> To be even more precise. If a function has registered more ftrace
> handlers then the global ftrace_enable setting shold affect only
> the handlers without the flag.
> 
> Is this the plan, please?

I think Joe's approach is much easier to understand and implement. The
"ftrace_enabled" is a global flag, and affects all things ftrace (the
function bindings). What this patch was doing, was what you said. Make
ftrace_enabled only affect the ftrace_ops without the "PERMANENT" flag
set. But that is complex and requires a bit more accounting in the
ftrace system. Something I think we should try to avoid.

What we are now proposing, is that if "ftrace_enabled" is not set, the
register_ftrace_function() will fail if the ftrace_ops passed to it has
the PERMANENT flag set (which would cause live patching to fail to
load). It also means that if ftrace_enabled was set, and we load a
ftrace_ops with the PERMANENT flag set, and the user tries to clear
ftrace_enabled, that operation will fail. That is, you will not be able
to clear ftrace_enabled if a ftrace_ops is loaded with the PERMANENT
flag set.

You will need to have your live kernel patching user space tooling make
sure that ftrace_enabled is set before loading, but that shouldn't be a
problem.

Does that make sense?

-- Steve

Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag

[Petr Mladek]

On Wed 2019-10-09 10:26:54, Steven Rostedt wrote:
> Petr Mladek <pmladek@suse.com> wrote:
> I think Joe's approach is much easier to understand and implement. The
> "ftrace_enabled" is a global flag, and affects all things ftrace (the
> function bindings). What this patch was doing, was what you said. Make
> ftrace_enabled only affect the ftrace_ops without the "PERMANENT" flag
> set. But that is complex and requires a bit more accounting in the
> ftrace system. Something I think we should try to avoid.

From my POV, the fact that livepatches make use of ftrace handlers
is just an implementation detail. Ideally, users should not know
about it. The livepatch handlers should be handled special way
and should not be affected by the ftrace sysfs interface.
And ideally, the ftrace sysfs interface should still be available
for other ftrace users.

I would understand if this would be too complicated and we would
need to find a compromise.

> What we are now proposing, is that if "ftrace_enabled" is not set, the
> register_ftrace_function() will fail if the ftrace_ops passed to it has
> the PERMANENT flag set (which would cause live patching to fail to
> load).

From the livepatch point of view it would be more reliable if
ftrace_ops with PERMANENT flag would force "ftrace_enabled"
to be always true.

It will make the flag unusable for other ftrace users. But it
will be already be the case when it can't be disabled.

Best Regards,
Petr

Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag

[Steven Rostedt]

On Thu, 10 Oct 2019 10:50:35 +0200
Petr Mladek <pmladek@suse.com> wrote:

> It will make the flag unusable for other ftrace users. But it
> will be already be the case when it can't be disabled.

Honestly, I hate that flag. Most people don't even know about it. It
was added in the beginning of ftrace as a way to stop function tracing
in the latency tracer. But that use case has been obsoleted by
328df4759c03e ("tracing: Add function-trace option to disable function
tracing of latency tracers"). I may just remove the damn thing and only
add it back if somebody complains about it.

-- Steve

Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag

[Miroslav Benes]

On Thu, 10 Oct 2019, Steven Rostedt wrote:

> On Thu, 10 Oct 2019 10:50:35 +0200
> Petr Mladek <pmladek@suse.com> wrote:
> 
> > It will make the flag unusable for other ftrace users. But it
> > will be already be the case when it can't be disabled.
> 
> Honestly, I hate that flag. Most people don't even know about it. It
> was added in the beginning of ftrace as a way to stop function tracing
> in the latency tracer. But that use case has been obsoleted by
> 328df4759c03e ("tracing: Add function-trace option to disable function
> tracing of latency tracers"). I may just remove the damn thing and only
> add it back if somebody complains about it.

That would of course solve the issue too and code removal is always 
better.

Miroslav

Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag

[Steven Rostedt]

On Thu, 10 Oct 2019 15:38:20 +0200 (CEST)
Miroslav Benes <mbenes@suse.cz> wrote:

> On Thu, 10 Oct 2019, Steven Rostedt wrote:
> 
> > On Thu, 10 Oct 2019 10:50:35 +0200
> > Petr Mladek <pmladek@suse.com> wrote:
> >   
> > > It will make the flag unusable for other ftrace users. But it
> > > will be already be the case when it can't be disabled.  
> > 
> > Honestly, I hate that flag. Most people don't even know about it. It
> > was added in the beginning of ftrace as a way to stop function tracing
> > in the latency tracer. But that use case has been obsoleted by
> > 328df4759c03e ("tracing: Add function-trace option to disable function
> > tracing of latency tracers"). I may just remove the damn thing and only
> > add it back if somebody complains about it.  
> 
> That would of course solve the issue too and code removal is always 
> better.
>

Yes, but let's still add the patch that does the permanent check. And
then I'll put the "remove this flag" patch on top (and revert
everything else). This way, if somebody complains, and Linus reverts
the removal patch, we don't end up breaking live kernel patching
again ;-)

-- Steve

Re: [PATCH 0/3] ftrace: Introduce PERMANENT ftrace_ops flag

[Steven Rostedt]

On Thu, 10 Oct 2019 09:43:52 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Yes, but let's still add the patch that does the permanent check. And
> then I'll put the "remove this flag" patch on top (and revert
> everything else). This way, if somebody complains, and Linus reverts
> the removal patch, we don't end up breaking live kernel patching
> again ;-)


Not to mention, the PERMANENT flag patch can be marked as stable. The
removal of the switch, not so.

-- Steve