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

Groups > linux.kernel > #1602325 > unrolled thread

[PATCH] acpi: fix incompatibility with mcount-based function graph tracing

Back to article view | Back to linux.kernel

Contents

  [PATCH] acpi: fix incompatibility with mcount-based function graph tracing Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-16 15:30 +0100
    Re: [PATCH] acpi: fix incompatibility with mcount-based function  graph tracing Steven Rostedt <rostedt@goodmis.org> - 2017-03-16 15:50 +0100
    Re: [PATCH] acpi: fix incompatibility with mcount-based function  graph tracing Paul Menzel <pmenzel@molgen.mpg.de> - 2017-03-21 21:50 +0100
      Re: [PATCH] acpi: fix incompatibility with mcount-based function  graph tracing Paul Menzel <pmenzel@molgen.mpg.de> - 2017-03-22 01:10 +0100
      Re: [PATCH] acpi: fix incompatibility with mcount-based function  graph tracing Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-24 19:20 +0100
        Re: [PATCH] acpi: fix incompatibility with mcount-based function  graph tracing Steven Rostedt <rostedt@goodmis.org> - 2017-03-24 19:50 +0100
          Re: [PATCH] acpi: fix incompatibility with mcount-based function graph tracing "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-25 14:30 +0100
            Re: [PATCH] acpi: fix incompatibility with mcount-based function  graph tracing Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-27 16:20 +0200
              [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing and  suspend-to-ram Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-27 17:00 +0200
                Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing  and suspend-to-ram Paul Menzel <pmenzel@molgen.mpg.de> - 2017-03-27 17:10 +0200
                  Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing  and suspend-to-ram Steven Rostedt <rostedt@goodmis.org> - 2017-03-27 17:30 +0200
                  [PATCH v2] ftrace/x86: fix x86-32 triple fault with graph tracing  and suspend-to-ram Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-27 17:30 +0200
                  Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing  and suspend-to-ram Paul Menzel <pmenzel@molgen.mpg.de> - 2017-03-28 12:00 +0200
                    Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing  and suspend-to-ram Steven Rostedt <rostedt@goodmis.org> - 2017-03-28 17:50 +0200
                      Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing  and suspend-to-ram Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-28 18:00 +0200
                        Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing and suspend-to-ram "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-28 23:20 +0200
                          Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing  and suspend-to-ram Josh Poimboeuf <jpoimboe@redhat.com> - 2017-03-28 23:50 +0200
                            Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing and suspend-to-ram "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-29 00:00 +0200
              Re: [PATCH] acpi: fix incompatibility with mcount-based function graph tracing "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2017-03-27 19:10 +0200

#1602325 — [PATCH] acpi: fix incompatibility with mcount-based function graph tracing

Paul Menzel reported a warning:

  WARNING: CPU: 0 PID: 774 at /build/linux-ROBWaj/linux-4.9.13/kernel/trace/trace_functions_graph.c:233 ftrace_return_to_handler+0x1aa/0x1e0
  Bad frame pointer: expected f6919d98, received f6919db0
    from func acpi_pm_device_sleep_wake return to c43b6f9d

The warning means that function graph tracing is broken for the
acpi_pm_device_sleep_wake() function.  That's because the acpi Makefile
unconditionally sets the '-Os' gcc flag to optimize for size.  That's an
issue because mcount-based function graph tracing is incompatible with
'-Os' on x86, thanks to the following gcc bug:

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109

I have another patch pending which will ensure that mcount-based
function graph tracing is never used with CONFIG_CC_OPTIMIZE_FOR_SIZE on
x86.

But this patch is needed in addition to that one because the ACPI
Makefile overrides that config option for no apparent reason.  It has
had this flag since the beginning of git history, and there's no related
comment, so I don't know why it's there.  As far as I can tell, there's
no reason for it to be there.  The appropriate behavior is for it to
honor CONFIG_CC_OPTIMIZE_FOR_{SIZE,PERFORMANCE} like the rest of the
kernel.

Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 drivers/acpi/Makefile | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
index a391bbc..d94f92f 100644
--- a/drivers/acpi/Makefile
+++ b/drivers/acpi/Makefile
@@ -2,7 +2,6 @@
 # Makefile for the Linux ACPI interpreter
 #
 
-ccflags-y			:= -Os
 ccflags-$(CONFIG_ACPI_DEBUG)	+= -DACPI_DEBUG_OUTPUT
 
 #
-- 
2.7.4

[toc] | [next] | [standalone]

#1602380 — Re: [PATCH] acpi: fix incompatibility with mcount-based function graph tracing

On Thu, 16 Mar 2017 08:56:28 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:

> Paul Menzel reported a warning:
> 
>   WARNING: CPU: 0 PID: 774 at /build/linux-ROBWaj/linux-4.9.13/kernel/trace/trace_functions_graph.c:233 ftrace_return_to_handler+0x1aa/0x1e0
>   Bad frame pointer: expected f6919d98, received f6919db0
>     from func acpi_pm_device_sleep_wake return to c43b6f9d
> 
> The warning means that function graph tracing is broken for the
> acpi_pm_device_sleep_wake() function.  That's because the acpi Makefile
> unconditionally sets the '-Os' gcc flag to optimize for size.  That's an
> issue because mcount-based function graph tracing is incompatible with
> '-Os' on x86, thanks to the following gcc bug:
> 
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109
> 
> I have another patch pending which will ensure that mcount-based
> function graph tracing is never used with CONFIG_CC_OPTIMIZE_FOR_SIZE on
> x86.
> 
> But this patch is needed in addition to that one because the ACPI
> Makefile overrides that config option for no apparent reason.  It has
> had this flag since the beginning of git history, and there's no related
> comment, so I don't know why it's there.  As far as I can tell, there's
> no reason for it to be there.  The appropriate behavior is for it to
> honor CONFIG_CC_OPTIMIZE_FOR_{SIZE,PERFORMANCE} like the rest of the
> kernel.
> 
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  drivers/acpi/Makefile | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index a391bbc..d94f92f 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -2,7 +2,6 @@
>  # Makefile for the Linux ACPI interpreter
>  #
>  
> -ccflags-y			:= -Os

Unless somebody can come up with a real reason to have this,

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

>  ccflags-$(CONFIG_ACPI_DEBUG)	+= -DACPI_DEBUG_OUTPUT
>  
>  #

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

#1605986 — Re: [PATCH] acpi: fix incompatibility with mcount-based function graph tracing

Dear Josh,


On 2017-03-16 14:56, Josh Poimboeuf wrote:
> Paul Menzel reported a warning:
> 
>   WARNING: CPU: 0 PID: 774 at
> /build/linux-ROBWaj/linux-4.9.13/kernel/trace/trace_functions_graph.c:233
> ftrace_return_to_handler+0x1aa/0x1e0
>   Bad frame pointer: expected f6919d98, received f6919db0
>     from func acpi_pm_device_sleep_wake return to c43b6f9d
> 
> The warning means that function graph tracing is broken for the
> acpi_pm_device_sleep_wake() function.  That's because the acpi Makefile
> unconditionally sets the '-Os' gcc flag to optimize for size.  That's 
> an
> issue because mcount-based function graph tracing is incompatible with
> '-Os' on x86, thanks to the following gcc bug:
> 
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109
> 
> I have another patch pending which will ensure that mcount-based
> function graph tracing is never used with CONFIG_CC_OPTIMIZE_FOR_SIZE 
> on
> x86.
> 
> But this patch is needed in addition to that one because the ACPI
> Makefile overrides that config option for no apparent reason.  It has
> had this flag since the beginning of git history, and there's no 
> related
> comment, so I don't know why it's there.  As far as I can tell, there's
> no reason for it to be there.  The appropriate behavior is for it to
> honor CONFIG_CC_OPTIMIZE_FOR_{SIZE,PERFORMANCE} like the rest of the
> kernel.
> 
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  drivers/acpi/Makefile | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index a391bbc..d94f92f 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -2,7 +2,6 @@
>  # Makefile for the Linux ACPI interpreter
>  #
> 
> -ccflags-y			:= -Os
>  ccflags-$(CONFIG_ACPI_DEBUG)	+= -DACPI_DEBUG_OUTPUT
> 
>  #

I checked out Linux 4.9.16, applied your patch on top, and copied the 
Debian 4.9 Linux kernel configuration, did `make menuconfig`, disabled 
building debugging symbols, and executed `ARCH=i386 make -j40 deb-pkg`.

I installed that package on the Lenovo X60, and the result with tracing 
enabled has improved. The system suspends without a crash. 
Unfortunately, instead of resuming when pressing the power button, it 
starts from scratch. Suspend and resume without tracing enabled works 
though.

I’ll try to collect logs, but I don’t know, if there will be any, if the 
system just resets.

Maybe, this can be reproduced in QEMU?


Kind regards,

Paul

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

#1606109 — Re: [PATCH] acpi: fix incompatibility with mcount-based function graph tracing

Dear Josh,


On 2017-03-21 21:44, Paul Menzel wrote:

> On 2017-03-16 14:56, Josh Poimboeuf wrote:
>> Paul Menzel reported a warning:
>> 
>>   WARNING: CPU: 0 PID: 774 at
>> /build/linux-ROBWaj/linux-4.9.13/kernel/trace/trace_functions_graph.c:233
>> ftrace_return_to_handler+0x1aa/0x1e0
>>   Bad frame pointer: expected f6919d98, received f6919db0
>>     from func acpi_pm_device_sleep_wake return to c43b6f9d
>> 
>> The warning means that function graph tracing is broken for the
>> acpi_pm_device_sleep_wake() function.  That's because the acpi 
>> Makefile
>> unconditionally sets the '-Os' gcc flag to optimize for size.  That's 
>> an
>> issue because mcount-based function graph tracing is incompatible with
>> '-Os' on x86, thanks to the following gcc bug:
>> 
>>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=42109
>> 
>> I have another patch pending which will ensure that mcount-based
>> function graph tracing is never used with CONFIG_CC_OPTIMIZE_FOR_SIZE 
>> on
>> x86.
>> 
>> But this patch is needed in addition to that one because the ACPI
>> Makefile overrides that config option for no apparent reason.  It has
>> had this flag since the beginning of git history, and there's no 
>> related
>> comment, so I don't know why it's there.  As far as I can tell, 
>> there's
>> no reason for it to be there.  The appropriate behavior is for it to
>> honor CONFIG_CC_OPTIMIZE_FOR_{SIZE,PERFORMANCE} like the rest of the
>> kernel.
>> 
>> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> ---
>>  drivers/acpi/Makefile | 1 -
>>  1 file changed, 1 deletion(-)
>> 
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index a391bbc..d94f92f 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -2,7 +2,6 @@
>>  # Makefile for the Linux ACPI interpreter
>>  #
>> 
>> -ccflags-y			:= -Os
>>  ccflags-$(CONFIG_ACPI_DEBUG)	+= -DACPI_DEBUG_OUTPUT
>> 
>>  #
> 
> I checked out Linux 4.9.16, applied your patch on top, and copied the
> Debian 4.9 Linux kernel configuration, did `make menuconfig`, disabled
> building debugging symbols, and executed `ARCH=i386 make -j40
> deb-pkg`.
> 
> I installed that package on the Lenovo X60, and the result with
> tracing enabled has improved. The system suspends without a crash.
> Unfortunately, instead of resuming when pressing the power button, it
> starts from scratch. Suspend and resume without tracing enabled works
> though.
> 
> I’ll try to collect logs, but I don’t know, if there will be any, if
> the system just resets.

The system is able to resume with `no_console_suspend` passed to Linux 
on its command line.

```
$ more /proc/cmdline
BOOT_IMAGE=/vmlinuz-4.9.16 
root=UUID=0198e82a-32df-4f18-bec0-3e08802b084d ro noisapnp 
pcie_aspm=force pcie_aspm.policy=powersave nmi_watchdog=0 iomem=relaxed 
console=t
tyS1,115200 console=tty0 no_console_suspend
```

Please tell me, what you need from my side.

> Maybe, this can be reproduced in QEMU?


Kind regards,

Paul

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

#1608790 — Re: [PATCH] acpi: fix incompatibility with mcount-based function graph tracing

On Tue, Mar 21, 2017 at 09:44:03PM +0100, Paul Menzel wrote:
> I checked out Linux 4.9.16, applied your patch on top, and copied the Debian
> 4.9 Linux kernel configuration, did `make menuconfig`, disabled building
> debugging symbols, and executed `ARCH=i386 make -j40 deb-pkg`.
> 
> I installed that package on the Lenovo X60, and the result with tracing
> enabled has improved. The system suspends without a crash. Unfortunately,
> instead of resuming when pressing the power button, it starts from scratch.
> Suspend and resume without tracing enabled works though.
> 
> I’ll try to collect logs, but I don’t know, if there will be any, if the
> system just resets.
> 
> Maybe, this can be reproduced in QEMU?

So I was able to recreate this issue in qemu, and after some hours of
debugging I managed to figure it out.

It's rebooting during the resume because of a triple fault in
prepare_ftrace_return().

acpi wakeup for secondary cpu
  startup_32_smp()
    load_ucode_ap()
      prepare_ftrace_return()
        ftrace_graph_is_dead()
	  dereferences virtual address (kill_ftrace_graph) in real mode <-- BOOM

I tried fixing it by changing load_ucode_ap() to notrace, but that
function calls some other functions which also have mcount hooks, which
call other functions, etc.

Instead I was able to "fix" it by ignoring ftrace calls in real mode:

-----
index 8f3d9cf..5c0d0c6 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -983,6 +983,9 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
 	unsigned long return_hooker = (unsigned long)
 				&return_to_handler;
 
+	if (__builtin_return_address(0) < TASK_SIZE_MAX)
+		return;
+
 	if (unlikely(ftrace_graph_is_dead()))
 		return;
---------------

I'm not sure what the best fix should really be.  A few ideas off the
top of my head:

- A real mode check similar to the above (except it should probably be
  more precise)

- Make tracing_graph_pause a percpu variable so that it can be read from
  prepare_ftrace_return()

- pause_graph_tracing() from ftrace_suspend_notifier_call()

Steven, thoughts?

-- 
Josh

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

#1608863 — Re: [PATCH] acpi: fix incompatibility with mcount-based function graph tracing

On Fri, 24 Mar 2017 13:12:54 -0500
Josh Poimboeuf <jpoimboe@redhat.com> wrote:


> Instead I was able to "fix" it by ignoring ftrace calls in real mode:
> 
> -----
> index 8f3d9cf..5c0d0c6 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -983,6 +983,9 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
>  	unsigned long return_hooker = (unsigned long)
>  				&return_to_handler;
>  
> +	if (__builtin_return_address(0) < TASK_SIZE_MAX)
> +		return;
> +
>  	if (unlikely(ftrace_graph_is_dead()))
>  		return;
> ---------------
> 
> I'm not sure what the best fix should really be.  A few ideas off the
> top of my head:
> 
> - A real mode check similar to the above (except it should probably be
>   more precise)

The real mode check hack may be good enough for now. Make sure that
it's commented well.

-- Steve

> 
> - Make tracing_graph_pause a percpu variable so that it can be read from
>   prepare_ftrace_return()
> 
> - pause_graph_tracing() from ftrace_suspend_notifier_call()
> 
> Steven, thoughts?
> 

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

#1609154

On Friday, March 24, 2017 02:41:14 PM Steven Rostedt wrote:
> On Fri, 24 Mar 2017 13:12:54 -0500
> Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> 
> 
> > Instead I was able to "fix" it by ignoring ftrace calls in real mode:
> > 
> > -----
> > index 8f3d9cf..5c0d0c6 100644
> > --- a/arch/x86/kernel/ftrace.c
> > +++ b/arch/x86/kernel/ftrace.c
> > @@ -983,6 +983,9 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
> >  	unsigned long return_hooker = (unsigned long)
> >  				&return_to_handler;
> >  
> > +	if (__builtin_return_address(0) < TASK_SIZE_MAX)
> > +		return;
> > +
> >  	if (unlikely(ftrace_graph_is_dead()))
> >  		return;
> > ---------------
> > 
> > I'm not sure what the best fix should really be.  A few ideas off the
> > top of my head:
> > 
> > - A real mode check similar to the above (except it should probably be
> >   more precise)
> 
> The real mode check hack may be good enough for now. Make sure that
> it's commented well.

Agreed.

Thanks,
Rafael

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

#1609873 — Re: [PATCH] acpi: fix incompatibility with mcount-based function graph tracing

On Sat, Mar 25, 2017 at 02:20:11PM +0100, Rafael J. Wysocki wrote:
> On Friday, March 24, 2017 02:41:14 PM Steven Rostedt wrote:
> > On Fri, 24 Mar 2017 13:12:54 -0500
> > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > 
> > 
> > > Instead I was able to "fix" it by ignoring ftrace calls in real mode:
> > > 
> > > -----
> > > index 8f3d9cf..5c0d0c6 100644
> > > --- a/arch/x86/kernel/ftrace.c
> > > +++ b/arch/x86/kernel/ftrace.c
> > > @@ -983,6 +983,9 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
> > >  	unsigned long return_hooker = (unsigned long)
> > >  				&return_to_handler;
> > >  
> > > +	if (__builtin_return_address(0) < TASK_SIZE_MAX)
> > > +		return;
> > > +
> > >  	if (unlikely(ftrace_graph_is_dead()))
> > >  		return;
> > > ---------------
> > > 
> > > I'm not sure what the best fix should really be.  A few ideas off the
> > > top of my head:
> > > 
> > > - A real mode check similar to the above (except it should probably be
> > >   more precise)
> > 
> > The real mode check hack may be good enough for now. Make sure that
> > it's commented well.
> 
> Agreed.

Just to clarify, there are two bugs related to function graph tracing
and suspend/resume.  The original patch in this thread (which removes
'-Os' from the acpi Makefile) is still needed.

-- 
Josh

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

#1609906 — [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing and suspend-to-ram

On x86-32, with CONFIG_FIRMWARE and multiple CPUs, if you enable
function graph tracing and then suspend to RAM, it will triple fault and
reboot when it resumes.

The first fault happens when booting a secondary CPU:

startup_32_smp()
  load_ucode_ap()
    prepare_ftrace_return()
      ftrace_graph_is_dead()
        (accesses 'kill_ftrace_graph')

The early head_32.S code calls into load_ucode_ap(), which has an an
ftrace hook, so it calls prepare_ftrace_return(), which calls
ftrace_graph_is_dead(), which tries to access the global
'kill_ftrace_graph' variable with a virtual address, causing a fault
because the CPU is still in real mode.

The fix is to add a check in prepare_ftrace_return() to make sure it's
running in protected mode before continuing.  The check makes sure the
stack pointer is a virtual kernel address.  It's a bit of a hack, but
it's not very intrusive and it works well enough.

For reference, here are a few other ways this could have potentially
been fixed:

- Move startup_32_smp()'s call to load_ucode_ap() down to *after* paging
  is enabled.  (No idea what that would break.)

- Track down load_ucode_ap()'s entire callee tree and mark all the
  functions 'notrace'.  (Probably not realistic.)

- Pause graph tracing in ftrace_suspend_notifier_call() or bringup_cpu()
  or __cpu_up(), and ensure that the pause facility can be queried from
  real mode.

Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
 arch/x86/kernel/ftrace.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 8f3d9cf..1c5c4e2 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -983,6 +983,17 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
 	unsigned long return_hooker = (unsigned long)
 				&return_to_handler;
 
+	/*
+	 * When resuming from suspend-to-ram, this function can be indirectly
+	 * called from early CPU startup code while the CPU is in real mode,
+	 * which would fail miserably.  Make sure the stack pointer is a
+	 * virtual address.
+	 *
+	 * This check isn't as accurate as virt_addr_valid(), but it should be
+	 * good enough for this purpose, and it's fast.
+	 */
+	if (unlikely((long)__builtin_frame_address(0) >= 0)) return;
+
 	if (unlikely(ftrace_graph_is_dead()))
 		return;
 
-- 
2.7.4

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

#1609935 — Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing and suspend-to-ram

Dear Josh,


On 03/27/17 16:54, Josh Poimboeuf wrote:
> On x86-32, with CONFIG_FIRMWARE and multiple CPUs, if you enable
> function graph tracing and then suspend to RAM, it will triple fault and
> reboot when it resumes.
>
> The first fault happens when booting a secondary CPU:
>
> startup_32_smp()
>   load_ucode_ap()
>     prepare_ftrace_return()
>       ftrace_graph_is_dead()
>         (accesses 'kill_ftrace_graph')
>
> The early head_32.S code calls into load_ucode_ap(), which has an an
> ftrace hook, so it calls prepare_ftrace_return(), which calls
> ftrace_graph_is_dead(), which tries to access the global
> 'kill_ftrace_graph' variable with a virtual address, causing a fault
> because the CPU is still in real mode.
>
> The fix is to add a check in prepare_ftrace_return() to make sure it's
> running in protected mode before continuing.  The check makes sure the
> stack pointer is a virtual kernel address.  It's a bit of a hack, but
> it's not very intrusive and it works well enough.
>
> For reference, here are a few other ways this could have potentially
> been fixed:
>
> - Move startup_32_smp()'s call to load_ucode_ap() down to *after* paging
>   is enabled.  (No idea what that would break.)
>
> - Track down load_ucode_ap()'s entire callee tree and mark all the
>   functions 'notrace'.  (Probably not realistic.)
>
> - Pause graph tracing in ftrace_suspend_notifier_call() or bringup_cpu()
>   or __cpu_up(), and ensure that the pause facility can be queried from
>   real mode.
>
> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
> ---
>  arch/x86/kernel/ftrace.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

Thank you for debugging this. It’s great that you were able to reproduce 
this in QEMU. Hopefully, that’ll make for an easy test case. ;-)

> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 8f3d9cf..1c5c4e2 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -983,6 +983,17 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
>  	unsigned long return_hooker = (unsigned long)
>  				&return_to_handler;
>
> +	/*
> +	 * When resuming from suspend-to-ram, this function can be indirectly
> +	 * called from early CPU startup code while the CPU is in real mode,
> +	 * which would fail miserably.  Make sure the stack pointer is a
> +	 * virtual address.
> +	 *
> +	 * This check isn't as accurate as virt_addr_valid(), but it should be
> +	 * good enough for this purpose, and it's fast.
> +	 */
> +	if (unlikely((long)__builtin_frame_address(0) >= 0)) return;

The coding style requires the `return;` to be on a separate line.

> +
>  	if (unlikely(ftrace_graph_is_dead()))
>  		return;

I’ll test your change this evening.


Kind regards,

Paul

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

#1609947 — Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing and suspend-to-ram

On Mon, 27 Mar 2017 17:01:53 +0200
Paul Menzel <pmenzel@molgen.mpg.de> wrote:

> > +	/*
> > +	 * When resuming from suspend-to-ram, this function can be indirectly
> > +	 * called from early CPU startup code while the CPU is in real mode,
> > +	 * which would fail miserably.  Make sure the stack pointer is a
> > +	 * virtual address.
> > +	 *
> > +	 * This check isn't as accurate as virt_addr_valid(), but it should be
> > +	 * good enough for this purpose, and it's fast.
> > +	 */
> > +	if (unlikely((long)__builtin_frame_address(0) >= 0)) return;  
> 
> The coding style requires the `return;` to be on a separate line.

