Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1649635 > unrolled thread

[PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

Started byThomas Gleixner <tglx@linutronix.de>
First post2017-05-24 15:50 +0200
Last post2017-05-26 03:40 +0200
Articles 6 on this page of 26 — 7 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-24 15:50 +0200
    Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not  RWX Masami Hiramatsu <mhiramat@kernel.org> - 2017-05-24 16:40 +0200
    Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not  RWX Steven Rostedt <rostedt@goodmis.org> - 2017-05-24 17:10 +0200
    Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-24 20:20 +0200
      Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not  RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-24 21:00 +0200
        Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-24 21:40 +0200
    Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not  RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-24 21:20 +0200
      Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not  RWX Steven Rostedt <rostedt@goodmis.org> - 2017-05-25 00:30 +0200
        Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-25 01:20 +0200
        Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not  RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-25 08:30 +0200
          [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not  RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-25 11:00 +0200
            Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are  not RWX Steven Rostedt <rostedt@goodmis.org> - 2017-05-25 17:20 +0200
            Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not  RWX "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-05-25 19:50 +0200
              Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX Kees Cook <keescook@chromium.org> - 2017-05-25 22:00 +0200
                Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are  not RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-26 09:10 +0200
                  Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not  RWX Heiko Carstens <heiko.carstens@de.ibm.com> - 2017-05-26 11:40 +0200
                    Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are  not RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-26 12:00 +0200
                    Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX Michael Ellerman <mpe@ellerman.id.au> - 2017-05-26 13:50 +0200
                Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are  not RWX Masami Hiramatsu <mhiramat@kernel.org> - 2017-05-26 11:50 +0200
            Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are  not RWX Steven Rostedt <rostedt@goodmis.org> - 2017-05-26 15:40 +0200
              Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are  not RWX Steven Rostedt <rostedt@goodmis.org> - 2017-05-26 16:00 +0200
              Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are  not RWX Thomas Gleixner <tglx@linutronix.de> - 2017-05-26 16:00 +0200
        Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not  RWX Masami Hiramatsu <mhiramat@kernel.org> - 2017-05-25 11:10 +0200
          Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not  RWX Masami Hiramatsu <mhiramat@kernel.org> - 2017-05-25 12:40 +0200
            Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not  RWX Steven Rostedt <rostedt@goodmis.org> - 2017-05-25 17:20 +0200
              Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not  RWX Masami Hiramatsu <mhiramat@kernel.org> - 2017-05-26 03:40 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1651399 — Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-26 16:00 +0200
SubjectRe: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tLnWx-6BD-1@gated-at.bofh.it>
In reply to#1651395
On Fri, 26 May 2017 15:50:38 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:


> That's very early boot, right? So interrupts have to be disabled.
> 
> So this wants to be:
> 
>    BUG_ON(irqs_disabled() && !early_boot_irqs_disabled);
> 

I was thinking the same thing. I'll add a patch and retest.

Thanks!

-- Steve

[toc] | [prev] | [next] | [standalone]

#1651402 — Re: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX

FromThomas Gleixner <tglx@linutronix.de>
Date2017-05-26 16:00 +0200
SubjectRe: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tLnWx-6BD-3@gated-at.bofh.it>
In reply to#1651395
On Fri, 26 May 2017, Steven Rostedt wrote:

> On Thu, 25 May 2017 10:57:51 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > ftrace use module_alloc() to allocate trampoline pages. The mapping of
> > module_alloc() is RWX, which makes sense as the memory is written to right
> > after allocation. But nothing makes these pages RO after writing to them.
> > 
> > Add proper set_memory_rw/ro() calls to protect the trampolines after
> > modification.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > ---
> 
> Unfortunately, this was triggered in my tests:
> 
> ftrace: allocating 54840 entries in 215 pages
> Starting tracer 'function'
> ------------[ cut here ]------------
> kernel BUG at /work/autotest/nobackup/linux-test.git/arch/x86/mm/pageattr.c:189!
>
> It appears that if you enable function tracing at boot up, calling
> set_memory_ro() with interrupts disabled can cause this. As pageattr.c
> at line 189 has:
> 
> 	BUG_ON(irqs_disabled());

That's very early boot, right? So interrupts have to be disabled.

So this wants to be:

   BUG_ON(irqs_disabled() && !early_boot_irqs_disabled);

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1650338 — Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

FromMasami Hiramatsu <mhiramat@kernel.org>
Date2017-05-25 11:10 +0200
SubjectRe: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tKWWl-6hJ-11@gated-at.bofh.it>
In reply to#1650000
On Wed, 24 May 2017 18:25:47 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Wed, 24 May 2017 21:13:27 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> 
> > > Oops: 0003 [#1] SMP
> > > Modules linked in:
> > > CPU: 3 PID: 1 Comm: swapper/0 Not tainted 4.12.0-rc2-test+ #42
> > > Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6
> > > 02/22/2014 task: ffff8802153a8000 task.stack: ffffc90000c74000
> > > RIP: 0010:new_slab+0x1e8/0x2b4
> > > RSP: 0000:ffffc90000c77b28 EFLAGS: 00010282
> > > RAX: 0000000040040000 RBX: ffff880216003f00 RCX: ffff880214f5c058
> > > RDX: 0000000000000000 RSI: ffff880214f5c000 RDI: ffff880216003f00
> > > RBP: ffffc90000c77b70 R08: 000000000000002a R09: 0000000000000000
> > > R10: 00000000000201e2 R11: 0000000000020190 R12: ffff880214f5c000
> > > R13: 000000000000002e R14: 0000000000000001 R15: ffffea000853d700
> > > FS:  0000000000000000(0000) GS:ffff88021eb80000(0000)
> > > knlGS:0000000000000000 CS:  0010 DS: 0000 ES: 0000 CR0:
> > > 0000000080050033 CR2: ffff880214f5c000 CR3: 000000000221d000 CR4:
> > > 00000000001406e0 Call Trace:
> > >  ? interleave_nodes+0x29/0x40
> > >  ___slab_alloc+0x2e8/0x49e  
> > 
> > That does not make any sense, but I'm digging into it.
> 
> The trampolines uses the module allocation, and it appears, that needs
> to become rw before freeing again.

Oops, I also have to fix kprobes side, because it has same issue.

Thanks!

> 
> I applied this patch, and it appears to fix the bug for me.
> 
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> 
> -- Steve
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 663a35d..5e93a9a 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -689,8 +689,12 @@ static inline void *alloc_tramp(unsigned long size)
>  {
>  	return module_alloc(size);
>  }
> -static inline void tramp_free(void *tramp)
> +static inline void tramp_free(void *tramp, int size)
>  {
> +	int npages;
> +
> +	npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> +	set_memory_rw((unsigned long)tramp, npages);
>  	module_memfree(tramp);
>  }
>  #else
> @@ -699,7 +703,7 @@ static inline void *alloc_tramp(unsigned long size)
>  {
>  	return NULL;
>  }
> -static inline void tramp_free(void *tramp) { }
> +static inline void tramp_free(void *tramp, int size) { }
>  #endif
>  
>  /* Defined as markers to the end of the ftrace default trampolines */
> @@ -771,7 +775,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
>  	/* Copy ftrace_caller onto the trampoline memory */
>  	ret = probe_kernel_read(trampoline, (void *)start_offset, size);
>  	if (WARN_ON(ret < 0)) {
> -		tramp_free(trampoline);
> +		tramp_free(trampoline, *tramp_size);
>  		return 0;
>  	}
>  
> @@ -797,7 +801,7 @@ create_trampoline(struct ftrace_ops *ops, unsigned int *tramp_size)
>  
>  	/* Are we pointing to the reference? */
>  	if (WARN_ON(memcmp(op_ptr.op, op_ref, 3) != 0)) {
> -		tramp_free(trampoline);
> +		tramp_free(trampoline, *tramp_size);
>  		return 0;
>  	}
>  
> @@ -943,7 +947,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
>  	if (!ops || !(ops->flags & FTRACE_OPS_FL_ALLOC_TRAMP))
>  		return;
>  
> -	tramp_free((void *)ops->trampoline);
> +	tramp_free((void *)ops->trampoline, ops->trampoline_size);
>  	ops->trampoline = 0;
>  }
>  


-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1650364 — Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

FromMasami Hiramatsu <mhiramat@kernel.org>
Date2017-05-25 12:40 +0200
SubjectRe: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tKYlr-70D-9@gated-at.bofh.it>
In reply to#1650338
On Thu, 25 May 2017 18:09:10 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:

> On Wed, 24 May 2017 18:25:47 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Wed, 24 May 2017 21:13:27 +0200 (CEST)
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > 
> > > > Oops: 0003 [#1] SMP
> > > > Modules linked in:
> > > > CPU: 3 PID: 1 Comm: swapper/0 Not tainted 4.12.0-rc2-test+ #42
> > > > Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6
> > > > 02/22/2014 task: ffff8802153a8000 task.stack: ffffc90000c74000
> > > > RIP: 0010:new_slab+0x1e8/0x2b4
> > > > RSP: 0000:ffffc90000c77b28 EFLAGS: 00010282
> > > > RAX: 0000000040040000 RBX: ffff880216003f00 RCX: ffff880214f5c058
> > > > RDX: 0000000000000000 RSI: ffff880214f5c000 RDI: ffff880216003f00
> > > > RBP: ffffc90000c77b70 R08: 000000000000002a R09: 0000000000000000
> > > > R10: 00000000000201e2 R11: 0000000000020190 R12: ffff880214f5c000
> > > > R13: 000000000000002e R14: 0000000000000001 R15: ffffea000853d700
> > > > FS:  0000000000000000(0000) GS:ffff88021eb80000(0000)
> > > > knlGS:0000000000000000 CS:  0010 DS: 0000 ES: 0000 CR0:
> > > > 0000000080050033 CR2: ffff880214f5c000 CR3: 000000000221d000 CR4:
> > > > 00000000001406e0 Call Trace:
> > > >  ? interleave_nodes+0x29/0x40
> > > >  ___slab_alloc+0x2e8/0x49e  
> > > 
> > > That does not make any sense, but I'm digging into it.
> > 
> > The trampolines uses the module allocation, and it appears, that needs
> > to become rw before freeing again.
> 
> Oops, I also have to fix kprobes side, because it has same issue.

OK, I've ensured that following command hits same BUG.

grep t /proc/kallsyms  | cut -f3 -d" " | grep -v .*\\..* | \
head -n 256 | while read i; do echo p $i+5 ; done > tracing/kprobe_events

echo 1 > tracing/events/kprobes/enable 
echo 0 > tracing/events/kprobes/enable
echo > tracing/kprobe_events
sleep 5

I'll send a bugfix asap.

Thank,

-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [next] | [standalone]

#1650580 — Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-25 17:20 +0200
SubjectRe: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tL2Iq-1qK-27@gated-at.bofh.it>
In reply to#1650364
On Thu, 25 May 2017 19:34:43 +0900
Masami Hiramatsu <mhiramat@kernel.org> wrote:


> OK, I've ensured that following command hits same BUG.
> 
> grep t /proc/kallsyms  | cut -f3 -d" " | grep -v .*\\..* | \
> head -n 256 | while read i; do echo p $i+5 ; done >
> tracing/kprobe_events
> 
> echo 1 > tracing/events/kprobes/enable 
> echo 0 > tracing/events/kprobes/enable
> echo > tracing/kprobe_events
> sleep 5

Hi Masami,

Can you add a selftest that checks this too?

Thanks!

-- Steve

> 
> I'll send a bugfix asap.
> 
> Thank,
>

[toc] | [prev] | [next] | [standalone]

#1651022 — Re: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX

FromMasami Hiramatsu <mhiramat@kernel.org>
Date2017-05-26 03:40 +0200
SubjectRe: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tLcop-7Hu-5@gated-at.bofh.it>
In reply to#1650580
On Thu, 25 May 2017 11:18:02 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 25 May 2017 19:34:43 +0900
> Masami Hiramatsu <mhiramat@kernel.org> wrote:
> 
> 
> > OK, I've ensured that following command hits same BUG.
> > 
> > grep t /proc/kallsyms  | cut -f3 -d" " | grep -v .*\\..* | \
> > head -n 256 | while read i; do echo p $i+5 ; done >
> > tracing/kprobe_events
> > 
> > echo 1 > tracing/events/kprobes/enable 
> > echo 0 > tracing/events/kprobes/enable
> > echo > tracing/kprobe_events
> > sleep 5
> 
> Hi Masami,
> 
> Can you add a selftest that checks this too?

Yes, however, it should be included after my fix is
accepted, or it can cause a kernel panic...

Thanks,



-- 
Masami Hiramatsu <mhiramat@kernel.org>

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web