[PATCH] [GIT PULL] tracing: allow to change permissions for text with dynamic ftrace enabled

[PATCH] [GIT PULL] tracing: allow to change permissions for text with dynamic ftrace enabled

[Steven Rostedt]

Ingo,

The cause of the ftrace bug you saw in tip/master was caused by a change
in tip's x86/mm branch.

Please pull the latest tip/x86/mm tree, which can be found at:

  git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
tip/x86/mm


Steven Rostedt (1):
      tracing: allow to change permissions for text with dynamic ftrace enabled

----
 arch/x86/mm/pageattr.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
---------------------------
commit 883242dd0e5faaba041528a9a99f483f2a656c83
Author: Steven Rostedt <srostedt@redhat.com>
Date:   Tue Oct 27 13:15:11 2009 -0400

    tracing: allow to change permissions for text with dynamic ftrace enabled
    
    The commit 74e081797bd9d2a7d8005fe519e719df343a2ba8
    x86-64: align RODATA kernel section to 2MB with CONFIG_DEBUG_RODATA
    prevents text sections from becoming read/write using set_memory_rw.
    
    The dynamic ftrace changes all text pages to read/write just before
    converting the calls to tracing to nops, and vice versa.
    
    I orginally just added a flag to allow this transaction when ftrace
    did the change, but I also found that when the CPA testing was running
    it would remove the read/write as well, and ftrace does not do the text
    conversion on boot up, and the CPA changes caused the dynamic tracer
    to fail on self tests.
    
    The current solution I have is to simply not to prevent
    change_page_attr from setting the RW bit for kernel text pages.
    
    Reported-by: Ingo Molnar <mingo@elte.hu>
    Cc: Suresh Siddha <suresh.b.siddha@intel.com>
    Cc: H. Peter Anvin <hpa@zytor.com>
    Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index b494fc4..78d3168 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -279,7 +279,8 @@ static inline pgprot_t static_protections(pgprot_t prot, unsigned long address,
 		   __pa((unsigned long)__end_rodata) >> PAGE_SHIFT))
 		pgprot_val(forbidden) |= _PAGE_RW;
 
