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


Groups > linux.kernel > #1650211 > unrolled thread

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

Started byThomas Gleixner <tglx@linutronix.de>
First post2017-05-25 08:30 +0200
Last post2017-05-26 16:00 +0200
Articles 13 — 7 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.


Contents

  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

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

FromThomas Gleixner <tglx@linutronix.de>
Date2017-05-25 08:30 +0200
SubjectRe: [PATCH] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tKUrw-4D7-3@gated-at.bofh.it>
On Wed, 24 May 2017, Steven Rostedt wrote:
> The trampolines uses the module allocation, and it appears, that needs
> to become rw before freeing again.

Indeed. I realized that when enabling more debug options, which led to a
reliable triple fault.

How intuitive.

> I applied this patch, and it appears to fix the bug for me.

It fixes the bug, but ...

> -static inline void tramp_free(void *tramp)
> +static inline void tramp_free(void *tramp, int size)
>  {
> +	int npages;
> +
> +	npages = PAGE_ALIGN(size) >> PAGE_SHIFT;

For correctness sake this wants

    	set_memory_nx(...);

as well.

> +	set_memory_rw((unsigned long)tramp, npages);
>  	module_memfree(tramp);
>  }

I'll clean that up and post a V2.

Thanks,

	tglx

[toc] | [next] | [standalone]


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

