Re: Re: Re: [PATCH v2] kernel/panic/kexec: fix "crash_kexec_post_notifiers" option issue in oops path

[Hidehiro Kawai]

Hi all,

What is the current status of this bug fix patch?
I think it's OK if resending Hatayama-san's patch with Ingo's.

Thanks,

(2015/03/25 2:04), Vivek Goyal wrote:
> On Tue, Mar 24, 2015 at 05:18:14PM +0100, Ingo Molnar wrote:
>>
>> * Vivek Goyal <vgoyal@xxxxxxxxxx> wrote:
>>
>>>> Yet the actual bug is in that commit, 'crash_kexec_post_notifiers' 
>>>> was clearly not a no-op in the default case, against expectations.
>>>
>>> Hi Ingo,
>>>
>>> I did a quick test and in default case crash_kexec() runs before 
>>> panic notifiers. So it does look like crash_kexec_post_notifiers is 
>>> a no-op in default case.
>>>
>>> What am I missing.
>>
>> Well, look at f06e5153f4ae:
>>
>> diff --git a/kernel/panic.c b/kernel/panic.c
>> index d02fa9fef46a..62e16cef9cc2 100644
>> --- a/kernel/panic.c
>> +++ b/kernel/panic.c
>> @@ -32,6 +32,7 @@ static unsigned long tainted_mask;
>>  static int pause_on_oops;
>>  static int pause_on_oops_flag;
>>  static DEFINE_SPINLOCK(pause_on_oops_lock);
>> +static bool crash_kexec_post_notifiers;
>>  
>>  int panic_timeout = CONFIG_PANIC_TIMEOUT;
>>  EXPORT_SYMBOL_GPL(panic_timeout);
>> @@ -112,9 +113,11 @@ void panic(const char *fmt, ...)
>>  	/*
>>  	 * If we have crashed and we have a crash kernel loaded let it handle
>>  	 * everything else.
>> -	 * Do we want to call this before we try to display a message?
>> +	 * If we want to run this after calling panic_notifiers, pass
>> +	 * the "crash_kexec_post_notifiers" option to the kernel.
>>  	 */
>> -	crash_kexec(NULL);
>> +	if (!crash_kexec_post_notifiers)
>> +		crash_kexec(NULL);
>>  
>>  	/*
>>  	 * Note smp_send_stop is the usual smp shutdown function, which
>> @@ -131,6 +134,15 @@ void panic(const char *fmt, ...)
>>  
>>  	kmsg_dump(KMSG_DUMP_PANIC);
>>  
>> +	/*
>> +	 * If you doubt kdump always works fine in any situation,
>> +	 * "crash_kexec_post_notifiers" offers you a chance to run
>> +	 * panic_notifiers and dumping kmsg before kdump.
>> +	 * Note: since some panic_notifiers can make crashed kernel
>> +	 * more unstable, it can increase risks of the kdump failure too.
>> +	 */
>> +	crash_kexec(NULL);
>> +
>>  	bust_spinlocks(0);
>>  
>>  	if (!panic_blink)
>>
>>
>> Without knowing what crash_kexec() does, the patch looks buggy: it 
>> should preserve the old behavior by default, yet it will now execute a 
>> second crash_kexec() after the kmsg_dump() line.
>>
>> So the invariant change would have been to do:
>>
>> -	crash_kexec(NULL);
>> +	if (!crash_kexec_post_notifiers)
>> +		crash_kexec(NULL);
>>
>> ...
>>
>> +	if (crash_kexec_post_notifiers)
>> +		crash_kexec(NULL);
>>
>> Which in the !crash_kexec_post_notifiers flag case reduces to:
>>
>> 	crash_kexec();
>>
>> 	...
>>
>> 	/* NOP */
>>
>> I.e. to exactly what the kernel was doing without the patch 
>> originally.
>>
>> Which is what my patch does. Nothing more, nothing less.
> 
> Ok, I got it what you mean.
> 
> crash_kexec() does not return if a kdump kernel is loaded. If kdump
> kernel is not loaded, then crash_kexec() returns without doing anything.
> 
> I think that explains why not making second call to crash_kexec() under
> if, did not create problems. In first case it will never be called and
> in second case, it will do nothing and simply return back.
> 
> But anyway, we need your patch as that's right thing to do.
> 
> Thanks
> Vivek
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
> 


-- 
Hidehiro Kawai
Hitachi, Ltd. Research & Development Group


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/