[PATCH][REGRESSION] panic: fix stack dump print on direct call to panic()

[PATCH][REGRESSION] panic: fix stack dump print on direct call to panic()

[Jason Wessel]

Commit 6e6f0a1f0 (panic: don't print redundant backtraces on oops)
causes a regression where no stack trace will be printed at all for
the case where kernel code calls panic() directly while not processing
an oops, and of course there are 100's of instances of this type of
call.

The original commit executed the check (!oops_in_progress), but this
will always be false because just before the dump_stack() there is a
call to bust_spinlocks(1), which does the following:

  void __attribute__((weak)) bust_spinlocks(int yes)
  {
	if (yes) {
		++oops_in_progress;

The proper way to resolve the problem that original commit tried to
solve is to avoid printing a stack dump from panic() when the either
of the following conditions is true:

  1) TAINT_DIE has been set (this is done by oops_end())
     This indicates and oops has already been printed.
  2) oops_in_progress > 1
     This guards against the rare case where panic() is invoked
     a second time, or in between oops_begin() and oops_end()

Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: stable@vger.kernel.org # >= 3.3
Signed-off-by: Jason Wessel <jason.wessel@windriver.com>
---
 kernel/panic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 80aed44..8ed89a1 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -97,7 +97,7 @@ void panic(const char *fmt, ...)
 	/*
 	 * Avoid nested stack-dumping if a panic occurs during oops processing
 	 */
-	if (!oops_in_progress)
+	if (!test_taint(TAINT_DIE) && oops_in_progress <= 1)
 		dump_stack();
 #endif
 
-- 
1.7.9.4

Re: [PATCH][REGRESSION] panic: fix stack dump print on direct call to panic()

[Andi Kleen]

> The proper way to resolve the problem that original commit tried to
> solve is to avoid printing a stack dump from panic() when the either
> of the following conditions is true:
>
>    1) TAINT_DIE has been set (this is done by oops_end())
>       This indicates and oops has already been printed.
>    2) oops_in_progress>  1
>       This guards against the rare case where panic() is invoked
>       a second time, or in between oops_begin() and oops_end()

Oops. I guess can just revert it for now. Thanks for catching, Jason.

The proper solution is probably some variant of
http://git.kernel.org/?p=linux/kernel/git/ak/linux-mce-2.6.git;a=shortlog;h=refs/heads/mce/xpanic

Let the caller pass in the proper action instead of all these hacks.

-Andi

Re: [PATCH][REGRESSION] panic: fix stack dump print on direct call to panic()

[Jason Wessel]

On 04/09/2012 01:34 PM, Andi Kleen wrote:
>> The proper way to resolve the problem that original commit tried to
>> solve is to avoid printing a stack dump from panic() when the either
>> of the following conditions is true:
>>
>>    1) TAINT_DIE has been set (this is done by oops_end())
>>       This indicates and oops has already been printed.
>>    2) oops_in_progress>  1
>>       This guards against the rare case where panic() is invoked
>>       a second time, or in between oops_begin() and oops_end()
> Oops. I guess can just revert it for now. Thanks for catching, Jason.
>
> The proper solution is probably some variant of
> http://git.kernel.org/?p=linux/kernel/git/ak/linux-mce-2.6.git;a=shortlog;h=refs/heads/mce/xpanic
>
> Let the caller pass in the proper action instead of all these hacks.

That is an interesting patch series, but I am not sure I agree about the caller propagating a flag to control what you see in panic.  I intentionally set CONFIG_DEBUG_VERBOSE=y for a reason, I want a stack trace if and when panic(), oops, BUG, etc... is ever called.  This might be a personal preference, but I do not wish to be searching on a string, to find out where the kernel execution terminated.  I want to see stack traces all the time, with the exception of the cases you pointed out in the original patch.

I had taken the time to instrument and the edge cases for the oops and panic() nesting.  I would agree that it is not useful to print further traces if the kernel has already processed an oops.  I also checked non-x86 archs to see that the original behavior was intact with the new version of the patch.  My opinion is that checking for the panic recursion is a necessary evil here, we just have to agree about the right approach.  :-)

Cheers,
Jason.

Re: [PATCH][REGRESSION] panic: fix stack dump print on direct call to panic()

[Andi Kleen]

> That is an interesting patch series, but I am not sure I agree about the caller propagating a flag to control what you see in panic.  I intentionally set CONFIG_DEBUG_VERBOSE=y for a reason, I want a stack trace if and when panic(), oops, BUG, etc... is ever called.  This might be a personal preference, but I do not wish to be searching on a string, to find out where the kernel execution terminated.  I want to see stack traces all the time, with the exception of the cases you pointed out in the original patch.

The idea was that callers who are calling panic not due to a programming
error (like no root, hardware error etc.) do not print the backtrace.
And only "assert like" panics would. 

This makes the screen less cluttered for user or hardware induced
panics.

Besides that it does a couple of related things, like set proper
timeouts.

-Andi

Re: [PATCH][REGRESSION] panic: fix stack dump print on direct call to panic()

[Andrew Morton]

On Mon, 09 Apr 2012 11:34:48 -0700
Andi Kleen <ak@linux.intel.com> wrote:

> > The proper way to resolve the problem that original commit tried to
> > solve is to avoid printing a stack dump from panic() when the either
> > of the following conditions is true:
> >
> >    1) TAINT_DIE has been set (this is done by oops_end())
> >       This indicates and oops has already been printed.
> >    2) oops_in_progress>  1
> >       This guards against the rare case where panic() is invoked
> >       a second time, or in between oops_begin() and oops_end()
> 
> Oops. I guess can just revert it for now. Thanks for catching, Jason.

You're suggesting a revert of 6e6f0a1f0, but Jason is suggesting that
it be repaired.  Confused.

Re: [PATCH][REGRESSION] panic: fix stack dump print on direct call to panic()

[Andi Kleen]

On Tue, Apr 10, 2012 at 12:19:49PM -0700, Andrew Morton wrote:
> On Mon, 09 Apr 2012 11:34:48 -0700
> Andi Kleen <ak@linux.intel.com> wrote:
> 
> > > The proper way to resolve the problem that original commit tried to
> > > solve is to avoid printing a stack dump from panic() when the either
> > > of the following conditions is true:
> > >
> > >    1) TAINT_DIE has been set (this is done by oops_end())
> > >       This indicates and oops has already been printed.
> > >    2) oops_in_progress>  1
> > >       This guards against the rare case where panic() is invoked
> > >       a second time, or in between oops_begin() and oops_end()
> > 
> > Oops. I guess can just revert it for now. Thanks for catching, Jason.
> 
> You're suggesting a revert of 6e6f0a1f0, but Jason is suggesting that
> it be repaired.  Confused.

Repairing is fine too for me.

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only