-#if defined(CONFIG_X86_64) && defined(CONFIG_DEBUG_RODATA)
+#if defined(CONFIG_X86_64) && defined(CONFIG_DEBUG_RODATA) && \
+	!defined(CONFIG_DYNAMIC_FTRACE)
 	/*
 	 * Kernel text mappings for the large page aligned .rodata section
 	 * will be read-only. For the kernel identity mappings covering

Re: [PATCH] [GIT PULL] tracing: allow to change permissions for text with dynamic ftrace enabled

[Ingo Molnar]

* Steven Rostedt <rostedt@goodmis.org> wrote:

> 
> Ingo,
> 
> The cause of the ftrace bug you saw in tip/master was caused by a change
> in tip's x86/mm branch.
> 
> Please pull the latest tip/x86/mm tree, which can be found at:
> 
>   git://git.kernel.org/pub/scm/linux/kernel/git/rostedt/linux-2.6-trace.git
> tip/x86/mm
> 
> 
> Steven Rostedt (1):
>       tracing: allow to change permissions for text with dynamic ftrace enabled
> 
> ----
>  arch/x86/mm/pageattr.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> ---------------------------
> commit 883242dd0e5faaba041528a9a99f483f2a656c83
> Author: Steven Rostedt <srostedt@redhat.com>
> Date:   Tue Oct 27 13:15:11 2009 -0400
> 
>     tracing: allow to change permissions for text with dynamic ftrace enabled
>     
>     The commit 74e081797bd9d2a7d8005fe519e719df343a2ba8
>     x86-64: align RODATA kernel section to 2MB with CONFIG_DEBUG_RODATA
>     prevents text sections from becoming read/write using set_memory_rw.
>     
>     The dynamic ftrace changes all text pages to read/write just before
>     converting the calls to tracing to nops, and vice versa.
>     
>     I orginally just added a flag to allow this transaction when ftrace
>     did the change, but I also found that when the CPA testing was running
>     it would remove the read/write as well, and ftrace does not do the text
>     conversion on boot up, and the CPA changes caused the dynamic tracer
>     to fail on self tests.
>     
>     The current solution I have is to simply not to prevent
>     change_page_attr from setting the RW bit for kernel text pages.
>     
>     Reported-by: Ingo Molnar <mingo@elte.hu>
>     Cc: Suresh Siddha <suresh.b.siddha@intel.com>
>     Cc: H. Peter Anvin <hpa@zytor.com>
>     Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> 
> diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
> index b494fc4..78d3168 100644
> --- a/arch/x86/mm/pageattr.c
> +++ b/arch/x86/mm/pageattr.c
> @@ -279,7 +279,8 @@ static inline pgprot_t static_protections(pgprot_t prot, unsigned long address,
>  		   __pa((unsigned long)__end_rodata) >> PAGE_SHIFT))
>  		pgprot_val(forbidden) |= _PAGE_RW;
>  
> -#if defined(CONFIG_X86_64) && defined(CONFIG_DEBUG_RODATA)
> +#if defined(CONFIG_X86_64) && defined(CONFIG_DEBUG_RODATA) && \
> +	!defined(CONFIG_DYNAMIC_FTRACE)
>  	/*
>  	 * Kernel text mappings for the large page aligned .rodata section
>  	 * will be read-only. For the kernel identity mappings covering

Pulled, thanks Steve!

	Ingo

Re: [PATCH] [GIT PULL] tracing: allow to change permissions for text with dynamic ftrace enabled

[Suresh Siddha]

On Tue, 2009-10-27 at 10:53 -0700, Steven Rostedt wrote:
> commit 883242dd0e5faaba041528a9a99f483f2a656c83
> Author: Steven Rostedt <srostedt@redhat.com>
> Date:   Tue Oct 27 13:15:11 2009 -0400
> 
>     tracing: allow to change permissions for text with dynamic ftrace enabled
>     
>     The commit 74e081797bd9d2a7d8005fe519e719df343a2ba8
>     x86-64: align RODATA kernel section to 2MB with CONFIG_DEBUG_RODATA
>     prevents text sections from becoming read/write using set_memory_rw.
>     
>     The dynamic ftrace changes all text pages to read/write just before
>     converting the calls to tracing to nops, and vice versa.
>     
>     I orginally just added a flag to allow this transaction when ftrace
>     did the change, but I also found that when the CPA testing was running
>     it would remove the read/write as well, and ftrace does not do the text
>     conversion on boot up, and the CPA changes caused the dynamic tracer
>     to fail on self tests.
>     
>     The current solution I have is to simply not to prevent
>     change_page_attr from setting the RW bit for kernel text pages.


Steven, Is it possible for dynamic ftrace to use the kernel identity
mapping instead of the kernel text mapping for converting the tracing
calls to nops etc.

That way, we can still retain large page mappings for kernel text, while
allowing the text to be changed.

I need to think through this to see if there are any subtle issues. What
do you think?

thanks,
suresh

Re: [PATCH] [GIT PULL] tracing: allow to change permissions for text with dynamic ftrace enabled

[Steven Rostedt]

On Tue, 2009-10-27 at 11:20 -0800, Suresh Siddha wrote:

> Steven, Is it possible for dynamic ftrace to use the kernel identity
> mapping instead of the kernel text mapping for converting the tracing
> calls to nops etc.

Not sure what you mean by "kernel identity" mapping.

> 
> That way, we can still retain large page mappings for kernel text, while
> allowing the text to be changed.
> 
> I need to think through this to see if there are any subtle issues. What
> do you think?

Yeah, this is for now the quick fix. But I'm sure there can be better
changes to fix this.

-- Steve

Re: [PATCH] [GIT PULL] tracing: allow to change permissions for text with dynamic ftrace enabled

[Suresh Siddha]

On Tue, 2009-10-27 at 11:33 -0700, Steven Rostedt wrote:
> On Tue, 2009-10-27 at 11:20 -0800, Suresh Siddha wrote:
> 
> > Steven, Is it possible for dynamic ftrace to use the kernel identity
> > mapping instead of the kernel text mapping for converting the tracing
> > calls to nops etc.
> 
> Not sure what you mean by "kernel identity" mapping.

64bit has the kernel text pages mapped at two locations. kernel identity
mapping  (__PAGE_OFFSET) and kernel image/text mapping
(__START_KERNEL_map).

DEBUG_RODATA patch was trying to preserve large page mapping (for perf
reasons) for kernel text. We can use the identity mapping for modifying
the kernel text.

This patch seems to fix.

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 5a1b975..7e1799b 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -215,6 +215,11 @@ do_ftrace_mod_code(unsigned long ip, void *new_code)
 }
 
 
+static inline int
+within(unsigned long addr, unsigned long start, unsigned long end)
+{
+	return addr >= start && addr < end;
+}
 
 
 static unsigned char ftrace_nop[MCOUNT_INSN_SIZE];
@@ -248,6 +253,14 @@ ftrace_modify_code(unsigned long ip, unsigned char *old_code,
 	if (memcmp(replaced, old_code, MCOUNT_INSN_SIZE) != 0)
 		return -EINVAL;
 
+	/*
+	 * On x86_64, we use the kernel identity mapping instead of the
+	 * kernel text mapping to modify the kernel text. This is a nop
+	 * for 32bit kernels.
+	 */
+	if (within(ip, (unsigned long)_text, (unsigned long)_etext))
+		ip = (unsigned long)__va(__pa(ip));
+
 	/* replace the text with the new text */
 	if (do_ftrace_mod_code(ip, new_code))
 		return -EPERM;

