Re: [PATCH] powerpc/fadump, x86/sev: Inform about unconditionally enabling crash_kexec_post_notifiers

["Guilherme G. Piccoli" <gpiccoli@igalia.com>]

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);
>