| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <50e7cfb4-4edd-4fa0-ba3e-b22d8d324b69@redhat.com>
Date: Tue, 4 Feb 2025 23:03:22 -0500
From: Waiman Long <llong@...hat.com>
To: Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>,
Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>,
Peter Zijlstra <peterz@...radead.org>
Cc: x86@...nel.org, linux-kernel@...r.kernel.org,
"H. Peter Anvin" <hpa@...or.com>, Rik van Riel <riel@...riel.com>
Subject: Re: [PATCH v4] x86/nmi: Add an emergency handler in nmi_desc & use it
in nmi_shootdown_cpus()
On 12/19/24 10:06 AM, Waiman Long wrote:
> Depending on the type of panics, it was found that the
> __register_nmi_handler() function can be called in NMI context from
> nmi_shootdown_cpus() leading to a lockdep splat like the following.
>
> [ 1123.133573] ================================
> [ 1123.137845] WARNING: inconsistent lock state
> [ 1123.142118] 6.12.0-31.el10.x86_64+debug #1 Not tainted
> [ 1123.147257] --------------------------------
> [ 1123.151529] inconsistent {INITIAL USE} -> {IN-NMI} usage.
> :
> [ 1123.261544] Possible unsafe locking scenario:
> [ 1123.261544]
> [ 1123.267463] CPU0
> [ 1123.269915] ----
> [ 1123.272368] lock(&nmi_desc[0].lock);
> [ 1123.276122] <Interrupt>
> [ 1123.278746] lock(&nmi_desc[0].lock);
> [ 1123.282671]
> [ 1123.282671] *** DEADLOCK ***
> :
> [ 1123.314088] Call Trace:
> [ 1123.316542] <NMI>
> [ 1123.318562] dump_stack_lvl+0x6f/0xb0
> [ 1123.322230] print_usage_bug.part.0+0x3d3/0x610
> [ 1123.330618] lock_acquire.part.0+0x2e6/0x360
> [ 1123.357217] _raw_spin_lock_irqsave+0x46/0x90
> [ 1123.366193] __register_nmi_handler+0x8f/0x3a0
> [ 1123.374401] nmi_shootdown_cpus+0x95/0x120
> [ 1123.378509] kdump_nmi_shootdown_cpus+0x15/0x20
> [ 1123.383040] native_machine_crash_shutdown+0x54/0x160
> [ 1123.388095] __crash_kexec+0x10f/0x1f0
> [ 1123.421465] ? __ghes_panic.cold+0x4f/0x5d
> [ 1123.482648] </NMI>
>
> In this particular case, the following panic message was printed before.
>
> [ 1122.808188] Kernel panic - not syncing: Fatal hardware error!
>
> This message seemed to be given out from __ghes_panic() running in
> NMI context.
>
> The __register_nmi_handler() function which takes the nmi_desc lock
> with irq disabled shouldn't be called from NMI context as this can
> lead to deadlock.
>
> The nmi_shootdown_cpus() function can only be invoked once. After the
> first invocation, all other CPUs should be stuck in the newly added
> crash_nmi_callback() and cannot respond to a second NMI.
>
> One way to address this problem is to remove all the panic() calls from
> NMI context, but that can be too restrictive.
>
> Another way to fix this problem while allowing panic() calls from
> NMI context is by adding a new emergency NMI handler to the nmi_desc
> structure and provide a new set_emergency_nmi_handler() helper to
> atomically set crash_nmi_callback() in any context. The new emergency
> handler will preempt other handlers in the linked list. That will
> eliminate the need to take any lock and serve the panic in NMI use case.
>
> Signed-off-by: Waiman Long <longman@...hat.com>
> Acked-by: Rik van Riel <riel@...riel.com>
> ---
> arch/x86/include/asm/nmi.h | 2 ++
> arch/x86/kernel/nmi.c | 45 ++++++++++++++++++++++++++++++++++++++
> arch/x86/kernel/reboot.c | 11 ++++------
> 3 files changed, 51 insertions(+), 7 deletions(-)
>
> [v4] Just twist the C comments, no code change from v3.
>
> diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
> index 41a0ebb699ec..6715c123eff4 100644
> --- a/arch/x86/include/asm/nmi.h
> +++ b/arch/x86/include/asm/nmi.h
> @@ -56,6 +56,8 @@ int __register_nmi_handler(unsigned int, struct nmiaction *);
>
> void unregister_nmi_handler(unsigned int, const char *);
>
> +int set_emergency_nmi_handler(unsigned int type, nmi_handler_t handler);
> +
> void stop_nmi(void);
> void restart_nmi(void);
> void local_touch_nmi(void);
> diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c
> index ed163c8c8604..2cb75a53c0c4 100644
> --- a/arch/x86/kernel/nmi.c
> +++ b/arch/x86/kernel/nmi.c
> @@ -40,8 +40,12 @@
> #define CREATE_TRACE_POINTS
> #include <trace/events/nmi.h>
>
> +/*
> + * An emergency handler can be set in any context including NMI
> + */
> struct nmi_desc {
> raw_spinlock_t lock;
> + nmi_handler_t emerg_handler; /* Emergency handler */
> struct list_head head;
> };
>
> @@ -132,9 +136,22 @@ static void nmi_check_duration(struct nmiaction *action, u64 duration)
> static int nmi_handle(unsigned int type, struct pt_regs *regs)
> {
> struct nmi_desc *desc = nmi_to_desc(type);
> + nmi_handler_t ehandler;
> struct nmiaction *a;
> int handled=0;
>
> + /*
> + * Call the emergency handler, if set
> + *
> + * In the case of crash_nmi_callback() emergency handler, it will
> + * return in the case of the crashing CPU to enable it to complete
> + * other necessary crashing actions ASAP. Other handlers in the
> + * linked list won't need to be run.
> + */
> + ehandler = READ_ONCE(desc->emerg_handler);
> + if (ehandler)
> + return ehandler(type, regs);
> +
> rcu_read_lock();
>
> /*
> @@ -224,6 +241,34 @@ void unregister_nmi_handler(unsigned int type, const char *name)
> }
> EXPORT_SYMBOL_GPL(unregister_nmi_handler);
>
> +/**
> + * set_emergency_nmi_handler - Set emergency handler
> + * @type: NMI type
> + * @handler: the emergency handler to be stored
> + * Return: 0 if success, -EEXIST if a handler had been stored
> + *
> + * Atomically set an emergency NMI handler which, if set, will preempt all
> + * the other handlers in the linked list. If a NULL handler is passed in,
> + * it will clear it.
> + */
> +int set_emergency_nmi_handler(unsigned int type, nmi_handler_t handler)
> +{
> + struct nmi_desc *desc = nmi_to_desc(type);
> + nmi_handler_t orig = NULL;
> +
> + if (!handler) {
> + orig = READ_ONCE(desc->emerg_handler);
> + WARN_ON_ONCE(!orig);
> + }
> +
> + if (try_cmpxchg(&desc->emerg_handler, &orig, handler))
> + return 0;
> + if (WARN_ON_ONCE(orig == handler))
> + return 0;
> + WARN_ONCE(1, "%s: failed to set emergency NMI handler!\n", __func__);
> + return -EEXIST;
> +}
> +
> static void
> pci_serr_error(unsigned char reason, struct pt_regs *regs)
> {
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 615922838c51..12df9e402d3c 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -926,15 +926,12 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
> shootdown_callback = callback;
>
> atomic_set(&waiting_for_crash_ipi, num_online_cpus() - 1);
> - /* Would it be better to replace the trap vector here? */
> - if (register_nmi_handler(NMI_LOCAL, crash_nmi_callback,
> - NMI_FLAG_FIRST, "crash"))
> - return; /* Return what? */
> +
> /*
> - * Ensure the new callback function is set before sending
> - * out the NMI
> + * Atomically set emergency handler to preempt other handlers.
> + * The action shouldn't fail or a warning will be printed.
> */
> - wmb();
> + set_emergency_nmi_handler(NMI_LOCAL, crash_nmi_callback);
>
> apic_send_IPI_allbutself(NMI_VECTOR);
>
Ping! Is further change needed for this patch?
Thanks,
Longman
Powered by blists - more mailing lists