[PATCH 0/4] ftrace: One more check on x86 and some small fixes

[PATCH 0/4] ftrace: One more check on x86 and some small fixes

[Petr Mladek]

1st patch adds one more paranoid check of the modified function on x86.

Plus there are 3 small changes that appeared when hunting down
the 1st patch.

Petr Mladek (4):
  ftrace/x86: Make sure to modify 5-bite instructions
  ftrace/x86: Do not crash when reading wrong ftrace func
  ftrace: Always destroy trampoline when shutting down the trace
  ftrace: Fixup trace_selftest_ops()

 arch/x86/kernel/ftrace.c      | 12 ++++++++-
 kernel/trace/ftrace.c         | 62 ++++++++++++++++++++++---------------------
 kernel/trace/trace_selftest.c |  7 ++---
 3 files changed, 47 insertions(+), 34 deletions(-)

-- 
1.8.5.6

[PATCH 1/4] ftrace/x86: Make sure to modify 5-bite instructions

[Petr Mladek]

The commit 8329e818f14926a604 ("ftrace/x86: Set ftrace_stub to weak to
prevent gcc from using short jumps to it") fixed a situation where we
replaced 2-byte with 5-byte jump instruction.

The original problem crashed the kernel with

  # cd /sys/kernel/trace
  # echo function_graph > current_tracer
  # echo nop > current_tracer

  [   78.122055] invalid opcode: 0000 [#1] SMP
  [   78.125125] Modules linked in: x86_pkg_temp_thermal kvm_intel kvm irqbypass \
crc32c_intel pcspkr iwldvm iwlwifi  [   78.128241] CPU: 2 PID: 17 Comm: migration/2 \
Not tainted 4.6.0-rc4+ #36  [   78.131310] Hardware name: LENOVO 4286A74/4286A74, \
BIOS 8DET56WW (1.26 ) 12/01/2011  [   78.134369] task: ffff88040bec4240 ti: \
ffff88040bee4000 task.ti: ffff88040bee4000  [   78.137412] RIP: \
0010:[<ffffffff818939a8>]  [<ffffffff818939a8>] ftrace_stub+0x0/0x8  [   78.140477] \
RSP: 0018:ffff88040bee7e48  EFLAGS: 00010246  [   78.143489] RAX: ffff88040bec4240 \
RBX: ffffffff8107a7b0 RCX: 000000000000001f  [   78.146512] RDX: 0000000000000000 \
RSI: ffff88041e2929d8 RDI: ffff88040bee7e50  [   78.149581] RBP: ffff88040bee7e80 \
R08: ffff88040bee4000 R09: 0000000000000000  [   78.152647] R10: 00000000000318b7 \
R11: ffff8800d661f800 R12: ffff88040d8011b0  [   78.155679] R13: ffffffff81e43620 \
R14: ffff88040bda8588 R15: ffffffff81e503e0  [   78.158675] FS:  \
0000000000000000(0000) GS:ffff88041e280000(0000) knlGS:0000000000000000  [   \
78.161699] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033  [   78.164690] CR2: \
00007fadb22dde1d CR3: 00000000d5ce2000 CR4: 00000000000406e0  [   78.167691] Stack:
  [   78.170658]  ffffffff8110b3ee ffffffff8188de90 00000009161d55f6 00000012306fc2e4
  [   78.173710]  0000000000000000 ffff880400000000 ffff88040bec4240 ffff88040bee7ec8
  [   78.176783]  ffffffff81893bbd 0000000000000000 ffff88040bec4240 ffffffff81893ba8
  [   78.179863] Call Trace:
  [   78.182853]  [<ffffffff8110b3ee>] ? ftrace_return_to_handler+0x8e/0x100
  [   78.185909]  [<ffffffff8188de90>] ? __schedule+0xae0/0xae0
  [   78.188941]  [<ffffffff81893bbd>] return_to_handler+0x15/0x27
  [   78.192001]  [<ffffffff81893ba8>] ? ftrace_graph_caller+0xa8/0xa8
  [   78.195091]  [<ffffffff8107a6f0>] ? sort_range+0x30/0x30
  [   78.198138]  [<ffffffff810778a9>] kthread+0xc9/0xe0
  [   78.201143]  [<ffffffff81891a12>] ret_from_fork+0x22/0x40
  [   78.204138]  [<ffffffff810777e0>] ? kthread_worker_fn+0x170/0x170
  [   78.207129] Code: 8b 44 24 48 48 8b 7c 24 70 48 8b 74 24 68 48 8b 54 24 60
                       48 8b 4c 24 58 48 8b 44 24 50 48 8b 6c 24 20 48 81 c4 d0
                       00 00 00 e9 fd <ff> ff ff 80 00 00 00 00 9c 55 ff 74 24 18
                       55 48 89 e5 ff 74 24
  [   78.213997] RIP  [<ffffffff818939a8>] ftrace_stub+0x0/0x8
  [   78.217190]  RSP <ffff88040bee7e48>
  [   78.220374] ---[ end trace 0af2f0d9f7301011 ]---

, see
https://lkml.kernel.org/g/1463147623-2575-1-git-send-email-namhyung@kernel.org

We already avoid similar crashes by paranoid checks, for example
in add_break() or ftrace_modify_code_direct(). These checks did
not help in the above situation because the old expected code
was read from the location that was modified. See the memcpy()
in update_ftrace_func().

Normally, the expected code is generated according to the expected
state of the modified location. update_ftrace_func() is called
in several situations. It is not easy to generate the expected code
a reasonable way. But the function seems to be called only when
we modify a call or a jump instruction. We could at least check
that it is the 5-byte variant.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 arch/x86/kernel/ftrace.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index d036cfb4495d..42ea69d35dfd 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -235,6 +235,15 @@ static int update_ftrace_func(unsigned long ip, void *new)
 
 	memcpy(old, (void *)ip, MCOUNT_INSN_SIZE);
 
+	/*
+	 * Make sure that we replace 5-byte instruction that
+	 * is either a call or a jump.
+	 */
+	if (old[0] != 0xe8 && old[0] != 0xe9) {
+		pr_warn("Expected e8 or e9, got %x\n", old[0]);
+		return -EINVAL;
+	}
+
 	ftrace_update_func = ip;
 	/* Make sure the breakpoints see the ftrace_update_func update */
 	smp_wmb();
-- 
1.8.5.6

[PATCH 2/4] ftrace/x86: Do not crash when reading wrong ftrace func

[Petr Mladek]

Ftrace modifies the code on many locations. It is paranoid
and avoid a kernel crash using probe_kernel_read() and
probe_kernel_write(). The only exception is update_ftrace_func()
where where we read the old code using memcpy().

It is true that this function is used only to modify well
defined functions that are part of the ftrace API. But
it might still make sense to be paranoid and be consistent
with the writing side.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 arch/x86/kernel/ftrace.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 42ea69d35dfd..8305c6792ad2 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -233,7 +233,8 @@ static int update_ftrace_func(unsigned long ip, void *new)
 	unsigned char old[MCOUNT_INSN_SIZE];
 	int ret;
 
-	memcpy(old, (void *)ip, MCOUNT_INSN_SIZE);
+	if (probe_kernel_read(old, (void *)ip, MCOUNT_INSN_SIZE))
+		return -EFAULT;
 
 	/*
 	 * Make sure that we replace 5-byte instruction that
-- 
1.8.5.6

[PATCH 3/4] ftrace: Always destroy trampoline when shutting down the trace

[Petr Mladek]

If have got the following kmemleak report when I enabled the ftrace self
test:

unreferenced object 0xffffffffa000a000 (size 179):
  comm "swapper/0", pid 1, jiffies 4294892507 (age 82553.780s)
  hex dump (first 32 bytes):
    55 ff 74 24 10 55 48 89 e5 ff 74 24 18 55 48 89  U.t$.UH...t$.UH.
    e5 48 81 ec a8 00 00 00 48 89 44 24 50 48 89 4c  .H......H.D$PH.L
  backtrace:
    [<ffffffff81915918>] kmemleak_alloc+0x28/0x50
    [<ffffffff811d7474>] __vmalloc_node_range+0x184/0x270
    [<ffffffff8104a463>] module_alloc+0x63/0x70
    [<ffffffff81046726>] arch_ftrace_update_trampoline+0x96/0x230
    [<ffffffff81149de6>] ftrace_startup+0x76/0x200
    [<ffffffff8114a330>] register_ftrace_function+0x50/0x70
    [<ffffffff8118e312>] trace_selftest_ops+0x1b8/0x308
    [<ffffffff81ff8b20>] trace_selftest_startup_function+0x25b/0x4a8
    [<ffffffff81ff9002>] register_tracer+0x1a7/0x2ec
    [<ffffffff81ff9753>] init_function_trace+0x90/0x92
    [<ffffffff810003b8>] do_one_initcall+0x88/0x1c0
    [<ffffffff81fd22dc>] kernel_init_freeable+0x1f9/0x289
    [<ffffffff81912d1e>] kernel_init+0xe/0x110
    [<ffffffff81920cf2>] ret_from_fork+0x22/0x50
    [<ffffffffffffffff>] 0xffffffffffffffff

It is ops->trampoline that gets allocated for dyn_ops in
trace_selftest_ops(). It is from the 2nd round when cnt == 2.
In this case, the default trampoline must be used because
there are two callbacks added to all functions, namely
trace_selftest_test_global_func() and trace_selftest_test_dyn_func().

It seems that we forget to free ops->trampoline when the ftrace function
gets unregistered and the trampoline is not being used.

This patch creates ftrace_ops_free() to share the code and avoid
such problems in the future.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/trace/ftrace.c | 62 ++++++++++++++++++++++++++-------------------------
 1 file changed, 32 insertions(+), 30 deletions(-)

diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c
index a6804823a058..39f90bdb0f44 100644
--- a/kernel/trace/ftrace.c
+++ b/kernel/trace/ftrace.c
@@ -2678,6 +2678,36 @@ static int ftrace_startup(struct ftrace_ops *ops, int command)
 	return 0;
 }
 
+/*
+ * Dynamic ops may be freed, we must make sure that all callers are done
+ * before leaving this function. The same goes for freeing the per_cpu
+ * data of the per_cpu ops.
+ *
+ * Again, normal synchronize_sched() is not good enough. We need to do
+ * a hard force of sched synchronization. This is because we use
+ * preempt_disable() to do RCU, but the function tracers can be called
+ * where RCU is not watching (like before user_exit()). We can not rely
+ * on the RCU infrastructure to do the synchronization, thus we must do
+ * it ourselves.
+ *
+ * The synchronization is not needed when the function tracing is not
+ * active at the moment.
+ */
+static void ftrace_ops_free(struct ftrace_ops *ops, bool sync)
+{
+	if (!(ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_PER_CPU)))
+		return;
+
+	if (sync)
+		schedule_on_each_cpu(ftrace_sync);
+
+	if (ops->flags & FTRACE_OPS_FL_DYNAMIC)
+		arch_ftrace_trampoline_free(ops);
+
+	if (ops->flags & FTRACE_OPS_FL_PER_CPU)
+		per_cpu_ops_free(ops);
+}
+
 static int ftrace_shutdown(struct ftrace_ops *ops, int command)
 {
 	int ret;
@@ -2711,14 +2741,7 @@ static int ftrace_shutdown(struct ftrace_ops *ops, int command)
 	}
 
 	if (!command || !ftrace_enabled) {
-		/*
-		 * If these are per_cpu ops, they still need their
-		 * per_cpu field freed. Since, function tracing is
-		 * not currently active, we can just free them
-		 * without synchronizing all CPUs.
-		 */
-		if (ops->flags & FTRACE_OPS_FL_PER_CPU)
-			per_cpu_ops_free(ops);
+		ftrace_ops_free(ops, false);
 		return 0;
 	}
 
@@ -2756,28 +2779,7 @@ static int ftrace_shutdown(struct ftrace_ops *ops, int command)
 	removed_ops = NULL;
 	ops->flags &= ~FTRACE_OPS_FL_REMOVING;
 
-	/*
-	 * Dynamic ops may be freed, we must make sure that all
-	 * callers are done before leaving this function.
-	 * The same goes for freeing the per_cpu data of the per_cpu
-	 * ops.
-	 *
-	 * Again, normal synchronize_sched() is not good enough.
-	 * We need to do a hard force of sched synchronization.
-	 * This is because we use preempt_disable() to do RCU, but
-	 * the function tracers can be called where RCU is not watching
-	 * (like before user_exit()). We can not rely on the RCU
-	 * infrastructure to do the synchronization, thus we must do it
-	 * ourselves.
-	 */
-	if (ops->flags & (FTRACE_OPS_FL_DYNAMIC | FTRACE_OPS_FL_PER_CPU)) {
-		schedule_on_each_cpu(ftrace_sync);
-
-		arch_ftrace_trampoline_free(ops);
-
-		if (ops->flags & FTRACE_OPS_FL_PER_CPU)
-			per_cpu_ops_free(ops);
-	}
+	ftrace_ops_free(ops, true);
 
 	return 0;
 }
