The kernel notifier infrastructure allows function callbacks to be
added in multiple lists, which are then called in the proper time,
like in a reboot or panic event. The panic_notifier_list specifically
contains the callbacks that are executed during a panic event. As any
other notifier list, the panic one has no filtering and all functions
previously registered are executed.

The kdump infrastructure, on the other hand, enables users to set
a crash kernel that is kexec'ed in a panic event, and vmcore/logs
are collected in such crash kernel. When kdump is set, by default
the panic notifiers are ignored - the kexec jumps to the crash kernel
before the list is checked and callbacks executed.

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), or maybe
some panic notifier is required to properly quiesce some hardware
that must be used to the crash kernel. For these cases, we have
the kernel parameter "crash_kexec_post_notifiers".

But there's a problem: currently it's an "all-or-nothing" situation,
the kdump user choice is either to execute all panic notifiers or
none of them. Given that panic notifiers may increase the risk of a
kdump failure, this is a tough decision and may affect the debug of
hard to reproduce bugs, if for some reason the user choice is to
enable panic notifiers, but kdump then fails.

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.

Currently up to 16 symbols may be passed in this list, we consider
that this numbers allows enough flexibility (and no matter what
architecture is used, at most 30 panic callbacks are registered).
In an experiment using a qemu x86 virtual machine, by default only
six callbacks are registered in the panic notifier list.
Once a valid callback name is provided in the list, such function
is allowed to be registered/unregistered in the panic_notifier_list;
all other panic callbacks are ignored. Notice that this filter is
only for the panic notifiers and has no effect in the other notifiers.

Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>
---



V4:

* Add some more clean-up suggestion from Andy (thanks).


V3:

* Implemented Alan's suggestion (thanks!), simplifying the check code
in the notifiers register/unregister functions. Notice that the
suggestion was missing a negative in the check function, I even
renamed it now, to be more clear:
s/is_panic_notifier_filtered/should_register_panic_notifier

The condition is !(A && B && C), and C is the check function, that
returns true when a symbol *is found* in the notifier filter; hence
we need to invert that here, as you can see in the code.


* Implemented Andy's suggestion (thanks!), to reduce the code in
the parameter parsing loop. Notice that strsep() modifies the buffer,
so not sure if it was a typo but the correct code here is:

single_param = strsep(&full_param_buffer, ",");


* "Bumped" the log output of the parsing function: users should be
warned in the errors (invalid symbol or exceeded entries) and
informed (pr_info) in case the parsing succeeded - I think pr_debug
was useless there.

Cheers,

Guilherme



 .../admin-guide/kernel-parameters.txt         | 14 +++++-
 include/linux/panic_notifier.h                | 10 +++++
 kernel/notifier.c                             | 44 +++++++++++++++++--
 kernel/panic.c                                | 39 ++++++++++++++++
 4 files changed, 102 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
index 2fba82431efb..2dc4e98823ae 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -3727,13 +3727,25 @@
 	panic_on_warn	panic() instead of WARN().  Useful to cause kdump
 			on a WARN().
 
+	panic_notifier_filter=[function-list]
+			Limit the functions registered by the panic notifier
+			infrastructure. This allowlist is composed by function
+			names, comma separated (invalid symbols are filtered
+			out). Such functionality is useful for kdump users
+			that set "crash_kexec_post_notifiers" in order to
+			execute	panic notifiers, but at the same time wish to
+			have just a subset of notifiers, not all of them. The
+			list of functions is limited to 16 entries currently.
+
 	crash_kexec_post_notifiers
 			Run kdump after running panic-notifiers and dumping
 			kmsg. This only for the users who doubt kdump always
 			succeeds in any situation.
 			Note that this also increases risks of kdump failure,
 			because some panic notifiers can make the crashed
-			kernel more unstable.
+			kernel more unstable. See the "panic_notifier_filter"
+			parameter to have more control of which notifiers to
+			execute.
 
 	parkbd.port=	[HW] Parallel port number the keyboard adapter is
 			connected to, default is 0.
diff --git a/include/linux/panic_notifier.h b/include/linux/panic_notifier.h
index 41e32483d7a7..9a96753e96d8 100644
--- a/include/linux/panic_notifier.h
+++ b/include/linux/panic_notifier.h
@@ -5,6 +5,16 @@
 #include <linux/notifier.h>
 #include <linux/types.h>
 
+/*
+ * The panic notifier filter infrastructure - each array element holds a
+ * function address, to be checked against panic_notifier register/unregister
+ * operations; these functions are allowed to be registered in the panic
+ * notifier list. This setting is useful for kdump, since users may want
+ * some panic notifiers to execute, but not all of them.
+ */
+extern unsigned long panic_nf_functions[];
+extern int panic_nf_count;
+
 extern struct atomic_notifier_head panic_notifier_list;
 
 extern bool crash_kexec_post_notifiers;
diff --git a/kernel/notifier.c b/kernel/notifier.c
index b8251dc0bc0f..4fc450bbf677 100644
--- a/kernel/notifier.c
+++ b/kernel/notifier.c
@@ -1,4 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0-only
+#include <linux/panic_notifier.h>
 #include <linux/kdebug.h>
 #include <linux/kprobes.h>
 #include <linux/export.h>
