All of lore.kernel.org
 help / color / mirror / Atom feed
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: Steven Rostedt <rostedt@goodmis.org>,
	Ingo Molnar <mingo@kernel.org>, Len Brown <lenb@kernel.org>,
	linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org,
	x86@kernel.org, Borislav Petkov <bp@alien8.de>,
	"Rafael J. Wysocki" <rjw@rjwysocki.net>
Subject: [PATCH v2] ftrace/x86: fix x86-32 triple fault with graph tracing and suspend-to-ram
Date: Mon, 27 Mar 2017 10:20:41 -0500	[thread overview]
Message-ID: <20170327152041.bxpgznmfnyah62zu@treble> (raw)
In-Reply-To: <d8fd5cfd-e273-ad29-9281-2436cb0c3723@molgen.mpg.de>

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


  reply	other threads:[~2017-03-27 15:21 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-16 13:56 [PATCH] acpi: fix incompatibility with mcount-based function graph tracing Josh Poimboeuf
2017-03-16 14:41 ` Steven Rostedt
2017-03-21 20:44 ` Paul Menzel
2017-03-22  0:04   ` Paul Menzel
2017-03-24 18:12   ` Josh Poimboeuf
2017-03-24 18:41     ` Steven Rostedt
2017-03-25 13:20       ` Rafael J. Wysocki
2017-03-27 14:08         ` Josh Poimboeuf
2017-03-27 14:54           ` [PATCH] ftrace/x86: fix x86-32 triple fault with graph tracing and suspend-to-ram Josh Poimboeuf
2017-03-27 15:01             ` Paul Menzel
2017-03-27 15:20               ` Josh Poimboeuf [this message]
2017-03-27 15:24               ` Steven Rostedt
2017-03-28  9:51               ` Paul Menzel
2017-03-28 15:39                 ` Steven Rostedt
2017-03-28 15:55                   ` Josh Poimboeuf
2017-03-28 21:12                     ` Rafael J. Wysocki
2017-03-28 21:42                       ` Josh Poimboeuf
2017-03-28 21:47                         ` Rafael J. Wysocki
2017-03-27 16:59           ` [PATCH] acpi: fix incompatibility with mcount-based function graph tracing Rafael J. Wysocki
2017-03-26 20:57 [PATCH] trace: Make trace_hwlat timestamp y2038 safe Deepa Dinamani
2017-03-27  9:25 ` kbuild test robot
2017-03-27  9:55   ` Arnd Bergmann
2017-03-27 14:28     ` Steven Rostedt
2017-03-27 14:53       ` Arnd Bergmann
2017-03-27 15:30         ` Steven Rostedt
2017-03-27 15:35           ` Arnd Bergmann
2017-03-27 16:11             ` Steven Rostedt
2017-03-27 21:02         ` Deepa Dinamani
2017-03-28  7:26           ` Arnd Bergmann
2017-03-27 10:04 ` kbuild test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170327152041.bxpgznmfnyah62zu@treble \
    --to=jpoimboe@redhat.com \
    --cc=bp@alien8.de \
    --cc=lenb@kernel.org \
    --cc=linux-acpi@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=pmenzel@molgen.mpg.de \
    --cc=rjw@rjwysocki.net \
    --cc=rostedt@goodmis.org \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.