Commit a9cdbe72 authored by Josh Poimboeuf's avatar Josh Poimboeuf Committed by Ingo Molnar
Browse files

x86/dumpstack: Fix partial register dumps 



The show_regs_safe() logic is wrong.  When there's an iret stack frame,
it prints the entire pt_regs -- most of which is random stack data --
instead of just the five registers at the end.

show_regs_safe() is also poorly named: the on_stack() checks aren't for
safety.  Rename the function to show_regs_if_on_stack() and add a
comment to explain why the checks are needed.

These issues were introduced with the "partial register dump" feature of
the following commit:

  b02fcf9b ("x86/unwinder: Handle stack overflows more gracefully")

That patch had gone through a few iterations of development, and the
above issues were artifacts from a previous iteration of the patch where
'regs' pointed directly to the iret frame rather than to the (partially
empty) pt_regs.

Tested-by: default avatarAlexander Tsoy <alexander@tsoy.me>
Signed-off-by: default avatarJosh Poimboeuf <jpoimboe@redhat.com>
Cc: Andy Lutomirski <luto@kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Toralf Förster <toralf.foerster@gmx.de>
Cc: stable@vger.kernel.org
Fixes: b02fcf9b ("x86/unwinder: Handle stack overflows more gracefully")
Link: http://lkml.kernel.org/r/5b05b8b344f59db2d3d50dbdeba92d60f2304c54.1514736742.git.jpoimboe@redhat.com


Signed-off-by: default avatarIngo Molnar <mingo@kernel.org>
parent 52994c25

arch/x86/include/asm/unwind.h

+13 −4
Original line number Diff line number Diff line
@@ -56,18 +56,27 @@ void unwind_start(struct unwind_state *state, struct task_struct *task, 


#if defined(CONFIG_UNWINDER_ORC) || defined(CONFIG_UNWINDER_FRAME_POINTER)

/*

 * WARNING: The entire pt_regs may not be safe to dereference.  In some cases,

 * only the iret frame registers are accessible.  Use with caution!

 * If 'partial' returns true, only the iret frame registers are valid.

 */

static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state)

static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state,

						    bool *partial)

{

	if (unwind_done(state))

		return NULL;



	if (partial) {

#ifdef CONFIG_UNWINDER_ORC

		*partial = !state->full_regs;

#else

		*partial = false;

#endif

	}



	return state->regs;

}

#else

static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state)

static inline struct pt_regs *unwind_get_entry_regs(struct unwind_state *state,

						    bool *partial)

{

	return NULL;

}

arch/x86/kernel/dumpstack.c

+20 −8
Original line number Diff line number Diff line
@@ -76,11 +76,22 @@ void show_iret_regs(struct pt_regs *regs) 
		regs->sp, regs->flags);

}



static void show_regs_safe(struct stack_info *info, struct pt_regs *regs)

static void show_regs_if_on_stack(struct stack_info *info, struct pt_regs *regs,

				  bool partial)

{

	if (on_stack(info, regs, sizeof(*regs)))

	/*

	 * These on_stack() checks aren't strictly necessary: the unwind code

	 * has already validated the 'regs' pointer.  The checks are done for

	 * ordering reasons: if the registers are on the next stack, we don't

	 * want to print them out yet.  Otherwise they'll be shown as part of

	 * the wrong stack.  Later, when show_trace_log_lvl() switches to the

	 * next stack, this function will be called again with the same regs so

	 * they can be printed in the right context.

	 */

	if (!partial && on_stack(info, regs, sizeof(*regs))) {

		__show_regs(regs, 0);

	else if (on_stack(info, (void *)regs + IRET_FRAME_OFFSET,



	} else if (partial && on_stack(info, (void *)regs + IRET_FRAME_OFFSET,

				       IRET_FRAME_SIZE)) {

		/*

		 * When an interrupt or exception occurs in entry code, the
@@ -98,6 +109,7 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs, 
	struct stack_info stack_info = {0};

	unsigned long visit_mask = 0;

	int graph_idx = 0;

	bool partial;



	printk("%sCall Trace:\n", log_lvl);
@@ -140,7 +152,7 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs, 
			printk("%s <%s>\n", log_lvl, stack_name);



		if (regs)

			show_regs_safe(&stack_info, regs);

			show_regs_if_on_stack(&stack_info, regs, partial);



		/*

		 * Scan the stack, printing any text addresses we find.  At the
@@ -164,7 +176,7 @@ void show_trace_log_lvl(struct task_struct *task, struct pt_regs *regs, 


			/*

			 * Don't print regs->ip again if it was already printed

			 * by show_regs_safe() below.

			 * by show_regs_if_on_stack().

			 */

			if (regs && stack == &regs->ip)

				goto next;
@@ -199,9 +211,9 @@ next: 
			unwind_next_frame(&state);



			/* if the frame has entry regs, print them */

			regs = unwind_get_entry_regs(&state);

			regs = unwind_get_entry_regs(&state, &partial);

			if (regs)

				show_regs_safe(&stack_info, regs);

				show_regs_if_on_stack(&stack_info, regs, partial);

		}



		if (stack_name)

arch/x86/kernel/stacktrace.c

+1 −1
Original line number Diff line number Diff line
@@ -98,7 +98,7 @@ static int __save_stack_trace_reliable(struct stack_trace *trace, 
	for (unwind_start(&state, task, NULL, NULL); !unwind_done(&state);

	     unwind_next_frame(&state)) {



		regs = unwind_get_entry_regs(&state);

		regs = unwind_get_entry_regs(&state, NULL);

		if (regs) {

			/*

			 * Kernel mode registers on the stack indicate an

CRA Git | Maintained and supported by SUSTech CRA and CCSE