Correct, and new versions of a patch should always start a new thread
(unless it's a single update of a patch in a long patch series).
Otherwise they get ignored. (hint hint)

-- Steve



> 
> > +
> >  	if (unlikely(ftrace_graph_is_dead()))
> >  		return;  
> 

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

#1609949 — [PATCH v2] ftrace/x86: fix x86-32 triple fault with graph tracing and suspend-to-ram

On x86-32, with CONFIG_FIRMWARE and multiple CPUs, if you enable
function graph tracing and then suspend to RAM, it will triple fault and
reboot when it resumes.

The first fault happens when booting a secondary CPU:

startup_32_smp()
  load_ucode_ap()
    prepare_ftrace_return()
      ftrace_graph_is_dead()
        (accesses 'kill_ftrace_graph')

The early head_32.S code calls into load_ucode_ap(), which has an an
ftrace hook, so it calls prepare_ftrace_return(), which calls
ftrace_graph_is_dead(), which tries to access the global
'kill_ftrace_graph' variable with a virtual address, causing a fault
because the CPU is still in real mode.

The fix is to add a check in prepare_ftrace_return() to make sure it's
running in protected mode before continuing.  The check makes sure the
stack pointer is a virtual kernel address.  It's a bit of a hack, but
it's not very intrusive and it works well enough.

For reference, here are a few other (more difficult) ways this could
have potentially been fixed:

- Move startup_32_smp()'s call to load_ucode_ap() down to *after* paging
  is enabled.  (No idea what that would break.)

- Track down load_ucode_ap()'s entire callee tree and mark all the
  functions 'notrace'.  (Probably not realistic.)

- Pause graph tracing in ftrace_suspend_notifier_call() or bringup_cpu()
  or __cpu_up(), and ensure that the pause facility can be queried from
  real mode.

Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
---
v2:
- put return statement on its own line

 arch/x86/kernel/ftrace.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 8f3d9cf..5b71535 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -983,6 +983,18 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
 	unsigned long return_hooker = (unsigned long)
 				&return_to_handler;
 
+	/*
+	 * When resuming from suspend-to-ram, this function can be indirectly
+	 * called from early CPU startup code while the CPU is in real mode,
+	 * which would fail miserably.  Make sure the stack pointer is a
+	 * virtual address.
+	 *
+	 * This check isn't as accurate as virt_addr_valid(), but it should be
+	 * good enough for this purpose, and it's fast.
+	 */
+	if (unlikely((long)__builtin_frame_address(0) >= 0))
+		return;
+
 	if (unlikely(ftrace_graph_is_dead()))
 		return;
 
-- 
2.7.4

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

#1610504 — Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing and suspend-to-ram

Dear Josh,


On 03/27/17 17:01, Paul Menzel wrote:

> On 03/27/17 16:54, Josh Poimboeuf wrote:
>> On x86-32, with CONFIG_FIRMWARE and multiple CPUs, if you enable
>> function graph tracing and then suspend to RAM, it will triple fault and
>> reboot when it resumes.
>>
>> The first fault happens when booting a secondary CPU:
>>
>> startup_32_smp()
>>   load_ucode_ap()
>>     prepare_ftrace_return()
>>       ftrace_graph_is_dead()
>>         (accesses 'kill_ftrace_graph')
>>
>> The early head_32.S code calls into load_ucode_ap(), which has an an
>> ftrace hook, so it calls prepare_ftrace_return(), which calls
>> ftrace_graph_is_dead(), which tries to access the global
>> 'kill_ftrace_graph' variable with a virtual address, causing a fault
>> because the CPU is still in real mode.
>>
>> The fix is to add a check in prepare_ftrace_return() to make sure it's
>> running in protected mode before continuing.  The check makes sure the
>> stack pointer is a virtual kernel address.  It's a bit of a hack, but
>> it's not very intrusive and it works well enough.
>>
>> For reference, here are a few other ways this could have potentially
>> been fixed:
>>
>> - Move startup_32_smp()'s call to load_ucode_ap() down to *after* paging
>>   is enabled.  (No idea what that would break.)
>>
>> - Track down load_ucode_ap()'s entire callee tree and mark all the
>>   functions 'notrace'.  (Probably not realistic.)
>>
>> - Pause graph tracing in ftrace_suspend_notifier_call() or bringup_cpu()
>>   or __cpu_up(), and ensure that the pause facility can be queried from
>>   real mode.
>>
>> Reported-by: Paul Menzel <pmenzel@molgen.mpg.de>
>> Signed-off-by: Josh Poimboeuf <jpoimboe@redhat.com>
>> ---
>>  arch/x86/kernel/ftrace.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>
> Thank you for debugging this. It’s great that you were able to reproduce
> this in QEMU. Hopefully, that’ll make for an easy test case. ;-)
>
>> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
>> index 8f3d9cf..1c5c4e2 100644
>> --- a/arch/x86/kernel/ftrace.c
>> +++ b/arch/x86/kernel/ftrace.c
>> @@ -983,6 +983,17 @@ void prepare_ftrace_return(unsigned long
>> self_addr, unsigned long *parent,
>>      unsigned long return_hooker = (unsigned long)
>>                  &return_to_handler;
>>
>> +    /*
>> +     * When resuming from suspend-to-ram, this function can be
>> indirectly
>> +     * called from early CPU startup code while the CPU is in real mode,
>> +     * which would fail miserably.  Make sure the stack pointer is a
>> +     * virtual address.
>> +     *
>> +     * This check isn't as accurate as virt_addr_valid(), but it
>> should be
>> +     * good enough for this purpose, and it's fast.
>> +     */
>> +    if (unlikely((long)__builtin_frame_address(0) >= 0)) return;
>
> The coding style requires the `return;` to be on a separate line.
>
>> +
>>      if (unlikely(ftrace_graph_is_dead()))
>>          return;
>
> I’ll test your change this evening.

With both patches applied `./analyze_suspend.py -config 
suspend-callgraph.cfg -filter i915` succeeds on a Lenovo X60t, so 
suspend and resume work perfectly, when tracing is enabled.

Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>

It’d be awesome, if you could tag both patches for inclusion into the 
stable Linux Kernel series.


Kind regards,

Paul

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

#1611128 — Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing and suspend-to-ram

On Tue, 28 Mar 2017 11:51:45 +0200
Paul Menzel <pmenzel@molgen.mpg.de> wrote:

> With both patches applied `./analyze_suspend.py -config 
> suspend-callgraph.cfg -filter i915` succeeds on a Lenovo X60t, so 
> suspend and resume work perfectly, when tracing is enabled.
> 
> Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> 
> It’d be awesome, if you could tag both patches for inclusion into the 
> stable Linux Kernel series.

As long as they are not dependent on my patch series, I'm fine with
these going to stable.

-- Steve

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

#1611132 — Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing and suspend-to-ram

On Tue, Mar 28, 2017 at 11:39:41AM -0400, Steven Rostedt wrote:
> On Tue, 28 Mar 2017 11:51:45 +0200
> Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> 
> > With both patches applied `./analyze_suspend.py -config 
> > suspend-callgraph.cfg -filter i915` succeeds on a Lenovo X60t, so 
> > suspend and resume work perfectly, when tracing is enabled.
> > 
> > Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> > 
> > It’d be awesome, if you could tag both patches for inclusion into the 
> > stable Linux Kernel series.
> 
> As long as they are not dependent on my patch series, I'm fine with
> these going to stable.

Stable sounds fine to me too.  Both patches are independent of your
x86-32 fentry patch set.

-- 
Josh

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

#1611365 — Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing and suspend-to-ram

On Tuesday, March 28, 2017 10:55:46 AM Josh Poimboeuf wrote:
> On Tue, Mar 28, 2017 at 11:39:41AM -0400, Steven Rostedt wrote:
> > On Tue, 28 Mar 2017 11:51:45 +0200
> > Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> > 
> > > With both patches applied `./analyze_suspend.py -config 
> > > suspend-callgraph.cfg -filter i915` succeeds on a Lenovo X60t, so 
> > > suspend and resume work perfectly, when tracing is enabled.
> > > 
> > > Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> > > 
> > > It’d be awesome, if you could tag both patches for inclusion into the 
> > > stable Linux Kernel series.
> > 
> > As long as they are not dependent on my patch series, I'm fine with
> > these going to stable.
> 
> Stable sounds fine to me too.  Both patches are independent of your
> x86-32 fentry patch set.

Does https://patchwork.kernel.org/patch/9628301/ need to go into any particular
-stable series or just all of them?

Or should a Fixes: tag be added to it?

Thanks,
Rafael

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

#1611387 — Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing and suspend-to-ram

On Tue, Mar 28, 2017 at 11:12:42PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, March 28, 2017 10:55:46 AM Josh Poimboeuf wrote:
> > On Tue, Mar 28, 2017 at 11:39:41AM -0400, Steven Rostedt wrote:
> > > On Tue, 28 Mar 2017 11:51:45 +0200
> > > Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> > > 
> > > > With both patches applied `./analyze_suspend.py -config 
> > > > suspend-callgraph.cfg -filter i915` succeeds on a Lenovo X60t, so 
> > > > suspend and resume work perfectly, when tracing is enabled.
> > > > 
> > > > Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> > > > 
> > > > It’d be awesome, if you could tag both patches for inclusion into the 
> > > > stable Linux Kernel series.
> > > 
> > > As long as they are not dependent on my patch series, I'm fine with
> > > these going to stable.
> > 
> > Stable sounds fine to me too.  Both patches are independent of your
> > x86-32 fentry patch set.
> 
> Does https://patchwork.kernel.org/patch/9628301/ need to go into any particular
> -stable series or just all of them?
> 
> Or should a Fixes: tag be added to it?

As far as I can tell this issue has been around since the function_graph
tracer was introduced in 2008:

  15e6cb3673ea ("tracing: add a tracer to catch execution time of kernel functions")

(Though only for gcc >= 4.4.)

Not sure if it's overkill to specify 'Fixes' for an 8+ year old bug?  I
guess it can't hurt anything.

I think it can go in all of the stable branches.

-- 
Josh

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

#1611390 — Re: [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing and suspend-to-ram

On Tuesday, March 28, 2017 04:42:18 PM Josh Poimboeuf wrote:
> On Tue, Mar 28, 2017 at 11:12:42PM +0200, Rafael J. Wysocki wrote:
> > On Tuesday, March 28, 2017 10:55:46 AM Josh Poimboeuf wrote:
> > > On Tue, Mar 28, 2017 at 11:39:41AM -0400, Steven Rostedt wrote:
> > > > On Tue, 28 Mar 2017 11:51:45 +0200
> > > > Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> > > > 
> > > > > With both patches applied `./analyze_suspend.py -config 
> > > > > suspend-callgraph.cfg -filter i915` succeeds on a Lenovo X60t, so 
> > > > > suspend and resume work perfectly, when tracing is enabled.
> > > > > 
> > > > > Tested-by: Paul Menzel <pmenzel@molgen.mpg.de>
> > > > > 
> > > > > It’d be awesome, if you could tag both patches for inclusion into the 
> > > > > stable Linux Kernel series.
> > > > 
> > > > As long as they are not dependent on my patch series, I'm fine with
> > > > these going to stable.
> > > 
> > > Stable sounds fine to me too.  Both patches are independent of your
> > > x86-32 fentry patch set.
> > 
> > Does https://patchwork.kernel.org/patch/9628301/ need to go into any particular
> > -stable series or just all of them?
> > 
> > Or should a Fixes: tag be added to it?
> 
> As far as I can tell this issue has been around since the function_graph
> tracer was introduced in 2008:
> 
>   15e6cb3673ea ("tracing: add a tracer to catch execution time of kernel functions")
> 
> (Though only for gcc >= 4.4.)
> 
> Not sure if it's overkill to specify 'Fixes' for an 8+ year old bug?  I
> guess it can't hurt anything.
> 
> I think it can go in all of the stable branches.

OK, thanks!

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

#1609996

On Monday, March 27, 2017 09:08:43 AM Josh Poimboeuf wrote:
> On Sat, Mar 25, 2017 at 02:20:11PM +0100, Rafael J. Wysocki wrote:
> > On Friday, March 24, 2017 02:41:14 PM Steven Rostedt wrote:
> > > On Fri, 24 Mar 2017 13:12:54 -0500
> > > Josh Poimboeuf <jpoimboe@redhat.com> wrote:
> > > 
> > > 
> > > > Instead I was able to "fix" it by ignoring ftrace calls in real mode:
> > > > 
> > > > -----
> > > > index 8f3d9cf..5c0d0c6 100644
> > > > --- a/arch/x86/kernel/ftrace.c
> > > > +++ b/arch/x86/kernel/ftrace.c
> > > > @@ -983,6 +983,9 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
> > > >  	unsigned long return_hooker = (unsigned long)
> > > >  				&return_to_handler;
> > > >  
> > > > +	if (__builtin_return_address(0) < TASK_SIZE_MAX)
> > > > +		return;
> > > > +
> > > >  	if (unlikely(ftrace_graph_is_dead()))
> > > >  		return;
> > > > ---------------
> > > > 
> > > > I'm not sure what the best fix should really be.  A few ideas off the
> > > > top of my head:
> > > > 
> > > > - A real mode check similar to the above (except it should probably be
> > > >   more precise)
> > > 
> > > The real mode check hack may be good enough for now. Make sure that
> > > it's commented well.
> > 
> > Agreed.
> 
> Just to clarify, there are two bugs related to function graph tracing
> and suspend/resume.  The original patch in this thread (which removes
> '-Os' from the acpi Makefile) is still needed.

OK

Thanks,
Rafael

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web