@@ -127,12 +128,34 @@ static int notifier_call_chain_robust(struct notifier_block **nl,
  *	use a spinlock, and call_chain is synchronized by RCU (no locks).
  */
 
+/*
+ * The following helper is part of the panic notifier filter infrastructure;
+ * users can filter what functions they wish to allow being registered in the
+ * notifier system, restricted to the panic notifier. This is useful for kdump
+ * for example, when some notifiers are relevant but running all of them imposes
+ * risks to the kdump kernel reliability.
+ */
+static bool should_register_panic_notifier(struct notifier_block *n)
+{
+	int i;
+
+	for (i = 0; i < panic_nf_count; i++) {
+		if ((unsigned long)(n->notifier_call) == panic_nf_functions[i])
+			return true;
+	}
+
+	return false;
+}
+
 /**
  *	atomic_notifier_chain_register - Add notifier to an atomic notifier chain
  *	@nh: Pointer to head of the atomic notifier chain
  *	@n: New entry in notifier chain
  *
  *	Adds a notifier to an atomic notifier chain.
+ *	If "panic_notifier_filter" is provided, we hereby filter the
+ *	panic_notifier_list and only allow registering the functions
+ *	that are present in the filter.
  *
  *	Currently always returns zero.
  */
@@ -140,10 +163,15 @@ int atomic_notifier_chain_register(struct atomic_notifier_head *nh,
 		struct notifier_block *n)
 {
 	unsigned long flags;
-	int ret;
+	int ret = 0;
 
 	spin_lock_irqsave(&nh->lock, flags);
-	ret = notifier_chain_register(&nh->head, n);
+
+	if (!(nh == &panic_notifier_list &&
+	     (panic_nf_count > 0) &&
+	     !should_register_panic_notifier(n)))
+		ret = notifier_chain_register(&nh->head, n);
+
 	spin_unlock_irqrestore(&nh->lock, flags);
 	return ret;
 }
@@ -155,6 +183,9 @@ EXPORT_SYMBOL_GPL(atomic_notifier_chain_register);
  *	@n: Entry to remove from notifier chain
  *
  *	Removes a notifier from an atomic notifier chain.
+ *	If "panic_notifier_filter" is provided, we hereby filter the
+ *	panic_notifier_list and only allow unregistering the functions
+ *	that are present in the filter.
  *
  *	Returns zero on success or %-ENOENT on failure.
  */
@@ -162,10 +193,15 @@ int atomic_notifier_chain_unregister(struct atomic_notifier_head *nh,
 		struct notifier_block *n)
 {
 	unsigned long flags;
-	int ret;
+	int ret = 0;
 
 	spin_lock_irqsave(&nh->lock, flags);
-	ret = notifier_chain_unregister(&nh->head, n);
+
+	if (!(nh == &panic_notifier_list &&
+	     (panic_nf_count > 0) &&
+	     !should_register_panic_notifier(n)))
+		ret = notifier_chain_unregister(&nh->head, n);
+
 	spin_unlock_irqrestore(&nh->lock, flags);
 	synchronize_rcu();
 	return ret;
diff --git a/kernel/panic.c b/kernel/panic.c
index cefd7d82366f..a06fcc4b1d6e 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -31,6 +31,7 @@
 #include <linux/console.h>
 #include <linux/bug.h>
 #include <linux/ratelimit.h>
+#include <linux/kallsyms.h>
 #include <linux/debugfs.h>
 #include <asm/sections.h>
 
@@ -67,6 +68,16 @@ EXPORT_SYMBOL_GPL(panic_timeout);
 #define PANIC_PRINT_ALL_PRINTK_MSG	0x00000020
 unsigned long panic_print;
 
+/*
+ * Kernel has currently < 30 panic handlers no matter the arch,
+ * based on some code counting; so 16 items seems a good amount;
+ * users that are filtering panic notifiers shouldn't add all
+ * of them in theory, that doesn't make sense...
+ */
+#define	PANIC_NF_MAX	16
+unsigned long panic_nf_functions[PANIC_NF_MAX];
+int panic_nf_count;
+
 ATOMIC_NOTIFIER_HEAD(panic_notifier_list);
 
 EXPORT_SYMBOL(panic_notifier_list);
@@ -146,6 +157,34 @@ void nmi_panic(struct pt_regs *regs, const char *msg)
 }
 EXPORT_SYMBOL(nmi_panic);
 
+static int __init panic_notifier_filter_setup(char *buf)
+{
+	char *func;
+	unsigned long addr;
+
+	while ((func = strsep(&buf, ","))) {
+		addr = kallsyms_lookup_name(func);
+		if (!addr) {
+			pr_warn("panic_notifier_filter: invalid symbol %s\n", func);
+			continue;
+		}
+
+		if (panic_nf_count < PANIC_NF_MAX) {
+			panic_nf_functions[panic_nf_count] = addr;
+			panic_nf_count++;
+			pr_info("panic_notifier_filter: added symbol %s\n", func);
+		} else {
+			pr_warn("panic_notifier_filter: exceeded maximum notifiers (%d), aborting\n",
+				PANIC_NF_MAX);
+			panic_nf_count = 0;
+			break;
+		}
+	}
+
+	return 0;
+}
+early_param("panic_notifier_filter", panic_notifier_filter_setup);
+
 static void panic_print_sys_info(void)
 {
 	if (panic_print & PANIC_PRINT_ALL_PRINTK_MSG)
-- 
2.34.1

Hey folks, sorry for the ping.
But is there any extra reviews? All comments are much appreciated!

Dave, what do you think about the patch? I remember we talked about it
in [0], seems you considered that a good idea right?

Thanks in advance,


Guilherme


[0]
https://lore.kernel.org/lkml/Yckaz79zg5HdEgcH@dhcp-128-65.nay.redhat.com/

On 01/08/22 at 12:34pm, Guilherme G. Piccoli wrote:
...... 
> 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.
> 
> Currently up to 16 symbols may be passed in this list, we consider
> that this numbers allows enough flexibility (and no matter what
> architecture is used, at most 30 panic callbacks are registered).
> In an experiment using a qemu x86 virtual machine, by default only
> six callbacks are registered in the panic notifier list.
> Once a valid callback name is provided in the list, such function
> is allowed to be registered/unregistered in the panic_notifier_list;
> all other panic callbacks are ignored. Notice that this filter is
> only for the panic notifiers and has no effect in the other notifiers.
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com>

This patch looks good to me, thx.

Acked-by: Baoquan He <bhe@redhat.com>

On 16/01/2022 10:11, Baoquan He wrote:
> [...]
> This patch looks good to me, thx.
> 
> Acked-by: Baoquan He <bhe@redhat.com>
> 

Thanks a lot Baoquan He !

> @@ -146,6 +157,34 @@ void nmi_panic(struct pt_regs *regs, const char *msg)
>  }
>  EXPORT_SYMBOL(nmi_panic);
> 
> +static int __init panic_notifier_filter_setup(char *buf)
> +{
> +       char *func;
> +       unsigned long addr;
> +
> +       while ((func = strsep(&buf, ","))) {
> +               addr = kallsyms_lookup_name(func);
> +               if (!addr) {
> +                       pr_warn("panic_notifier_filter: invalid symbol %s\n", func);
> +                       continue;
> +               }

Could you remove this check?

panic_notifier_list is exported to kernel modules and this check
prevents such users from using this feature.

Thanks.
HATAYAMA, Daisuke

On 25/01/2022 08:50, d.hatayama@fujitsu.com wrote:
>> +       while ((func = strsep(&buf, ","))) {
>> +               addr = kallsyms_lookup_name(func);
>> +               if (!addr) {
>> +                       pr_warn("panic_notifier_filter: invalid symbol %s\n", func);
>> +                       continue;
>> +               }
> 
> Could you remove this check?
> 
> panic_notifier_list is exported to kernel modules and this check
> prevents such users from using this feature.
> 
> Thanks.
> HATAYAMA, Daisuke


Hi, thanks for the review. First of all, notice that it's very likely
this patch isn't gonna get merged this way, we are considering a
refactor that included 2 panic notifiers: one a bit earlier (pre_dump),
that includes functions less risky, as watchdog unloaders, kernel offset
dump, etc, and the second panic notifier (post_dump) will keep the
majority of callbacks, and can be conditionally executed on kdump
through the usage of "crash_kexec_post_notifiers".

Anyway, I'm curious with your code review - how can we use this filter
with modules, if the filter setup is invoked as early_param(), before
modules load? In that case, module functions won't have a valid address,
correct? So, in that moment, this lookup fails, we cannot record an
unloaded module address in such list. Please, correct me if I'm wrong.

Cheers,


Guilherme

> Hi, thanks for the review. First of all, notice that it's very likely
> this patch isn't gonna get merged this way, we are considering a
> refactor that included 2 panic notifiers: one a bit earlier (pre_dump),
> that includes functions less risky, as watchdog unloaders, kernel offset
> dump, etc, and the second panic notifier (post_dump) will keep the
> majority of callbacks, and can be conditionally executed on kdump
> through the usage of "crash_kexec_post_notifiers".

But the pre_dump cannot avoid calling multiple unnecessary handlers, right?
It's more risky than the previous idea...

> Anyway, I'm curious with your code review - how can we use this filter
> with modules, if the filter setup is invoked as early_param(), before
> modules load? In that case, module functions won't have a valid address,
> correct? So, in that moment, this lookup fails, we cannot record an
> unloaded module address in such list. Please, correct me if I'm wrong.

For example, how about simply maintaining function symbol names in the list
as string, not address.

Thanks.
HATAYAMA, Daisuke

On 25/01/2022 10:06, d.hatayama@fujitsu.com wrote:
> 
> But the pre_dump cannot avoid calling multiple unnecessary handlers, right?
> It's more risky than the previous idea...
> 

I think we could have 2 kernel parameters then:

crash_kernel_disable_pre_notitifers (of course we can think in some
better name here heh)

crash_kernel_enable_post_notifiers (which is the same as the current
"crash_kernel_post_notifiers", we can keep it)

The point being (if I understand correctly): some callbacks are really
simple and don't introduce risks for kdump, like the RCU; a bunch of
them just set one variable. Those could be enable by default, before the
kdump.

The majority would fit in the 2nd group, meaning they are not enabled by
default, requiring some parameter for that.

Petr, let me know if that makes sense and is aligned with your suggestion.


> For example, how about simply maintaining function symbol names in the list
> as string, not address.
> 

I considered that before, it was my first idea but it's not great due to
memory allocation. We'd need to use memblock to allocate a struct to
hold function names, and the comparison on register time is slower, I
guess... so it's much easier to pre-allocate some handlers and only
track the addresses of the function. I personally do not see much use in
this filter for module callbacks, but if that's a use case, we can think
on how to do that. But notice that the current implementation of the
filter wont hold if we end-up following the suggestions in this thread,
not sure even if we're gonna have a filter...

Cheers,


Guilherme

On Thu 2022-01-27 14:16:20, Guilherme G. Piccoli wrote:
> On 25/01/2022 10:06, d.hatayama@fujitsu.com wrote:
> > 
> > But the pre_dump cannot avoid calling multiple unnecessary handlers, right?
> > It's more risky than the previous idea...
> > 
> 
> I think we could have 2 kernel parameters then:
> 
> crash_kernel_disable_pre_notitifers (of course we can think in some
> better name here heh)
> 
> crash_kernel_enable_post_notifiers (which is the same as the current
> "crash_kernel_post_notifiers", we can keep it)
> 
> The point being (if I understand correctly): some callbacks are really
> simple and don't introduce risks for kdump, like the RCU; a bunch of
> them just set one variable. Those could be enable by default, before the
> kdump.
> 
> The majority would fit in the 2nd group, meaning they are not enabled by
> default, requiring some parameter for that.
> 
> Petr, let me know if that makes sense and is aligned with your suggestion.

First, I am sorry for the very long mail. But the problem is really
complicated. I did my best to describe it a clean way.

I have discussed these problems with a colleague and he had some good
points. And my view evolved even further.

There are two groups of people interested in panic() behavior:

1. Users wants to get as reliable as possible: kdump, kmsg_dump,
   console log, useful last message on screen, reboot, hypervisor
   notification.

   Different users have different priorities according to the use case.


2. Subsystem maintainers and developers that need to do something
   special in panic(). They have to deal with the user requirements
   and bug reports.

   Most operations in panic() have unclear results because the system
   is in unclear state. Maintainers and developers wants to make their
   life easier. They do not want to deal with problems caused by
   others. So that they want to disable others or run as early as
   possible.

   It is nicely visible. kdump maintainer is afraid of calling
   anything before kdump. Many people support the idea of filtering
   because it moves problems to the user side.


I see two basic problems here: ordering and reliability:

1. Ordering problems are partly solved by configuration and partly by
   definition. I mean that:

      + kdump, kmsg_dump, panic_print_sys_info() are optional
      + console output is the best effort; more risky in final flush
      + reboot, infinite loop are the very last step

   IMHO, the ordering should be pretty clear:

      + panic_print_sys_info(), kmsg_dump(), kdump(), console flush, reboot

   Why?

      + panic_print_sys_info(), kmsg_dump(), kdump() are all optional
	   and disabled by default
      + Users might want panic_print_sys_info() in kmsg_dump() and kdump()
      + Users might prefer kmsg_dump() over kdump()
      + kdump() is the last operation when enabled

   Where are panic notifiers in this picture?
   Where are CPUs stopped?


2. Reliability is the best effort and any subsystem should do its
   best.

   Users need to be aware (documentation, warning) that:

      + kmsg_dump() is less reliable when panic_print_sys_info() is enabled
      + kdump() is less reliable when panic_print_sys_info() and/or
	kmsg_dump() is enabled.

   Where are panic notifiers in this picture?
   How stopped CPUs affect reliability?


Regarding panic notifiers. They look like a problematic black box:

    + ordering against other operations is not clear
    + are not safe enough
    + various users require some and do not need others
    + some are too complex so that only few people know what
      they do


So far, we have two proposals how to handle panic notifiers:

1. Allow to filter them with parameter:

     + pros:
	+ it allows users to customize and work around problems

     + cons:
	+ ordering is still not clear

	+ user has to know what he does; note that sometimes only
	  few people know what their notifier does

	+ hard to use in the long term; callbacks names are
	  implementation detail; new notifiers are added

	+ lower motivation to solve problems; easy to wave them with
	  "just disable it when it does not work for you..."


2. Split notifiers into more lists:

    + pros:
	+ might solve ordering problems

	+ subsystem maintainers could find the proper and more safe
	  location


    + cons:
	+ subsystem maintainers tend to think that their problem is
	  the most important one; they will tend to do the operation
	  as early as possible; so that even dangerous operations
	  might be done early  => the original problem is still there

	+ it might not motivate developers enough to make the notifiers as
	  safe as possible

	+ some might still need to be optional; for example, it should
	  be possible to disable hypervisor notifier when it breaks
	  kdump


Regarding stopped CPUs, it looks like a good idea most of the time:

    + They should stop all tasks and reduce further damage of the
      system.

    + It should also reduce noise (messages) produced by other CPUs.

    + But a special handling is needed when it is done before crash
      dump.


Sigh, it looks really really complicated. We should be careful.

OK, the original problems are:

   + allow to call this order: panic_print_sys_info(), kmsg_dump(), kdump()
   + make it more safe with problematic notifiers


My opinion:

   + allow the desired ordering makes sense

   + something should be done with notifiers:

       + adding filer looks like a workaround that is not much
	 usable; it is not easy to use; it does not motivate people
	 fix problems so that is might make things worse in
	 the long term

       + splitting might make sense but it is not clear how

       + some notifiers make always sense before kmsg_dump;
	 some should be optional

   + we need a compromise to keep the panic() code sane and can't
     support all combinations


I think about the following solution:

    + split the notifiers into three lists:

	+ info: stop watchdogs, provide extra info
	+ hypervisor: poke hypervisor
	+ reboot: actions needed only when crash dump did not happen

    + allow to call hypervisor notifiers before or after kdump

    + stop CPUs before kdump when either hypervisor notifiers or
      kmsg_dump is enabled

Note that it still allows to call kdump as the first action when
hypervisor notifiers are called after kdump and no kmsg dumper
is registered.


void panic(void)
{
	[...]

	if (crash_kexec_post_hypervisor || panic_print || enabled_kmsg_dump()) {
		/*
		 * Stop CPUs when some extra action is required before
		 * crash dump. We will need architecture dependent extra
		 * works in addition to stopping other CPUs.
		 */
		 crash_smp_send_stop();
		 cpus_stopped = true;
	}

	if (crash_kexec_post_hypervisor) {
		  /* Tell hypervisor about the panic */
		  atomic_notifier_call_chain(&panic_hypervisor_notifier_list, 0, buf);
	}

	if (enabled_kmsg_dump) {
		  /*
		   * Print extra info by notifiers.
		   * Prevent rumors, for example, by stopping watchdogs.
		   */
		  atomic_notifier_call_chain(&panic_info_notifier_list, 0, buf);
	}

	/* Optional extra info */
	panic_printk_sys_info();

	/* No dumper by default */
	kmsg_dump();

	/* Used only when crash kernel loaded */
	__crash_kexec(NULL);

	if (!cpus_stopped) {
		/*
		 * Note smp_send_stop is the usual smp shutdown function, which
		 * unfortunately means it may not be hardened to work in a
		 * panic situation.
		 */
		smp_send_stop();
	}

	if (!crash_kexec_post_hypervisor) {
		  /* Tell hypervisor about the panic */
		  atomic_notifier_call_chain(&panic_hypervisor_notifier_list, 0, buf);
	}

	if (!enabled_kmsg_dump) {
		  /*
		   * Print extra info by notifiers.
		   * Prevent rumors, for example, by stopping watchdogs.
		   */
		  atomic_notifier_call_chain(&panic_info_notifier_list, 0, buf);
	}

	/*
	 * Help to reboot a safe way.
	 */
	atomic_notifier_call_chain(&panic_reboot_notifier_list, 0, buf);

	[...]
}

Any opinion?
Do the notifier list names make sense?

Best Regards,
Petr

PS: I have vacation the following week. I'll continue in the
    discussion when I am back.

On 28/01/2022 10:38, Petr Mladek wrote:
> [...] On Thu 2022-01-27 14:16:20, Guilherme G. Piccoli wrote:
> First, I am sorry for the very long mail. But the problem is really
> complicated. I did my best to describe it a clean way.
> 
> I have discussed these problems with a colleague and he had some good
> points. And my view evolved even further.

Thanks Petr for the very comprehensive and detailed email - this helps a
lot in shaping the future of panic notifier(s)!


> [...] 
> I think about the following solution:
> 
>     + split the notifiers into three lists:
> 
> 	+ info: stop watchdogs, provide extra info
> 	+ hypervisor: poke hypervisor
> 	+ reboot: actions needed only when crash dump did not happen
> 
>     + allow to call hypervisor notifiers before or after kdump
> 
>     + stop CPUs before kdump when either hypervisor notifiers or
>       kmsg_dump is enabled
> 
> Note that it still allows to call kdump as the first action when
> hypervisor notifiers are called after kdump and no kmsg dumper
> is registered.
> 
> 
> void panic(void)
> {
> 	[...]
> 
> 	if (crash_kexec_post_hypervisor || panic_print || enabled_kmsg_dump()) {
> 		/*
> 		 * Stop CPUs when some extra action is required before
> 		 * crash dump. We will need architecture dependent extra
> 		 * works in addition to stopping other CPUs.
> 		 */
> 		 crash_smp_send_stop();
> 		 cpus_stopped = true;
> 	}
> 
> 	if (crash_kexec_post_hypervisor) {
> 		  /* Tell hypervisor about the panic */
> 		  atomic_notifier_call_chain(&panic_hypervisor_notifier_list, 0, buf);
> 	}
> 
> 	if (enabled_kmsg_dump) {
> 		  /*
> 		   * Print extra info by notifiers.
> 		   * Prevent rumors, for example, by stopping watchdogs.
> 		   */
> 		  atomic_notifier_call_chain(&panic_info_notifier_list, 0, buf);
> 	}
> 
> 	/* Optional extra info */
> 	panic_printk_sys_info();
> 
> 	/* No dumper by default */
> 	kmsg_dump();
> 
> 	/* Used only when crash kernel loaded */
> 	__crash_kexec(NULL);
> 
> 	if (!cpus_stopped) {
> 		/*
> 		 * Note smp_send_stop is the usual smp shutdown function, which
> 		 * unfortunately means it may not be hardened to work in a
> 		 * panic situation.
> 		 */
> 		smp_send_stop();
> 	}
> 
> 	if (!crash_kexec_post_hypervisor) {
> 		  /* Tell hypervisor about the panic */
> 		  atomic_notifier_call_chain(&panic_hypervisor_notifier_list, 0, buf);
> 	}
> 
> 	if (!enabled_kmsg_dump) {
> 		  /*
> 		   * Print extra info by notifiers.
> 		   * Prevent rumors, for example, by stopping watchdogs.
> 		   */
> 		  atomic_notifier_call_chain(&panic_info_notifier_list, 0, buf);
> 	}
> 
> 	/*
> 	 * Help to reboot a safe way.
> 	 */
> 	atomic_notifier_call_chain(&panic_reboot_notifier_list, 0, buf);
> 
> 	[...]
> }
> 
> Any opinion?
> Do the notifier list names make sense?
> 

This was exposed very clearly, thanks. I agree with you, it's a good
approach, and we can evolve that during the implementation phase, like
"function A is not good in the hypervisor list because of this and
that", so we move it to the reboot list. Also, name of the lists is not
so relevant, might evolve in the implementation phase - I personally
liked them, specially the "info" and "hypervisor" ones (reboot seems
good but not great heh).

So, what are the opinions from kdump maintainers about this idea?
Baoquan / Vivek / Dave, does it make sense to you? Do you have any
suggestions/concerns to add on top of Petr draft?

I prefer this refactor than the filter, certainly. If nobody else
working on that, I can try implementing that - it's very interesting.
The only thing I'd like to have first is an ACK from the kdump
maintainers about the general idea.

Cheers,


Guilherme

On 02/08/22 at 03:51pm, Guilherme G. Piccoli wrote:
> On 28/01/2022 10:38, Petr Mladek wrote:
> > [...] On Thu 2022-01-27 14:16:20, Guilherme G. Piccoli wrote:
> > First, I am sorry for the very long mail. But the problem is really
> > complicated. I did my best to describe it a clean way.
> > 
> > I have discussed these problems with a colleague and he had some good
> > points. And my view evolved even further.
> 
> Thanks Petr for the very comprehensive and detailed email - this helps a
> lot in shaping the future of panic notifier(s)!
> 
> 
> > [...] 
> > I think about the following solution:
> > 
> >     + split the notifiers into three lists:
> > 
> > 	+ info: stop watchdogs, provide extra info
> > 	+ hypervisor: poke hypervisor
> > 	+ reboot: actions needed only when crash dump did not happen
> > 
> >     + allow to call hypervisor notifiers before or after kdump
> > 
> >     + stop CPUs before kdump when either hypervisor notifiers or
> >       kmsg_dump is enabled
> > 
> > Note that it still allows to call kdump as the first action when
> > hypervisor notifiers are called after kdump and no kmsg dumper
> > is registered.
> > 
> > 
> > void panic(void)
> > {
> > 	[...]
> > 
> > 	if (crash_kexec_post_hypervisor || panic_print || enabled_kmsg_dump()) {
> > 		/*
> > 		 * Stop CPUs when some extra action is required before
> > 		 * crash dump. We will need architecture dependent extra
> > 		 * works in addition to stopping other CPUs.
> > 		 */
> > 		 crash_smp_send_stop();
> > 		 cpus_stopped = true;
> > 	}
> > 
> > 	if (crash_kexec_post_hypervisor) {
> > 		  /* Tell hypervisor about the panic */
> > 		  atomic_notifier_call_chain(&panic_hypervisor_notifier_list, 0, buf);
> > 	}
> > 
> > 	if (enabled_kmsg_dump) {
> > 		  /*
> > 		   * Print extra info by notifiers.
> > 		   * Prevent rumors, for example, by stopping watchdogs.
> > 		   */
> > 		  atomic_notifier_call_chain(&panic_info_notifier_list, 0, buf);
> > 	}
> > 
> > 	/* Optional extra info */
> > 	panic_printk_sys_info();
> > 
> > 	/* No dumper by default */
> > 	kmsg_dump();
> > 
> > 	/* Used only when crash kernel loaded */
> > 	__crash_kexec(NULL);
> > 
> > 	if (!cpus_stopped) {
> > 		/*
> > 		 * Note smp_send_stop is the usual smp shutdown function, which
> > 		 * unfortunately means it may not be hardened to work in a
> > 		 * panic situation.
> > 		 */
> > 		smp_send_stop();
> > 	}
> > 
> > 	if (!crash_kexec_post_hypervisor) {
> > 		  /* Tell hypervisor about the panic */
> > 		  atomic_notifier_call_chain(&panic_hypervisor_notifier_list, 0, buf);
> > 	}
> > 
> > 	if (!enabled_kmsg_dump) {
> > 		  /*
> > 		   * Print extra info by notifiers.
> > 		   * Prevent rumors, for example, by stopping watchdogs.
> > 		   */
> > 		  atomic_notifier_call_chain(&panic_info_notifier_list, 0, buf);
> > 	}
> > 
> > 	/*
> > 	 * Help to reboot a safe way.
> > 	 */
> > 	atomic_notifier_call_chain(&panic_reboot_notifier_list, 0, buf);
> > 
> > 	[...]
> > }
> > 
> > Any opinion?
> > Do the notifier list names make sense?
> > 
> 
> This was exposed very clearly, thanks. I agree with you, it's a good
> approach, and we can evolve that during the implementation phase, like
> "function A is not good in the hypervisor list because of this and
> that", so we move it to the reboot list. Also, name of the lists is not
> so relevant, might evolve in the implementation phase - I personally
> liked them, specially the "info" and "hypervisor" ones (reboot seems
> good but not great heh).
> 
> So, what are the opinions from kdump maintainers about this idea?
> Baoquan / Vivek / Dave, does it make sense to you? Do you have any
> suggestions/concerns to add on top of Petr draft?

Yeah, it's reasonable. As I replied to Michael in another thread, I
think splitting the current notifier list is a good idea. At least the
code to archieve hyper-V's goal with panic_notifier is a little odd and
should be taken out and execute w/o conditional before kdump, and maybe
some others Petr has combed out.

For those which will be switched on with the need of adding panic_notifier
or panic_print into cmdline, the heavy users like HATAYAMA and Masa can
help check.

For Petr's draft code, does it mean hyper-V need another knob to trigger
the needed notifiers? Will you go with the draft direclty? Hyper-V now
runs panic notifiers by default, just a reminder.

> 
> I prefer this refactor than the filter, certainly. If nobody else
> working on that, I can try implementing that - it's very interesting.
> The only thing I'd like to have first is an ACK from the kdump
> maintainers about the general idea.
> 
> Cheers,
> 
> 
> Guilherme
> 

On 08/02/2022 21:31, bhe@redhat.com wrote:
> [...]
>> So, what are the opinions from kdump maintainers about this idea?
>> Baoquan / Vivek / Dave, does it make sense to you? Do you have any
>> suggestions/concerns to add on top of Petr draft?
> 
> Yeah, it's reasonable. As I replied to Michael in another thread, I
> think splitting the current notifier list is a good idea. At least the
> code to archieve hyper-V's goal with panic_notifier is a little odd and
> should be taken out and execute w/o conditional before kdump, and maybe
> some others Petr has combed out.
> 
> For those which will be switched on with the need of adding panic_notifier
> or panic_print into cmdline, the heavy users like HATAYAMA and Masa can
> help check.
> 
> For Petr's draft code, does it mean hyper-V need another knob to trigger
> the needed notifiers? Will you go with the draft direclty? Hyper-V now
> runs panic notifiers by default, just a reminder.
> 

Hi Baoquan, thanks for your comments.

I'll need to study the Hyper-V code and how it's done today - I guess
most part of this implementation will be studying the notifiers we have
currently, split them among the 3 new notifiers and comb them into
patches, so they can be reviewed for all relevant maintainers (who know
the code we are changing).

I'm not sure if I go directly with the draft, likely it'll have some
changes, but the draft should be the skeleton of the new implementation.
Specially if you/other kdump maintainers agree it's a good idea =)

Cheers,


Guilherme

From: Guilherme G. Piccoli <gpiccoli@igalia.com> Sent: Thursday, February 10, 2022 8:40 AM
> 
> On 08/02/2022 21:31, bhe@redhat.com wrote:
> > [...]
> >> So, what are the opinions from kdump maintainers about this idea?
> >> Baoquan / Vivek / Dave, does it make sense to you? Do you have any
> >> suggestions/concerns to add on top of Petr draft?
> >
> > Yeah, it's reasonable. As I replied to Michael in another thread, I
> > think splitting the current notifier list is a good idea. At least the
> > code to archieve hyper-V's goal with panic_notifier is a little odd and
> > should be taken out and execute w/o conditional before kdump, and maybe
> > some others Petr has combed out.
> >
> > For those which will be switched on with the need of adding panic_notifier
> > or panic_print into cmdline, the heavy users like HATAYAMA and Masa can
> > help check.
> >
> > For Petr's draft code, does it mean hyper-V need another knob to trigger
> > the needed notifiers? Will you go with the draft direclty? Hyper-V now
> > runs panic notifiers by default, just a reminder.
> >
> 
> Hi Baoquan, thanks for your comments.
> 
> I'll need to study the Hyper-V code and how it's done today -

Let me know if you need any assistance or explanation as you look
at the Hyper-V code.

Michael Kelley
Principal SW Engineer
Linux Systems Group
Microsoft Corporation

> I guess
> most part of this implementation will be studying the notifiers we have
> currently, split them among the 3 new notifiers and comb them into
> patches, so they can be reviewed for all relevant maintainers (who know
> the code we are changing).
> 
> I'm not sure if I go directly with the draft, likely it'll have some
> changes, but the draft should be the skeleton of the new implementation.
> Specially if you/other kdump maintainers agree it's a good idea =)
> 
> Cheers,
> 
> 
> Guilherme

On 10/02/2022 14:26, Michael Kelley (LINUX) wrote:
> From: Guilherme G. Piccoli <gpiccoli@igalia.com> Sent: Thursday, February 10, 2022 8:40 AM
> [...]>> I'll need to study the Hyper-V code and how it's done today -
> 
> Let me know if you need any assistance or explanation as you look
> at the Hyper-V code.
>
Perfect Michael, thanks a lot! Much appreciated =)

On 28/01/2022 10:38, Petr Mladek wrote:
> [...] 
> I think about the following solution:
> 
>     + split the notifiers into three lists:
> 
> 	+ info: stop watchdogs, provide extra info
> 	+ hypervisor: poke hypervisor
> 	+ reboot: actions needed only when crash dump did not happen
> 
>     + allow to call hypervisor notifiers before or after kdump
> 
>     + stop CPUs before kdump when either hypervisor notifiers or
>       kmsg_dump is enabled
> 
> Note that it still allows to call kdump as the first action when
> hypervisor notifiers are called after kdump and no kmsg dumper
> is registered.
> 
> 
> void panic(void)
> {
> 	[...]
> 
> 	if (crash_kexec_post_hypervisor || panic_print || enabled_kmsg_dump()) {
> 		/*
> 		 * Stop CPUs when some extra action is required before
> 		 * crash dump. We will need architecture dependent extra
> 		 * works in addition to stopping other CPUs.
> 		 */
> 		 crash_smp_send_stop();
> 		 cpus_stopped = true;
> 	}
> 
> 	if (crash_kexec_post_hypervisor) {
> 		  /* Tell hypervisor about the panic */
> 		  atomic_notifier_call_chain(&panic_hypervisor_notifier_list, 0, buf);
> 	}
> 
> 	if (enabled_kmsg_dump) {
> 		  /*
> 		   * Print extra info by notifiers.
> 		   * Prevent rumors, for example, by stopping watchdogs.
> 		   */
> 		  atomic_notifier_call_chain(&panic_info_notifier_list, 0, buf);
> 	}
> 
> 	/* Optional extra info */
> 	panic_printk_sys_info();
> 
> 	/* No dumper by default */
> 	kmsg_dump();
> 
> 	/* Used only when crash kernel loaded */
> 	__crash_kexec(NULL);
> 
> 	if (!cpus_stopped) {
> 		/*
> 		 * Note smp_send_stop is the usual smp shutdown function, which
> 		 * unfortunately means it may not be hardened to work in a
> 		 * panic situation.
> 		 */
> 		smp_send_stop();
> 	}
> 
> 	if (!crash_kexec_post_hypervisor) {
> 		  /* Tell hypervisor about the panic */
> 		  atomic_notifier_call_chain(&panic_hypervisor_notifier_list, 0, buf);
> 	}
> 
> 	if (!enabled_kmsg_dump) {
> 		  /*
> 		   * Print extra info by notifiers.
> 		   * Prevent rumors, for example, by stopping watchdogs.
> 		   */
> 		  atomic_notifier_call_chain(&panic_info_notifier_list, 0, buf);
> 	}
> 
> 	/*
> 	 * Help to reboot a safe way.
> 	 */
> 	atomic_notifier_call_chain(&panic_reboot_notifier_list, 0, buf);
> 
> 	[...]
> }
> 
> Any opinion?
> Do the notifier list names make sense?
> 
> Best Regards,
> Petr


Hi folks, I'm working on this now, and while looking into it I've
noticed that we have the concept of "priority" in the notifiers list.
Basically, you can order the calls the way it fits best, priority is an
integer and must the set in the moment of registration, it's up to the
users of the notifiers to set it and enforce the ordering.

So what I'm thinking is: currently, only 3 or 4 panic notifiers make use
of that. What if, since we're re-working this, we add a priority for
*all* notifiers and enforce its usage? This way we guarantee
consistency, it'd make debug easier and maybe even more important:
having all the notifiers and their priorities in a list present in the
header file would be great documentation about all the existing
notifiers and how they are called - today this information is quite
obscure and requires lots of code grepping!

Let me know your thoughts Petr / Baoquan - it would add slightly more
code / complexity, but in my opinion the payback is very good.
Cheers,


Guilherme