Re: [PATCH] [GIT PULL] tracing: allow to change permissions for text with dynamic ftrace enabled

[Steven Rostedt]

On Tue, 2009-10-27 at 14:26 -0800, Suresh Siddha wrote:
> On Tue, 2009-10-27 at 11:33 -0700, Steven Rostedt wrote:
> > On Tue, 2009-10-27 at 11:20 -0800, Suresh Siddha wrote:
> > 
> > > Steven, Is it possible for dynamic ftrace to use the kernel identity
> > > mapping instead of the kernel text mapping for converting the tracing
> > > calls to nops etc.
> > 
> > Not sure what you mean by "kernel identity" mapping.
> 
> 64bit has the kernel text pages mapped at two locations. kernel identity
> mapping  (__PAGE_OFFSET) and kernel image/text mapping
> (__START_KERNEL_map).

I didn't realize that x86_64 mapped it two different ways.

> 
> DEBUG_RODATA patch was trying to preserve large page mapping (for perf
> reasons) for kernel text. We can use the identity mapping for modifying
> the kernel text.
> 
> This patch seems to fix.
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 5a1b975..7e1799b 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -215,6 +215,11 @@ do_ftrace_mod_code(unsigned long ip, void *new_code)
>  }
>  
> 
> +static inline int
> +within(unsigned long addr, unsigned long start, unsigned long end)
> +{
> +	return addr >= start && addr < end;
> +}
>  
> 
>  static unsigned char ftrace_nop[MCOUNT_INSN_SIZE];
> @@ -248,6 +253,14 @@ ftrace_modify_code(unsigned long ip, unsigned char *old_code,
>  	if (memcmp(replaced, old_code, MCOUNT_INSN_SIZE) != 0)
>  		return -EINVAL;
>  
> +	/*
> +	 * On x86_64, we use the kernel identity mapping instead of the
> +	 * kernel text mapping to modify the kernel text. This is a nop
> +	 * for 32bit kernels.
> +	 */

Is it really a nop on 32bit? Does it just turn into ip = ip?


> +	if (within(ip, (unsigned long)_text, (unsigned long)_etext))
> +		ip = (unsigned long)__va(__pa(ip));
> +
>  	/* replace the text with the new text */
>  	if (do_ftrace_mod_code(ip, new_code))
>  		return -EPERM;

I'll test it out, and if it does work, you can write up a formal patch
and remove the !define that I added.

Thanks,

-- Steve

Re: [PATCH] [GIT PULL] tracing: allow to change permissions for text with dynamic ftrace enabled

[Suresh Siddha]

On Tue, 2009-10-27 at 14:35 -0700, Steven Rostedt wrote: 
> On Tue, 2009-10-27 at 14:26 -0800, Suresh Siddha wrote:
> >  
> > +	/*
> > +	 * On x86_64, we use the kernel identity mapping instead of the
> > +	 * kernel text mapping to modify the kernel text. This is a nop
> > +	 * for 32bit kernels.
> > +	 */
> 
> Is it really a nop on 32bit? Does it just turn into ip = ip?

Yes. it will be ip = ip for 32bit.

> > +	if (within(ip, (unsigned long)_text, (unsigned long)_etext))
> > +		ip = (unsigned long)__va(__pa(ip));
> > +
> >  	/* replace the text with the new text */
> >  	if (do_ftrace_mod_code(ip, new_code))
> >  		return -EPERM;
> 
> I'll test it out, and if it does work, you can write up a formal patch
> and remove the !define that I added.

I just saw one more place calling do_ftrace_mod_code(). So moved this
check inside the do_ftrace_mod_code(). Does this cover all the cases?
Thanks.

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 5a1b975..e239fd7 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -189,9 +189,23 @@ static void wait_for_nmi(void)
 	nmi_wait_count++;
 }
 
