| Message ID | 20240830141752.460173-1-gpiccoli@igalia.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | powerpc/fadump, x86/sev: Inform about unconditionally enabling crash_kexec_post_notifiers | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 21 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 5 jobs. |
"Guilherme G. Piccoli" <gpiccoli@igalia.com> writes: > Inspired by commit d57d6fe5bf34 ("drivers: hv: log when enabling crash_kexec_post_notifiers"), > a good idea is to signal on dmesg about unconditionally enabling the kernel > parameter crash_kexec_post_notifiers, which affects how kdump works. > > By checking the source code, found 2 more cases besides Hyper-V, so let's > print that on dmesg for them as well. This is a great catch! One of those was already there when I sent the original patch, but the other is new. Could we maybe go further than this, and delete the public declarations of crash_kexec_post_notifiers in "include/linux"? (I see two). We could replace the users that set it to true with a function that logs the change so that it's impossible for new code to set it directly without notifying the user. Something like this? Compile tested only for x86. commit da8691a25d7b0c2f914720bc054dd1d9dbe4b373 Author: Stephen Brennan <stephen.s.brennan@oracle.com> Date: Fri Aug 30 10:49:24 2024 -0700 panic: make crash_kexec_post_notifiers private This requires that any in-kernel user setting it directly must log the reason so that users are aware their panic behavior may be different from their configuration. Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com> diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c index a612e7513a4f8..9966f29409599 100644 --- a/arch/powerpc/kernel/fadump.c +++ b/arch/powerpc/kernel/fadump.c @@ -1818,7 +1818,7 @@ int __init setup_fadump(void) * lets panic() function take crash friendly path before panic * notifiers are invoked. */ - crash_kexec_post_notifiers = true; + enable_crash_kexec_post_notifiers("PPC/fadump"); return 1; } diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c index 0ce17766c0e52..6e9f5f8d13cc5 100644 --- a/arch/x86/virt/svm/sev.c +++ b/arch/x86/virt/svm/sev.c @@ -256,7 +256,7 @@ static int __init snp_rmptable_init(void) * Setting crash_kexec_post_notifiers to 'true' to ensure that SNP panic * notifier is invoked to do SNP IOMMU shutdown before kdump. */ - crash_kexec_post_notifiers = true; + enable_crash_kexec_post_notifiers("AMD/SEV"); return 0; diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c index 9c452bfbd5719..fa3bbb66235de 100644 --- a/drivers/hv/hv_common.c +++ b/drivers/hv/hv_common.c @@ -303,8 +303,7 @@ int __init hv_common_init(void) if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) { u64 hyperv_crash_ctl; - crash_kexec_post_notifiers = true; - pr_info("Hyper-V: enabling crash_kexec_post_notifiers\n"); + enable_crash_kexec_post_notifiers("Hyper-V"); /* * Panic message recording (sysctl_record_panic_msg) diff --git a/include/linux/panic.h b/include/linux/panic.h index 54d90b6c5f47b..697184664c6f4 100644 --- a/include/linux/panic.h +++ b/include/linux/panic.h @@ -31,8 +31,6 @@ extern int sysctl_panic_on_rcu_stall; extern int sysctl_max_rcu_stall_to_panic; extern int sysctl_panic_on_stackoverflow; -extern bool crash_kexec_post_notifiers; - extern void __stack_chk_fail(void); void abort(void); diff --git a/include/linux/panic_notifier.h b/include/linux/panic_notifier.h index 41e32483d7a7b..97c31cf5c2fdb 100644 --- a/include/linux/panic_notifier.h +++ b/include/linux/panic_notifier.h @@ -7,6 +7,6 @@ extern struct atomic_notifier_head panic_notifier_list; -extern bool crash_kexec_post_notifiers; +void enable_crash_kexec_post_notifiers(const char *reason); #endif /* _LINUX_PANIC_NOTIFIERS_H */ diff --git a/kernel/crash_core.c b/kernel/crash_core.c index 63cf89393c6eb..634c6b99717c5 100644 --- a/kernel/crash_core.c +++ b/kernel/crash_core.c @@ -33,6 +33,9 @@ /* Per cpu memory for storing cpu states in case of system crash. */ note_buf_t __percpu *crash_notes; +/* Defined in kernel/panic.c and needed here, but not intended to be public. */ +extern bool crash_kexec_post_notifiers; + #ifdef CONFIG_CRASH_DUMP int kimage_crash_copy_vmcoreinfo(struct kimage *image) diff --git a/kernel/panic.c b/kernel/panic.c index 2a0449144f82e..f4ae3abbea7ed 100644 --- a/kernel/panic.c +++ b/kernel/panic.c @@ -137,6 +137,12 @@ static long no_blink(int state) return 0; } +void enable_crash_kexec_post_notifiers(const char *reason) +{ + crash_kexec_post_notifiers = true; + pr_info("%s: enabling crash_kexec_post_notifiers\n", reason); +} + /* Returns how long it waited in ms */ long (*panic_blink)(int state); EXPORT_SYMBOL(panic_blink);
On 30/08/2024 14:54, Stephen Brennan wrote: > [...] > Could we maybe go further than this, and delete the public declarations > of crash_kexec_post_notifiers in "include/linux"? (I see two). We could > replace the users that set it to true with a function that logs the > change so that it's impossible for new code to set it directly without > notifying the user. Something like this? Compile tested only for x86. > > commit da8691a25d7b0c2f914720bc054dd1d9dbe4b373 > Author: Stephen Brennan <stephen.s.brennan@oracle.com> > Date: Fri Aug 30 10:49:24 2024 -0700 > > panic: make crash_kexec_post_notifiers private > > This requires that any in-kernel user setting it directly must log the > reason so that users are aware their panic behavior may be different > from their configuration. > > Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com> > Thanks Stephen! I'm totally into that, your approach is very good. Cheers, Guilherme > diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c > index a612e7513a4f8..9966f29409599 100644 > --- a/arch/powerpc/kernel/fadump.c > +++ b/arch/powerpc/kernel/fadump.c > @@ -1818,7 +1818,7 @@ int __init setup_fadump(void) > * lets panic() function take crash friendly path before panic > * notifiers are invoked. > */ > - crash_kexec_post_notifiers = true; > + enable_crash_kexec_post_notifiers("PPC/fadump"); > > return 1; > } > diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c > index 0ce17766c0e52..6e9f5f8d13cc5 100644 > --- a/arch/x86/virt/svm/sev.c > +++ b/arch/x86/virt/svm/sev.c > @@ -256,7 +256,7 @@ static int __init snp_rmptable_init(void) > * Setting crash_kexec_post_notifiers to 'true' to ensure that SNP panic > * notifier is invoked to do SNP IOMMU shutdown before kdump. > */ > - crash_kexec_post_notifiers = true; > + enable_crash_kexec_post_notifiers("AMD/SEV"); > > return 0; > > diff --git a/drivers/hv/hv_common.c b/drivers/hv/hv_common.c > index 9c452bfbd5719..fa3bbb66235de 100644 > --- a/drivers/hv/hv_common.c > +++ b/drivers/hv/hv_common.c > @@ -303,8 +303,7 @@ int __init hv_common_init(void) > if (ms_hyperv.misc_features & HV_FEATURE_GUEST_CRASH_MSR_AVAILABLE) { > u64 hyperv_crash_ctl; > > - crash_kexec_post_notifiers = true; > - pr_info("Hyper-V: enabling crash_kexec_post_notifiers\n"); > + enable_crash_kexec_post_notifiers("Hyper-V"); > > /* > * Panic message recording (sysctl_record_panic_msg) > diff --git a/include/linux/panic.h b/include/linux/panic.h > index 54d90b6c5f47b..697184664c6f4 100644 > --- a/include/linux/panic.h > +++ b/include/linux/panic.h > @@ -31,8 +31,6 @@ extern int sysctl_panic_on_rcu_stall; > extern int sysctl_max_rcu_stall_to_panic; > extern int sysctl_panic_on_stackoverflow; > > -extern bool crash_kexec_post_notifiers; > - > extern void __stack_chk_fail(void); > void abort(void); > > diff --git a/include/linux/panic_notifier.h b/include/linux/panic_notifier.h > index 41e32483d7a7b..97c31cf5c2fdb 100644 > --- a/include/linux/panic_notifier.h > +++ b/include/linux/panic_notifier.h > @@ -7,6 +7,6 @@ > > extern struct atomic_notifier_head panic_notifier_list; > > -extern bool crash_kexec_post_notifiers; > +void enable_crash_kexec_post_notifiers(const char *reason); > > #endif /* _LINUX_PANIC_NOTIFIERS_H */ > diff --git a/kernel/crash_core.c b/kernel/crash_core.c > index 63cf89393c6eb..634c6b99717c5 100644 > --- a/kernel/crash_core.c > +++ b/kernel/crash_core.c > @@ -33,6 +33,9 @@ > /* Per cpu memory for storing cpu states in case of system crash. */ > note_buf_t __percpu *crash_notes; > > +/* Defined in kernel/panic.c and needed here, but not intended to be public. */ > +extern bool crash_kexec_post_notifiers; > + > #ifdef CONFIG_CRASH_DUMP > > int kimage_crash_copy_vmcoreinfo(struct kimage *image) > diff --git a/kernel/panic.c b/kernel/panic.c > index 2a0449144f82e..f4ae3abbea7ed 100644 > --- a/kernel/panic.c > +++ b/kernel/panic.c > @@ -137,6 +137,12 @@ static long no_blink(int state) > return 0; > } > > +void enable_crash_kexec_post_notifiers(const char *reason) > +{ > + crash_kexec_post_notifiers = true; > + pr_info("%s: enabling crash_kexec_post_notifiers\n", reason); > +} > + > /* Returns how long it waited in ms */ > long (*panic_blink)(int state); > EXPORT_SYMBOL(panic_blink); >
diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c index a612e7513a4f..37dee89a0bf2 100644 --- a/arch/powerpc/kernel/fadump.c +++ b/arch/powerpc/kernel/fadump.c @@ -1819,6 +1819,7 @@ int __init setup_fadump(void) * notifiers are invoked. */ crash_kexec_post_notifiers = true; + pr_info("PPC/fadump: enabling crash_kexec_post_notifiers\n"); return 1; } diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c index 0ce17766c0e5..ac445ad2fcc8 100644 --- a/arch/x86/virt/svm/sev.c +++ b/arch/x86/virt/svm/sev.c @@ -257,6 +257,7 @@ static int __init snp_rmptable_init(void) * notifier is invoked to do SNP IOMMU shutdown before kdump. */ crash_kexec_post_notifiers = true; + pr_info("AMD/SEV: enabling crash_kexec_post_notifiers\n"); return 0;
Inspired by commit d57d6fe5bf34 ("drivers: hv: log when enabling crash_kexec_post_notifiers"), a good idea is to signal on dmesg about unconditionally enabling the kernel parameter crash_kexec_post_notifiers, which affects how kdump works. By checking the source code, found 2 more cases besides Hyper-V, so let's print that on dmesg for them as well. Signed-off-by: Guilherme G. Piccoli <gpiccoli@igalia.com> --- arch/powerpc/kernel/fadump.c | 1 + arch/x86/virt/svm/sev.c | 1 + 2 files changed, 2 insertions(+)