FromThomas Gleixner <tglx@linutronix.de>
Date2017-05-25 11:00 +0200
Subject[PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tKWMG-5YF-15@gated-at.bofh.it>
In reply to#1650211
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>
---
 arch/x86/kernel/ftrace.c |   20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -689,8 +689,12 @@ static inline void *alloc_tramp(unsigned
 {
 	return module_alloc(size);
 }
-static inline void tramp_free(void *tramp)
+static inline void tramp_free(void *tramp, int size)
 {
+	int npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
+
+	set_memory_nx((unsigned long)tramp, npages);
+	set_memory_rw((unsigned long)tramp, npages);
 	module_memfree(tramp);
 }
 #else
@@ -699,7 +703,7 @@ static inline void *alloc_tramp(unsigned
 {
 	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
 	/* 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
 
 	/* 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;
 	}
 
@@ -839,7 +843,7 @@ void arch_ftrace_update_trampoline(struc
 	unsigned long offset;
 	unsigned long ip;
 	unsigned int size;
-	int ret;
+	int ret, npages;
 
 	if (ops->trampoline) {
 		/*
@@ -848,11 +852,14 @@ void arch_ftrace_update_trampoline(struc
 		 */
 		if (!(ops->flags & FTRACE_OPS_FL_ALLOC_TRAMP))
 			return;
+		npages = PAGE_ALIGN(ops->trampoline_size) >> PAGE_SHIFT;
+		set_memory_rw(ops->trampoline, npages);
 	} else {
 		ops->trampoline = create_trampoline(ops, &size);
 		if (!ops->trampoline)
 			return;
 		ops->trampoline_size = size;
+		npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
 	}
 
 	offset = calc_trampoline_call_offset(ops->flags & FTRACE_OPS_FL_SAVE_REGS);
@@ -863,6 +870,7 @@ void arch_ftrace_update_trampoline(struc
 	/* Do a safe modify in case the trampoline is executing */
 	new = ftrace_call_replace(ip, (unsigned long)func);
 	ret = update_ftrace_func(ip, new);
+	set_memory_ro(ops->trampoline, npages);
 
 	/* The update should never fail */
 	WARN_ON(ret);
@@ -939,7 +947,7 @@ void arch_ftrace_trampoline_free(struct
 	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;
 }
 

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


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

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-25 17:20 +0200
SubjectRe: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tL2Iq-1qK-23@gated-at.bofh.it>
In reply to#1650336
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>

OK, I pulled this in and I'm currently running it through my test
suite. I'm about to board a flight, hopefully it runs smoothly and will
finish by the time I get home.

-- Steve

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


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

From"Luis R. Rodriguez" <mcgrof@kernel.org>
Date2017-05-25 19:50 +0200
SubjectRe: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tL53z-2KJ-7@gated-at.bofh.it>
In reply to#1650336
On Thu, May 25, 2017 at 10:57:51AM +0200, Thomas Gleixner 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>
> ---
>  arch/x86/kernel/ftrace.c |   20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -689,8 +689,12 @@ static inline void *alloc_tramp(unsigned
>  {
>  	return module_alloc(size);
>  }
> -static inline void tramp_free(void *tramp)
> +static inline void tramp_free(void *tramp, int size)
>  {
> +	int npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> +
> +	set_memory_nx((unsigned long)tramp, npages);
> +	set_memory_rw((unsigned long)tramp, npages);
>  	module_memfree(tramp);
>  }

Can/should module_memfree() just do this for users? With Masami's fix that'd
be 2 users already.

   Luis

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


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

FromKees Cook <keescook@chromium.org>
Date2017-05-25 22:00 +0200
SubjectRe: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tL75o-40J-7@gated-at.bofh.it>
In reply to#1650701
On Thu, May 25, 2017 at 10:46 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> On Thu, May 25, 2017 at 10:57:51AM +0200, Thomas Gleixner 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>
>> ---
>>  arch/x86/kernel/ftrace.c |   20 ++++++++++++++------
>>  1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> --- a/arch/x86/kernel/ftrace.c
>> +++ b/arch/x86/kernel/ftrace.c
>> @@ -689,8 +689,12 @@ static inline void *alloc_tramp(unsigned
>>  {
>>       return module_alloc(size);
>>  }
>> -static inline void tramp_free(void *tramp)
>> +static inline void tramp_free(void *tramp, int size)
>>  {
>> +     int npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
>> +
>> +     set_memory_nx((unsigned long)tramp, npages);
>> +     set_memory_rw((unsigned long)tramp, npages);
>>       module_memfree(tramp);
>>  }
>
> Can/should module_memfree() just do this for users? With Masami's fix that'd
> be 2 users already.

It seems like it really should. That would put it in a single place
and avoid this mistake again in the future. Does module_memfree() have
access to the allocation size, or does that need to get plumbed?

-Kees

-- 
Kees Cook
Pixel Security

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


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

FromThomas Gleixner <tglx@linutronix.de>
Date2017-05-26 09:10 +0200
SubjectRe: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tLhxL-2Oy-3@gated-at.bofh.it>
In reply to#1650781
On Thu, 25 May 2017, Kees Cook wrote:
> On Thu, May 25, 2017 at 10:46 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > On Thu, May 25, 2017 at 10:57:51AM +0200, Thomas Gleixner 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>
> >> ---
> >>  arch/x86/kernel/ftrace.c |   20 ++++++++++++++------
> >>  1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> --- a/arch/x86/kernel/ftrace.c
> >> +++ b/arch/x86/kernel/ftrace.c
> >> @@ -689,8 +689,12 @@ static inline void *alloc_tramp(unsigned
> >>  {
> >>       return module_alloc(size);
> >>  }
> >> -static inline void tramp_free(void *tramp)
> >> +static inline void tramp_free(void *tramp, int size)
> >>  {
> >> +     int npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> >> +
> >> +     set_memory_nx((unsigned long)tramp, npages);
> >> +     set_memory_rw((unsigned long)tramp, npages);
> >>       module_memfree(tramp);
> >>  }
> >
> > Can/should module_memfree() just do this for users? With Masami's fix that'd
> > be 2 users already.
> 
> It seems like it really should. That would put it in a single place
> and avoid this mistake again in the future. Does module_memfree() have
> access to the allocation size, or does that need to get plumbed?

No, it doesn't. But the number of instances is pretty limited.

Btw, looking at BPF. It allocates memory via module_alloc() which means
it's RWX. There is nothing in that BPF code which changes the permissions
afterwards ....

Thanks,

	tglx

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


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

FromHeiko Carstens <heiko.carstens@de.ibm.com>
Date2017-05-26 11:40 +0200
SubjectRe: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tLjSW-490-15@gated-at.bofh.it>
In reply to#1651118
On Fri, May 26, 2017 at 09:03:13AM +0200, Thomas Gleixner wrote:
> > It seems like it really should. That would put it in a single place
> > and avoid this mistake again in the future. Does module_memfree() have
> > access to the allocation size, or does that need to get plumbed?
> 
> No, it doesn't. But the number of instances is pretty limited.
> 
> Btw, looking at BPF. It allocates memory via module_alloc() which means
> it's RWX. There is nothing in that BPF code which changes the permissions
> afterwards ....

For BPF you're probably referring to bpf_jit_binary_alloc()? Permissions
are changed with bpf_jit_binary_lock_ro() within each architecure backend.

Well, except for powerpc (cc'ed Michael).

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


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

FromThomas Gleixner <tglx@linutronix.de>
Date2017-05-26 12:00 +0200
SubjectRe: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tLkch-4g5-3@gated-at.bofh.it>
In reply to#1651264
On Fri, 26 May 2017, Heiko Carstens wrote:

> On Fri, May 26, 2017 at 09:03:13AM +0200, Thomas Gleixner wrote:
> > > It seems like it really should. That would put it in a single place
> > > and avoid this mistake again in the future. Does module_memfree() have
> > > access to the allocation size, or does that need to get plumbed?
> > 
> > No, it doesn't. But the number of instances is pretty limited.
> > 
> > Btw, looking at BPF. It allocates memory via module_alloc() which means
> > it's RWX. There is nothing in that BPF code which changes the permissions
> > afterwards ....
> 
> For BPF you're probably referring to bpf_jit_binary_alloc()? Permissions
> are changed with bpf_jit_binary_lock_ro() within each architecure backend.
> 
> Well, except for powerpc (cc'ed Michael).

The problem starts elsewhere. module_alloc() should not allocate RWX memory
in the first place. It should allocated RW and then the usage sites should
set it to RX when the code is ready to go.

Thanks,

	tglx

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


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

FromMichael Ellerman <mpe@ellerman.id.au>
Date2017-05-26 13:50 +0200
SubjectRe: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tLlUJ-5nB-13@gated-at.bofh.it>
In reply to#1651264
Heiko Carstens <heiko.carstens@de.ibm.com> writes:

> On Fri, May 26, 2017 at 09:03:13AM +0200, Thomas Gleixner wrote:
>> > It seems like it really should. That would put it in a single place
>> > and avoid this mistake again in the future. Does module_memfree() have
>> > access to the allocation size, or does that need to get plumbed?
>> 
>> No, it doesn't. But the number of instances is pretty limited.
>> 
>> Btw, looking at BPF. It allocates memory via module_alloc() which means
>> it's RWX. There is nothing in that BPF code which changes the permissions
>> afterwards ....
>
> For BPF you're probably referring to bpf_jit_binary_alloc()? Permissions
> are changed with bpf_jit_binary_lock_ro() within each architecure backend.
>
> Well, except for powerpc (cc'ed Michael).

[hangs head in shame]

Thanks, we are working on this stuff (stricter RWX perms) at the moment,
so will add this to the list. It's complicated somewhat by the variety
of MMUs we support, but still.

cheers

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


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

FromMasami Hiramatsu <mhiramat@kernel.org>
Date2017-05-26 11:50 +0200
SubjectRe: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tLk2B-4cR-1@gated-at.bofh.it>
In reply to#1650781
On Thu, 25 May 2017 12:51:21 -0700
Kees Cook <keescook@chromium.org> wrote:

> On Thu, May 25, 2017 at 10:46 AM, Luis R. Rodriguez <mcgrof@kernel.org> wrote:
> > On Thu, May 25, 2017 at 10:57:51AM +0200, Thomas Gleixner 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>
> >> ---
> >>  arch/x86/kernel/ftrace.c |   20 ++++++++++++++------
> >>  1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> --- a/arch/x86/kernel/ftrace.c
> >> +++ b/arch/x86/kernel/ftrace.c
> >> @@ -689,8 +689,12 @@ static inline void *alloc_tramp(unsigned
> >>  {
> >>       return module_alloc(size);
> >>  }
> >> -static inline void tramp_free(void *tramp)
> >> +static inline void tramp_free(void *tramp, int size)
> >>  {
> >> +     int npages = PAGE_ALIGN(size) >> PAGE_SHIFT;
> >> +
> >> +     set_memory_nx((unsigned long)tramp, npages);
> >> +     set_memory_rw((unsigned long)tramp, npages);
> >>       module_memfree(tramp);
> >>  }
> >
> > Can/should module_memfree() just do this for users? With Masami's fix that'd
> > be 2 users already.
> 
> It seems like it really should. That would put it in a single place
> and avoid this mistake again in the future. Does module_memfree() have
> access to the allocation size, or does that need to get plumbed?

module_memfree() is just a wrapper of vfree, so find_vm_area()
will help us to get the size.

Thank you,


> 
> -Kees
> 
> -- 
> Kees Cook
> Pixel Security


-- 
Masami Hiramatsu <mhiramat@kernel.org>

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


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

FromSteven Rostedt <rostedt@goodmis.org>
Date2017-05-26 15:40 +0200
SubjectRe: [PATCH V2] x86/ftrace: Make sure that ftrace trampolines are not RWX
Message-ID<tLnDb-6sK-5@gated-at.bofh.it>
In reply to#1650336
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!
invalid opcode: 0000 [#1] SMP
Modules linked in:
CPU: 0 PID: 0 Comm: swapper Not tainted 4.12.0-rc2-test+ #3
Hardware name: MSI MS-7823/CSM-H87M-G43 (MS-7823), BIOS V1.6 02/22/2014
task: ffffffffb4222500 task.stack: ffffffffb4200000
RIP: 0010:change_page_attr_set_clr+0x269/0x302
RSP: 0000:ffffffffb4203c88 EFLAGS: 00010046
RAX: 0000000000000046 RBX: 0000000000000000 RCX: 00000001b6000000
RDX: ffffffffb4203d40 RSI: 0000000000000000 RDI: ffffffffb4240d60
RBP: ffffffffb4203d18 R08: 00000001b6000000 R09: 0000000000000001
R10: ffffffffb4203aa8 R11: 0000000000000003 R12: ffffffffc029b000
R13: ffffffffb4203d40 R14: 0000000000000001 R15: 0000000000000000
FS:  0000000000000000(0000) GS:ffff9a639ea00000(0000) knlGS:0000000000000000
CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffff9a636b384000 CR3: 00000001ea21d000 CR4: 00000000000406b0
Call Trace:
 change_page_attr_clear+0x1f/0x21
 set_memory_ro+0x1e/0x20
 arch_ftrace_update_trampoline+0x207/0x21c
 ? ftrace_caller+0x64/0x64
 ? 0xffffffffc029b000
 ftrace_startup+0xf4/0x198
 register_ftrace_function+0x26/0x3c
 function_trace_init+0x5e/0x73
 tracer_init+0x1e/0x23
 tracing_set_tracer+0x127/0x15a
 register_tracer+0x19b/0x1bc
 init_function_trace+0x90/0x92
 early_trace_init+0x236/0x2b3
 start_kernel+0x200/0x3f5
 x86_64_start_reservations+0x29/0x2b
 x86_64_start_kernel+0x17c/0x18f
 secondary_startup_64+0x9f/0x9f
 ? secondary_startup_64+0x9f/0x9f
Code: 89 df e8 79 f4 ff ff 48 85 c0 74 12 f6 00 01 74 0d be 00 10 00 00 48 89 df e8 84 e8 ff ff 49 ff c4 eb a4 9c 58 0f ba e0 09 72 02 <0f> 0b 49 8d 84 24 ff 0f 00 00 48 25 00 f0 ff ff 49 39 c4 74 02
RIP: change_page_attr_set_clr+0x269/0x302 RSP: ffffffffb4203c88
---[ end trace 418d67f4f812a298 ]---
Kernel panic - not syncing: Attempted to kill the idle task!
---[ end Kernel panic - not syncing: Attempted to kill the idle task!

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

in cpa_flush_range()

-- Steve

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


#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] | [standalone]


Back to top | Article view | linux.kernel


csiph-web