+static inline int
+within(unsigned long addr, unsigned long start, unsigned long end)
+{
+	return addr >= start && addr < end;
+}
+
 static int
 do_ftrace_mod_code(unsigned long ip, void *new_code)
 {
+	/*
+	 * On x86_64, we use the kernel identity mapping instead of the
+	 * kernel text mapping to modify the kernel text. For 32bit kernels,
+	 * these mappings are same.
+	 */
+	if (within(ip, (unsigned long)_text, (unsigned long)_etext))
+		ip = (unsigned long)__va(__pa(ip));
+
 	mod_code_ip = (void *)ip;
 	mod_code_newcode = new_code;
 

Re: [PATCH] [GIT PULL] tracing: allow to change permissions for text with dynamic ftrace enabled

[Steven Rostedt]

On Tue, 2009-10-27 at 15:23 -0800, Suresh Siddha wrote:

> > 
> > I'll test it out, and if it does work, you can write up a formal patch
> > and remove the !define that I added.
> 
> I just saw one more place calling do_ftrace_mod_code(). So moved this
> check inside the do_ftrace_mod_code(). Does this cover all the cases?
> Thanks.

For ftrace, probably. But I just realized this may have other
consequences. The CPA_DEBUG tests setting a bit in the page table at
random locations. The code you added makes any change to kernel text
page attributes remove the RW bit. On boot up, it is considered that
kernel text is writable.

Perhaps you need to add a check in your if statement to only prevent
that bit when kernel_set_to_readonly is not true.

-- Steve

Re: [PATCH] [GIT PULL] tracing: allow to change permissions for text with dynamic ftrace enabled

[Suresh Siddha]

On Tue, 2009-10-27 at 16:31 -0700, Steven Rostedt wrote: 
> On Tue, 2009-10-27 at 15:23 -0800, Suresh Siddha wrote:
> 
> > > 
> > > I'll test it out, and if it does work, you can write up a formal patch
> > > and remove the !define that I added.
> > 
> > I just saw one more place calling do_ftrace_mod_code(). So moved this
> > check inside the do_ftrace_mod_code(). Does this cover all the cases?
> > Thanks.
> 
> For ftrace, probably. But I just realized this may have other
> consequences. The CPA_DEBUG tests setting a bit in the page table at
> random locations.

CPA_DEBUG is setting/clearing unused bit 9. It has no interaction with
RW bits. Though the random page and random length selections may end up
splitting the large page mappings for kernel text. This is a different
issue that needs to be addressed (perhaps I can ignore this bit 9 as
well for the kernel text mapping. In general it sounds like a bad idea
to break any large page mappings to small page mappings for cpa debug
tests, especially when we don't restore the large page mapping when we
clear those unused bits once the test is done. Will look into this).

> The code you added makes any change to kernel text
> page attributes remove the RW bit.

Only for CONFIG_DEBUG_RODATA.

> On boot up, it is considered that
> kernel text is writable.

For CONFIG_DEBUG_RODATA, during the boot we change it to RO.

> Perhaps you need to add a check in your if statement to only prevent
> that bit when kernel_set_to_readonly is not true.

Are you worried that kernel_set_to_readonly might not be true but we
might have the kernel text mapping read-only?

As ftrace is now using kernel identity mapping on 64-bit, we can change
the semantics of kernel_set_to_readonly to reflect the status of kernel
identity mapping for 64-bit. I can update the comments.

thanks,
suresh

Re: [PATCH] [GIT PULL] tracing: allow to change permissions for text with dynamic ftrace enabled

[Steven Rostedt]

On Wed, 2009-10-28 at 11:56 -0800, Suresh Siddha wrote:
> On Tue, 2009-10-27 at 16:31 -0700, Steven Rostedt wrote: 
> > On Tue, 2009-10-27 at 15:23 -0800, Suresh Siddha wrote:
> > 
> > > > 
> > > > I'll test it out, and if it does work, you can write up a formal patch
> > > > and remove the !define that I added.
> > > 
> > > I just saw one more place calling do_ftrace_mod_code(). So moved this
> > > check inside the do_ftrace_mod_code(). Does this cover all the cases?
> > > Thanks.
> > 
> > For ftrace, probably. But I just realized this may have other
> > consequences. The CPA_DEBUG tests setting a bit in the page table at
> > random locations.
> 
> CPA_DEBUG is setting/clearing unused bit 9. It has no interaction with
> RW bits. Though the random page and random length selections may end up

Actually it does. When you call the clear_page_attribute, and go to set
or clear bit 9, the static_protections() function is called against the
new_prot. This will clear the RW bit if it is set, even if the original
caller did not touch that bit.

> splitting the large page mappings for kernel text. This is a different
> issue that needs to be addressed (perhaps I can ignore this bit 9 as
> well for the kernel text mapping. In general it sounds like a bad idea
> to break any large page mappings to small page mappings for cpa debug
> tests, especially when we don't restore the large page mapping when we
> clear those unused bits once the test is done. Will look into this).
> 
> > The code you added makes any change to kernel text
> > page attributes remove the RW bit.
> 
> Only for CONFIG_DEBUG_RODATA.
> 
> > On boot up, it is considered that
> > kernel text is writable.
> 
> For CONFIG_DEBUG_RODATA, during the boot we change it to RO.

We do that at the end of boot up. I'm sure there's a reason for this :-/

> 
> > Perhaps you need to add a check in your if statement to only prevent
> > that bit when kernel_set_to_readonly is not true.
> 
> Are you worried that kernel_set_to_readonly might not be true but we
> might have the kernel text mapping read-only?

Yes, because the code clears the RW bit whenever something changes any
attribute of that page table. Including bit 9.

> 
> As ftrace is now using kernel identity mapping on 64-bit, we can change
> the semantics of kernel_set_to_readonly to reflect the status of kernel
> identity mapping for 64-bit. I can update the comments.

Well, the identity mapping should still be set to readonly after boot
up, right?

-- Steve

Re: [PATCH] [GIT PULL] tracing: allow to change permissions for text with dynamic ftrace enabled

[Suresh Siddha]

On Wed, 2009-10-28 at 13:15 -0700, Steven Rostedt wrote:
> On Wed, 2009-10-28 at 11:56 -0800, Suresh Siddha wrote:
> > CPA_DEBUG is setting/clearing unused bit 9. It has no interaction with
> > RW bits. Though the random page and random length selections may end up
> 
> Actually it does. When you call the clear_page_attribute, and go to set
> or clear bit 9, the static_protections() function is called against the
> new_prot. This will clear the RW bit if it is set, even if the original
> caller did not touch that bit.

True. It can happen if the debug test or someone does cpa() before we
mark the text as RO.

> > For CONFIG_DEBUG_RODATA, during the boot we change it to RO.
> 
> We do that at the end of boot up. I'm sure there's a reason for this :-/

Well, alternate_instructions() is one :(. I am convinced that I should
add a check of kernel_set_to_readonly, and if set, I should prevent the
write attribute for the whole large page mapping kernel text, so that we
don't end up breaking it to small pages..

> Yes, because the code clears the RW bit whenever something changes any
> attribute of that page table. Including bit 9.

Yep, noticed it after your earlier comments.

Will send another patch to fix this too. You are ok with the ftrace
change right? I will send both of these shortly.

thanks,
suresh

Re: [PATCH] [GIT PULL] tracing: allow to change permissions for text with dynamic ftrace enabled

[Steven Rostedt]

On Wed, 2009-10-28 at 14:07 -0800, Suresh Siddha wrote:

> Will send another patch to fix this too. You are ok with the ftrace
> change right? I will send both of these shortly.

Yep, I thoroughly tested your ftrace change. Looks good. You can even
add a "Tested-by" me too. I took out my previous change, and your patch
worked fine.

Just worried about the other side effects that we discussed. But that
has nothing to do with ftrace.

Thanks,

-- Steve