-- 
1.8.5.6

[PATCH 4/4] ftrace: Fixup trace_selftest_ops()

[Petr Mladek]

I was hunting a memory leak related to trace_selftest_ops() and found
two small mistakes there.

First, the dynamic trace must be freed in case of error. There was
one wrong goto target. It was even in the original commit
95950c2ecb31314ef ("ftrace: Add self-tests for multiple function
trace users").

Second, ftrace_init_array_ops() is called only when cnt > 1 but
but ftrace_reset_array_ops() is always called. It probably does
not harm but it is strange and might eventually hide a bug.
This was added by the commit 4104d326b670c2 ("ftrace: Remove global
function list and call function directly").

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 kernel/trace/trace_selftest.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/trace/trace_selftest.c b/kernel/trace/trace_selftest.c
index b0f86ea77881..0b5ae79b5b0c 100644
--- a/kernel/trace/trace_selftest.c
+++ b/kernel/trace/trace_selftest.c
@@ -272,7 +272,7 @@ static int trace_selftest_ops(struct trace_array *tr, int cnt)
 		goto out_free;
 	if (cnt > 1) {
 		if (trace_selftest_test_global_cnt == 0)
-			goto out;
+			goto out_free;
 	}
 	if (trace_selftest_test_dyn_cnt == 0)
 		goto out_free;
@@ -298,9 +298,10 @@ static int trace_selftest_ops(struct trace_array *tr, int cnt)
 	unregister_ftrace_function(&test_probe1);
 	unregister_ftrace_function(&test_probe2);
 	unregister_ftrace_function(&test_probe3);
-	if (cnt > 1)
+	if (cnt > 1) {
 		unregister_ftrace_function(tr->ops);
-	ftrace_reset_array_ops(tr);
+		ftrace_reset_array_ops(tr);
+	}
 
 	/* Make sure everything is off */
 	reset_counts();
-- 
1.8.5.6

Re: [PATCH 2/4] ftrace/x86: Do not crash when reading wrong ftrace func

[Steven Rostedt]

On Mon, 27 Jun 2016 15:54:35 +0200
Petr Mladek <pmladek@suse.com> wrote:

> Ftrace modifies the code on many locations. It is paranoid
> and avoid a kernel crash using probe_kernel_read() and
> probe_kernel_write(). The only exception is update_ftrace_func()
> where where we read the old code using memcpy().
> 
> It is true that this function is used only to modify well
> defined functions that are part of the ftrace API. But
> it might still make sense to be paranoid and be consistent
> with the writing side.

I'm not so sure I'm too hot on this patch. I left it with the memcpy()
because it was a well known location. If this is wrong, then we should
crash the kernel.

I'm not totally against it. But a comment should be added stating
something like:

	/*
	 * ip points to the ftrace infrastructure. If this fails,
	 * then something is totally messed up.
	 */

and perhaps even add a WARN_ON() here too.

-- Steve

> 
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
>  arch/x86/kernel/ftrace.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 42ea69d35dfd..8305c6792ad2 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -233,7 +233,8 @@ static int update_ftrace_func(unsigned long ip, void *new)
>  	unsigned char old[MCOUNT_INSN_SIZE];
>  	int ret;
>  
> -	memcpy(old, (void *)ip, MCOUNT_INSN_SIZE);
> +	if (probe_kernel_read(old, (void *)ip, MCOUNT_INSN_SIZE))
> +		return -EFAULT;
>  
>  	/*
>  	 * Make sure that we replace 5-byte instruction that

Re: [PATCH 0/4] ftrace: One more check on x86 and some small fixes

[Namhyung Kim]

On Mon, Jun 27, 2016 at 10:54 PM, Petr Mladek <pmladek@suse.com> wrote:
> 1st patch adds one more paranoid check of the modified function on x86.
>
> Plus there are 3 small changes that appeared when hunting down
> the 1st patch.

For the series,

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


>
> Petr Mladek (4):
>   ftrace/x86: Make sure to modify 5-bite instructions
>   ftrace/x86: Do not crash when reading wrong ftrace func
>   ftrace: Always destroy trampoline when shutting down the trace
>   ftrace: Fixup trace_selftest_ops()
>
>  arch/x86/kernel/ftrace.c      | 12 ++++++++-
>  kernel/trace/ftrace.c         | 62 ++++++++++++++++++++++---------------------
>  kernel/trace/trace_selftest.c |  7 ++---
>  3 files changed, 47 insertions(+), 34 deletions(-)
>
> --
> 1.8.5.6
>

Re: [PATCH 2/4] ftrace/x86: Do not crash when reading wrong ftrace func

[Petr Mladek]

On Mon 2016-06-27 10:48:58, Steven Rostedt wrote:
> On Mon, 27 Jun 2016 15:54:35 +0200
> Petr Mladek <pmladek@suse.com> wrote:
> 
> > Ftrace modifies the code on many locations. It is paranoid
> > and avoid a kernel crash using probe_kernel_read() and
> > probe_kernel_write(). The only exception is update_ftrace_func()
> > where where we read the old code using memcpy().
> > 
> > It is true that this function is used only to modify well
> > defined functions that are part of the ftrace API. But
> > it might still make sense to be paranoid and be consistent
> > with the writing side.
> 
> I'm not so sure I'm too hot on this patch. I left it with the memcpy()
> because it was a well known location. If this is wrong, then we should
> crash the kernel.

I had a suspicion that you had had a reason for using memcpy(). This
is why I put it into a separate patch ;-)

Well, I still would feel better if the kernel survive. An error here
means that there is a bug in the core ftrace infrastructure but
it does not need to be fatal for the running system.


> I'm not totally against it. But a comment should be added stating
> something like:
> 
> 	/*
> 	 * ip points to the ftrace infrastructure. If this fails,
> 	 * then something is totally messed up.
> 	 */
> 
> and perhaps even add a WARN_ON() here too.

Yup, WARN_ON() seems appropriate. I like it. Please,
find the updated patch below.


>From 168e3edcf44f0e9a31284b40818cfa693fee78a2 Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.com>
Date: Wed, 22 Jun 2016 15:49:06 +0200
Subject: [PATCH 2/4] ftrace/x86: Do not crash when reading wrong ftrace func

Ftrace modifies the code on many locations. It is paranoid
and avoid a kernel crash using probe_kernel_read() and
probe_kernel_write(). The only exception is update_ftrace_func()
where where we read the old code using memcpy().

It is true that this function is used only to modify well
defined functions that are part of the ftrace API. But
it might still make sense to be paranoid and be consistent
with the writing side.

Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 arch/x86/kernel/ftrace.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 42ea69d35dfd..951c4bd639c4 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -233,7 +233,13 @@ static int update_ftrace_func(unsigned long ip, void *new)
 	unsigned char old[MCOUNT_INSN_SIZE];
 	int ret;
 
-	memcpy(old, (void *)ip, MCOUNT_INSN_SIZE);
+	/*
+	 * ip points to the ftrace infrastructure. If this fails,
+	 * then something is totally messed up.
+	 */
+	ret = probe_kernel_read(old, (void *)ip, MCOUNT_INSN_SIZE);
+	if (WARN_ON(ret))
+		return -EFAULT;
 
 	/*
 	 * Make sure that we replace 5-byte instruction that
-- 
1.8.5.6

Re: [PATCH 0/4] ftrace: One more check on x86 and some small fixes

[Steven Rostedt]

On Mon, 27 Jun 2016 15:54:33 +0200
Petr Mladek <pmladek@suse.com> wrote:

> 1st patch adds one more paranoid check of the modified function on x86.
> 
> Plus there are 3 small changes that appeared when hunting down
> the 1st patch.
> 
> Petr Mladek (4):
>   ftrace/x86: Make sure to modify 5-bite instructions
>   ftrace/x86: Do not crash when reading wrong ftrace func

The first two are mostly just "detect error, when it happens again"
chanes.

>   ftrace: Always destroy trampoline when shutting down the trace
>   ftrace: Fixup trace_selftest_ops()

These two seem to fix real bugs. I'm thinking they should go in now and
be backported via stable.

Thoughts?

-- Steve

> 
>  arch/x86/kernel/ftrace.c      | 12 ++++++++-
>  kernel/trace/ftrace.c         | 62 ++++++++++++++++++++++---------------------
>  kernel/trace/trace_selftest.c |  7 ++---
>  3 files changed, 47 insertions(+), 34 deletions(-)
> 

Re: [PATCH 0/4] ftrace: One more check on x86 and some small fixes

[Petr Mladek]

On Tue 2016-06-28 15:17:56, Steven Rostedt wrote:
> On Mon, 27 Jun 2016 15:54:33 +0200
> Petr Mladek <pmladek@suse.com> wrote:
> 
> > 1st patch adds one more paranoid check of the modified function on x86.
> > 
> > Plus there are 3 small changes that appeared when hunting down
> > the 1st patch.
> > 
> > Petr Mladek (4):
> >   ftrace/x86: Make sure to modify 5-bite instructions
> >   ftrace/x86: Do not crash when reading wrong ftrace func
> 
> The first two are mostly just "detect error, when it happens again"
> chanes.

Yup.

> >   ftrace: Always destroy trampoline when shutting down the trace
> >   ftrace: Fixup trace_selftest_ops()
> 
> These two seem to fix real bugs. I'm thinking they should go in now and
> be backported via stable.
> 
> Thoughts?

Makes sense. Should I do anything, please?

Best Regards,
Petr