Re: [RFC][PATCH] x86/ftrace: Have ftrace trampolines turn read-only at the end of system boot up
On Fri, May 01, 2020 at 12:19:16PM -0400, Steven Rostedt wrote: > > Peter, what about you? It's all a bit unfortunate, but yeah, seems reasonable. Ack.
Re: [RFC][PATCH] x86/ftrace: Have ftrace trampolines turn read-only at the end of system boot up
On Fri, 1 May 2020 10:13:10 -0500 Josh Poimboeuf wrote: > On Fri, May 01, 2020 at 09:24:04AM -0400, Steven Rostedt wrote: > > On Fri, 1 May 2020 00:17:06 -0500 > > Josh Poimboeuf wrote: > > > > > > Would it be easier to just call a new __text_poke_bp() which skips the > > > > SYSTEM_BOOTING check, since you know the trampoline will always be > > > > read-only? > > > > > > > > Like: > > > > > > early_trace_init() is called after mm_init(), so I thought it might > > > work, but I guess not: > > > > Yeah, I was about to say that this happens before mm_init() ;-) > > It happens *after* mm_init(). But now text_poke() has a dependency on > poking_init(), has a dependency on proc_caches_init(), which has a > dependency on kmem_cache_init_late(), etc. > > So how early do you need early_trace_init()? I'm assuming moving it to > after kmem_cache_init_late() would be too late. People have asked to move it even earlier. The point of having it early is to allow tracing to debug early boot up. > > > It's why we already have magic for enabling function tracing the first time. > > > > Do you see anything wrong with this current solution? It probably needs > > more comments, but I wanted to get acceptance on the logic before I go and > > pretty it up and send a non RFC patch. > > Assuming we can't get text_poke() working earlier, it seems reasonable > to me. > Thanks. Peter, what about you? -- Steve
Re: [RFC][PATCH] x86/ftrace: Have ftrace trampolines turn read-only at the end of system boot up
On Fri, May 01, 2020 at 09:24:04AM -0400, Steven Rostedt wrote: > On Fri, 1 May 2020 00:17:06 -0500 > Josh Poimboeuf wrote: > > > > Would it be easier to just call a new __text_poke_bp() which skips the > > > SYSTEM_BOOTING check, since you know the trampoline will always be > > > read-only? > > > > > > Like: > > > > early_trace_init() is called after mm_init(), so I thought it might > > work, but I guess not: > > Yeah, I was about to say that this happens before mm_init() ;-) It happens *after* mm_init(). But now text_poke() has a dependency on poking_init(), has a dependency on proc_caches_init(), which has a dependency on kmem_cache_init_late(), etc. So how early do you need early_trace_init()? I'm assuming moving it to after kmem_cache_init_late() would be too late. > It's why we already have magic for enabling function tracing the first time. > > Do you see anything wrong with this current solution? It probably needs > more comments, but I wanted to get acceptance on the logic before I go and > pretty it up and send a non RFC patch. Assuming we can't get text_poke() working earlier, it seems reasonable to me. -- Josh
Re: [RFC][PATCH] x86/ftrace: Have ftrace trampolines turn read-only at the end of system boot up
On Fri, 1 May 2020 00:17:06 -0500 Josh Poimboeuf wrote: > > Would it be easier to just call a new __text_poke_bp() which skips the > > SYSTEM_BOOTING check, since you know the trampoline will always be > > read-only? > > > > Like: > > early_trace_init() is called after mm_init(), so I thought it might > work, but I guess not: Yeah, I was about to say that this happens before mm_init() ;-) It's why we already have magic for enabling function tracing the first time. Do you see anything wrong with this current solution? It probably needs more comments, but I wanted to get acceptance on the logic before I go and pretty it up and send a non RFC patch. -- Steve
Re: [RFC][PATCH] x86/ftrace: Have ftrace trampolines turn read-only at the end of system boot up
On Thu, Apr 30, 2020 at 11:47:33PM -0500, Josh Poimboeuf wrote: > On Thu, Apr 30, 2020 at 08:21:47PM -0400, Steven Rostedt wrote: > > The cause is the "ftrace=function" would register the function tracer > > and create a trampoline, and it will set it as executable and > > read-only. Then the "trace_options=func_stack_trace" would then update > > the same trampoline to include the stack tracer version of the function > > tracer. But since the trampoline already exists, it updates it with > > text_poke_bp(). The problem is that text_poke_bp() called while > > system_state == SYSTEM_BOOTING, it will simply do a memcpy() and not > > the page mapping, as it would think that the text is still read-write. > > But in this case it is not, and we take a fault and crash. > > > > Instead, lets keep the ftrace trampolines read-write during boot up, > > and then when the kernel executable text is set to read-only, the > > ftrace trampolines get set to read-only as well. > > Would it be easier to just call a new __text_poke_bp() which skips the > SYSTEM_BOOTING check, since you know the trampoline will always be > read-only? > > Like: early_trace_init() is called after mm_init(), so I thought it might work, but I guess not: [0.206271] Starting tracer 'function' [0.232032] BUG: kernel NULL pointer dereference, address: 0050 [0.232035] #PF: supervisor read access in kernel mode [0.232036] #PF: error_code(0x) - not-present page [0.232037] PGD 0 P4D 0 [0.232040] Oops: [#1] PREEMPT SMP PTI [0.232042] CPU: 0 PID: 0 Comm: swapper Not tainted 5.7.0-rc2-next-20200424+ #127 [0.232043] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.12.0-2.fc30 04/01/2014 [0.232047] RIP: 0010:walk_to_pmd+0x11/0x140 [0.232048] Code: 78 c1 32 82 e8 a0 68 ff ff 0f 0b 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 0f 1f 44 00 00 48 89 f0 48 c1 e8 24 25 f8 0f 00 00 <48> 03 47 50 0f 84 12 01 00 00 41 54 49 89 fc 55 48 89 c5 53 48 8b [0.232050] RSP: :82603c68 EFLAGS: 00010046 [0.232051] RAX: RBX: RCX: [0.232052] RDX: 82603cc8 RSI: RDI: [0.232053] RBP: 8063 R08: 0001 R09: fff00fff [0.232054] R10: R11: R12: 82603cc8 [0.232055] R13: R14: 0066 R15: feff [0.232056] FS: () GS:88813b60() knlGS: [0.232057] CS: 0010 DS: ES: CR0: 80050033 [0.232058] CR2: 0050 CR3: 02612001 CR4: 000606b0 [0.232061] DR0: DR1: DR2: [0.232062] DR3: DR6: fffe0ff0 DR7: 0400 [0.232063] Call Trace: [0.232066] __get_locked_pte+0x19/0x100 [0.232069] ? 0xa065 [0.232071] __text_poke+0x14d/0x640 [0.232073] ? 0xa065 [0.232076] text_poke_bp_batch+0x8b/0x1c0 [0.232078] ? 0xa065 [0.232080] __text_poke_bp+0x3a/0x60 [0.232083] arch_ftrace_update_trampoline+0xac/0x300 [0.232087] __register_ftrace_function+0x7c/0xc0 [0.232089] ftrace_startup+0x1e/0xf0 [0.232091] register_ftrace_function+0x24/0x70 [0.232093] func_set_flag+0x6f/0x90 [0.232096] __set_tracer_option.isra.0+0x24/0x50 [0.232098] trace_set_options+0x149/0x160 [0.232102] apply_trace_boot_options+0x44/0x6d [0.232105] register_tracer+0x1d8/0x1e9 [0.232107] early_trace_init+0x266/0x378 [0.232109] start_kernel+0x337/0x614 [0.232112] ? x86_family+0x5/0x20 [0.232114] secondary_startup_64+0xa4/0xb0 [0.232119] Modules linked in: [0.232120] CR2: 0050 [0.232126] random: get_random_bytes called from print_oops_end_marker+0x26/0x40 with crng_init=0 [0.232128] ---[ end trace 71e23a89b9b5224f ]--- [0.232130] RIP: 0010:walk_to_pmd+0x11/0x140 [0.232131] Code: 78 c1 32 82 e8 a0 68 ff ff 0f 0b 66 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 00 0f 1f 44 00 00 48 89 f0 48 c1 e8 24 25 f8 0f 00 00 <48> 03 47 50 0f 84 12 01 00 00 41 54 49 89 fc 55 48 89 c5 53 48 8b [0.232133] RSP: :82603c68 EFLAGS: 00010046 [0.232134] RAX: RBX: RCX: [0.232135] RDX: 82603cc8 RSI: RDI: [0.232136] RBP: 8063 R08: 0001 R09: fff00fff [0.232137] R10: R11: R12: 82603cc8 [0.232138] R13: R14: 0066 R15: feff [0.232139] FS: () GS:88813b60() knlGS: [0.232140] CS: 0010 DS: ES: CR0: 80050033 [0.232141] CR2: 0050 CR3: 02612001 CR4: 000606b0 [0.232142] DR0:
Re: [RFC][PATCH] x86/ftrace: Have ftrace trampolines turn read-only at the end of system boot up
On Thu, Apr 30, 2020 at 08:21:47PM -0400, Steven Rostedt wrote:
> The cause is the "ftrace=function" would register the function tracer
> and create a trampoline, and it will set it as executable and
> read-only. Then the "trace_options=func_stack_trace" would then update
> the same trampoline to include the stack tracer version of the function
> tracer. But since the trampoline already exists, it updates it with
> text_poke_bp(). The problem is that text_poke_bp() called while
> system_state == SYSTEM_BOOTING, it will simply do a memcpy() and not
> the page mapping, as it would think that the text is still read-write.
> But in this case it is not, and we take a fault and crash.
>
> Instead, lets keep the ftrace trampolines read-write during boot up,
> and then when the kernel executable text is set to read-only, the
> ftrace trampolines get set to read-only as well.
Would it be easier to just call a new __text_poke_bp() which skips the
SYSTEM_BOOTING check, since you know the trampoline will always be
read-only?
Like:
diff --git a/arch/x86/include/asm/text-patching.h
b/arch/x86/include/asm/text-patching.h
index 67315fa3956a..710106256916 100644
--- a/arch/x86/include/asm/text-patching.h
+++ b/arch/x86/include/asm/text-patching.h
@@ -45,6 +45,7 @@ extern void *text_poke(void *addr, const void *opcode, size_t
len);
extern void text_poke_sync(void);
extern void *text_poke_kgdb(void *addr, const void *opcode, size_t len);
extern int poke_int3_handler(struct pt_regs *regs);
+extern void __text_poke_bp(void *addr, const void *opcode, size_t len, const
void *emulate);
extern void text_poke_bp(void *addr, const void *opcode, size_t len, const
void *emulate);
extern void text_poke_queue(void *addr, const void *opcode, size_t len, const
void *emulate);
diff --git a/arch/x86/kernel/alternative.c b/arch/x86/kernel/alternative.c
index 7867dfb3963e..9cc983cc9291 100644
--- a/arch/x86/kernel/alternative.c
+++ b/arch/x86/kernel/alternative.c
@@ -1265,6 +1265,14 @@ void __ref text_poke_queue(void *addr, const void
*opcode, size_t len, const voi
text_poke_loc_init(tp, addr, opcode, len, emulate);
}
+void __ref __text_poke_bp(void *addr, const void *opcode, size_t len, const
void *emulate)
+{
+ struct text_poke_loc tp;
+
+ text_poke_loc_init(, addr, opcode, len, emulate);
+ text_poke_bp_batch(, 1);
+}
+
/**
* text_poke_bp() -- update instructions on live kernel on SMP
* @addr: address to patch
@@ -1278,13 +1286,10 @@ void __ref text_poke_queue(void *addr, const void
*opcode, size_t len, const voi
*/
void __ref text_poke_bp(void *addr, const void *opcode, size_t len, const void
*emulate)
{
- struct text_poke_loc tp;
-
if (unlikely(system_state == SYSTEM_BOOTING)) {
text_poke_early(addr, opcode, len);
return;
}
- text_poke_loc_init(, addr, opcode, len, emulate);
- text_poke_bp_batch(, 1);
+ __text_poke_bp(addr, opcode, len, emulate);
}
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 867c126ddabe..c36f51f01f6e 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -469,7 +469,7 @@ void arch_ftrace_update_trampoline(struct ftrace_ops *ops)
mutex_lock(_mutex);
/* Do a safe modify in case the trampoline is executing */
new = ftrace_call_replace(ip, (unsigned long)func);
- text_poke_bp((void *)ip, new, MCOUNT_INSN_SIZE, NULL);
+ __text_poke_bp((void *)ip, new, MCOUNT_INSN_SIZE, NULL);
mutex_unlock(_mutex);
}
[RFC][PATCH] x86/ftrace: Have ftrace trampolines turn read-only at the end of system boot up
From: Steven Rostedt (VMware)
Booting one of my machines, it triggered the following crash:
Kernel/User page tables isolation: enabled
ftrace: allocating 36577 entries in 143 pages
Starting tracer 'function'
BUG: unable to handle page fault for address: a05c
#PF: supervisor write access in kernel mode
#PF: error_code(0x0003) - permissions violation
PGD 2014067 P4D 2014067 PUD 2015063 PMD 7b253067 PTE 7b252061
Oops: 0003 [#1] PREEMPT SMP PTI
CPU: 0 PID: 0 Comm: swapper Not tainted 5.4.0-test+ #24
Hardware name: To Be Filled By O.E.M. To Be Filled By O.E.M./To be filled by
O.E.M., BIOS SDBLI944.86P 05/08/2007
RIP: 0010:text_poke_early+0x4a/0x58
Code: 34 24 48 89 54 24 08 e8 bf 72 0b 00 48 8b 34 24 48 8b 4c 24 08 84 c0 74
0b 48 89 df f3 a4 48 83 c4 10 5b c3 9c 58 fa 48 89 df a4 50 9d 48 83 c4 10
5b e9 d6 f9 ff ff
0 41 57 49
RSP: :82003d38 EFLAGS: 00010046
RAX: 0046 RBX: a05c RCX: 0005
RDX: 0005 RSI: 825b9a90 RDI: a05c
RBP: a05c R08: R09: 8206e6e0
R10: 88807b01f4c0 R11: 8176c106 R12: 8206e6e0
R13: 824f2440 R14: R15: 8206eac0
FS: () GS:88807d40() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: a05c CR3: 02012000 CR4: 06b0
Call Trace:
text_poke_bp+0x27/0x64
? mutex_lock+0x36/0x5d
arch_ftrace_update_trampoline+0x287/0x2d5
? ftrace_replace_code+0x14b/0x160
? ftrace_update_ftrace_func+0x65/0x6c
__register_ftrace_function+0x6d/0x81
ftrace_startup+0x23/0xc1
register_ftrace_function+0x20/0x37
func_set_flag+0x59/0x77
__set_tracer_option.isra.19+0x20/0x3e
trace_set_options+0xd6/0x13e
apply_trace_boot_options+0x44/0x6d
register_tracer+0x19e/0x1ac
early_trace_init+0x21b/0x2c9
start_kernel+0x241/0x518
? load_ucode_intel_bsp+0x21/0x52
secondary_startup_64+0xa4/0xb0
I was able to trigger it on other machines, when I added to the kernel
command line of both "ftrace=function" and "trace_options=func_stack_trace".
The cause is the "ftrace=function" would register the function tracer
and create a trampoline, and it will set it as executable and
read-only. Then the "trace_options=func_stack_trace" would then update
the same trampoline to include the stack tracer version of the function
tracer. But since the trampoline already exists, it updates it with
text_poke_bp(). The problem is that text_poke_bp() called while
system_state == SYSTEM_BOOTING, it will simply do a memcpy() and not
the page mapping, as it would think that the text is still read-write.
But in this case it is not, and we take a fault and crash.
Instead, lets keep the ftrace trampolines read-write during boot up,
and then when the kernel executable text is set to read-only, the
ftrace trampolines get set to read-only as well.
Cc: sta...@vger.kernel.org
Fixes: 768ae4406a5c ("x86/ftrace: Use text_poke()")
Signed-off-by: Steven Rostedt (VMware)
---
diff --git a/arch/x86/include/asm/ftrace.h b/arch/x86/include/asm/ftrace.h
index dbd9b08bf173..e14f792b106c 100644
--- a/arch/x86/include/asm/ftrace.h
+++ b/arch/x86/include/asm/ftrace.h
@@ -43,6 +43,12 @@ struct dyn_arch_ftrace {
#ifndef __ASSEMBLY__
+#ifdef CONFIG_FUNCTION_TRACER
+extern void set_ftrace_ops_ro(void);
+#else
+static inline void set_ftrace_ops_ro(void) { }
+#endif
+
#define ARCH_HAS_SYSCALL_MATCH_SYM_NAME
static inline bool arch_syscall_match_sym_name(const char *sym, const char
*name)
{
diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 3d8adeba2651..dd30ba1e0244 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -419,7 +419,8 @@ create_trampoline(struct ftrace_ops *ops, unsigned int
*tramp_size)
set_vm_flush_reset_perms(trampoline);
- set_memory_ro((unsigned long)trampoline, npages);
+ if (likely(system_state != SYSTEM_BOOTING))
+ set_memory_ro((unsigned long)trampoline, npages);
set_memory_x((unsigned long)trampoline, npages);
return (unsigned long)trampoline;
fail:
@@ -427,6 +428,32 @@ create_trampoline(struct ftrace_ops *ops, unsigned int
*tramp_size)
return 0;
}
+void set_ftrace_ops_ro(void)
+{
+ struct ftrace_ops *ops;
+ unsigned long start_offset;
+ unsigned long end_offset;
+ unsigned long npages;
+ unsigned long size;
+
+ do_for_each_ftrace_op(ops, ftrace_ops_list) {
+ if (!(ops->flags & FTRACE_OPS_FL_ALLOC_TRAMP))
+ continue;
+
+ if (ops->flags & FTRACE_OPS_FL_SAVE_REGS) {
+ start_offset = (unsigned long)ftrace_regs_caller;
+ end_offset = (unsigned long)ftrace_regs_caller_end;
+ } else {
+ start_offset = (unsigned long)ftrace_caller;
+ end_offset =
