Re: [PATCH V4] notifier/panic: Introduce panic_notifier_filter

[Guilherme G. Piccoli]

Hi Petr, thanks for the great response, and for CCing more (potentially)
interested parties! Some comments inline below; also, I'm CCing Michael
Kelley as well.


On 20/01/2022 12:14, Petr Mladek wrote:
> Adding some more people into Cc. Some modified the logic in the past.
> Some are familiar with some interesting areas where the panic
> notfiers are used.
> 
> On Sat 2022-01-08 12:34:51, Guilherme G. Piccoli wrote:
>> [...]
>> There are some cases though in which kdump users might want to
>> allow panic notifier callbacks to execute _before_ the kexec to
>> the crash kernel, for a variety of reasons - for example, users
>> may think kexec is very prone to fail and want to give a chance
>> to kmsg dumpers to run (and save logs using pstore),
> 
> Yes, this seems to be original intention for the
> "crash_kexec_post_notifiers" option, see the commit
> f06e5153f4ae2e2f3b0300f ("kernel/panic.c: add
> "crash_kexec_post_notifiers" option for kdump after panic_notifiers")
> 
>> some panic notifier is required to properly quiesce some hardware
>> that must be used to the crash kernel.
> 
> Do you have any example, please? The above mentioned commit
> says "crash_kexec_post_notifiers" actually increases risk
> of kdump failure.
> 
> Note that kmsg_dump() is called after the notifiers only because
> some are printing more information, see the commit
> 6723734cdff15211bb78a ("panic: call panic handlers before kmsg_dump").
> They might still increase the change that kmsg_dump() will never
> be called.
> 

Sure! I guess Michael Kelley's response here is the perfect example:
https://lore.kernel.org/lkml/MWHPR21MB1593A32A3433F5F262796FCFD75B9@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/

In my understanding, he is referring the function hyperv_panic_event().
But I also found another 2 examples in a quick look: bcm_vk_on_panic()
and brcmstb_pm_panic_notify().


>> [...]
>> So, this patch aims to ease this decision: we hereby introduce a filter
>> for the panic notifier list, in which users may select specifically
>> which callbacks they wish to run, allowing a safer kdump. The allowlist
>> should be provided using the parameter "panic_notifier_filter=a,b,..."
>> where a, b are valid callback names. Invalid symbols are discarded.
> 
> I am afraid that this is almost unusable solution:
> 
>    + requires deep knowledge of what each notifier does
>    + might need debugging what notifier causes problems
>    + the list might need to be updated when new notifiers are added
>    + function names are implementation detail and might change
>    + requires kallsyms
> 
> 
> It is only workaround for a real problem. The problem is that
> "panic_notifier_list" is used for many purposes that break
> each other.
> 
> I checked some notifiers and found few groups:
> 
>    + disable watchdogs:
>       + hung_task_panic()
>       + rcu_panic()
> 
>    + dump information:
>       + kernel_offset_notifier()
>       + trace_panic_handler()     (duplicate of panic_print=0x10)
> 
>    + inform hypervisor
>       + xen_panic_event()
>       + pvpanic_panic_notify()
>       + hyperv_panic_event()
> 
>    + misc cleanup / flush / blinking
>       + panic_event()   in ipmi_msghandler.c
>       + panic_happened()   in heartbeat.c
>       + led_trigger_panic_notifier()
> 
> 
> IMHO, the right solution is to split the callbacks into 2 or more
> notifier list. Then we might rework panic() to do:
> 
> void panic(void)
> {
> 	[...]
> 
> 	/* stop watchdogs + extra info */
> 	atomic_notifier_call_chain(&panic_disable_watchdogs_notifier_list, 0, buf);
> 	atomic_notifier_call_chain(&panic_info_notifier_list, 0, buf);
> 	panic_print_sys_info();
> 
> 	/* crash_kexec + kmsg_dump in configurable order */
> 	if (!_crash_kexec_post_kmsg_dump) {
> 		__crash_kexec(NULL);
> 		smp_send_stop();
> 	} else {
> 		crash_smp_send_stop();
> 	}
> 
> 	kmsg_dump();
> 	if (_crash_kexec_post_kmsg_dump)
> 		__crash_kexec(NULL);
> 
> 	/* infinite loop or reboot */
> 	atomic_notifier_call_chain(&panic_hypervisor_notifier_list, 0, buf);
> 	atomic_notifier_call_chain(&panic_rest_notifier_list, 0, buf);
> 
> 	console_flush_on_panic(CONSOLE_FLUSH_PENDING);
> [...] 
> Two notifier lists might be enough in the above scenario. I would call
> them:
> 
> 	panic_pre_dump_notifier_list
> 	panic_post_dump_notifier_list
> 
> 
> It is a real solution that will help everyone. It is more complicated now
> but it will makes things much easier in the long term. And it might be done
> step by step:
> 
>      1. introduce the two notifier lists
>      2. convert all users: one by one
>      3. remove the original notifier list when there is no user

That's a great idea! I'm into it, if we have a consensus. The thing that
scares me most here is that this is a big change and consumes time to
implement - I'd not risk such time if somebody is really against that.
So, let's see more opinions, maybe the kdump maintainers have good input.

Also, I'd be interested in still keeping a filter for the pre_dump list,
could be a blacklist by function name for example; since the post_dump
is conditioned to "crash_kexec_post_notifiers" and most of information
output will be in the first notifier, I don't see a strong reason
anymore for filtering the second notifier.

Cheers,


Guilherme