| 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: <5d4b2b95-bb11-4d1f-8cba-0bcd8e864711@suse.com>
Date: Tue, 1 Apr 2025 18:00:57 +0300
From: Nikolay Borisov <nik.borisov@...e.com>
To: Sohil Mehta <sohil.mehta@...el.com>, x86@...nel.org,
Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>
Cc: Borislav Petkov <bp@...en8.de>, Dave Hansen
<dave.hansen@...ux.intel.com>, "H . Peter Anvin" <hpa@...or.com>,
Josh Poimboeuf <jpoimboe@...nel.org>, Peter Zijlstra <peterz@...radead.org>,
"Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
Kai Huang <kai.huang@...el.com>,
Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
Mike Rapoport <rppt@...nel.org>, Petr Mladek <pmladek@...e.com>,
Jani Nikula <jani.nikula@...el.com>, Tony Luck <tony.luck@...el.com>,
Xin Li <xin@...or.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/9] x86/nmi: Simplify unknown NMI panic handling
On 28.03.25 г. 1:46 ч., Sohil Mehta wrote:
> The unknown_nmi_panic variable is used to control whether the kernel
> should panic on unknown NMIs. There is a sysctl entry for the same, which
> can be used to change the behavior at runtime.
>
> However, it seems that in some places, the option unnecessarily depends
> on CONFIG_X86_LOCAL_APIC. Other code in nmi.c uses unknown_nmi_panic
> without such a dependency. This results in a few messy #ifdefs
> splattered across the code. The dependency was likely introduce due to a
> potential compile issue [1] reported a long time ago. Such an issue no
> longer exists.
>
> Also, similar NMI panic options, such as panic_on_unrecovered_nmi and
> panic_on_io_nmi, do not have an explicit dependency on the local APIC.
> Though, it's hard to imagine a production system without the local APIC
> configuration, making a specific NMI sysctl option dependent on it
> doesn't make sense.
>
> Remove the explicit dependency between unknown NMI handling and the
> local APIC to make the code cleaner and more consistent.
>
> While at it, reorder the header includes to maintain alphabetical order.
> > [1]: https://lore.kernel.org/lkml/40BC67F9.3000609@myrealbox.com/
>
> Signed-off-by: Sohil Mehta <sohil.mehta@...el.com>
> ---
> arch/x86/include/asm/nmi.h | 4 ++--
> arch/x86/kernel/setup.c | 37 ++++++++++++++++---------------------
> 2 files changed, 18 insertions(+), 23 deletions(-)
>
> diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h
> index f677382093f3..9cf96cce02fc 100644
> --- a/arch/x86/include/asm/nmi.h
> +++ b/arch/x86/include/asm/nmi.h
> @@ -14,10 +14,10 @@ extern void release_perfctr_nmi(unsigned int);
> extern int reserve_evntsel_nmi(unsigned int);
> extern void release_evntsel_nmi(unsigned int);
>
> -extern int unknown_nmi_panic;
> -
> #endif /* CONFIG_X86_LOCAL_APIC */
>
> +extern int unknown_nmi_panic;
> +
> #define NMI_FLAG_FIRST 1
>
> enum {
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index c7164a8de983..c3e1ae7373e9 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -7,10 +7,11 @@
> */
> #include <linux/acpi.h>
> #include <linux/console.h>
> -#include <linux/cpu.h>
> #include <linux/crash_dump.h>
> +#include <linux/cpu.h>
> #include <linux/dma-map-ops.h>
> #include <linux/efi.h>
> +#include <linux/hugetlb.h>
> #include <linux/ima.h>
> #include <linux/init_ohci1394_dma.h>
> #include <linux/initrd.h>
> @@ -18,21 +19,19 @@
> #include <linux/memblock.h>
> #include <linux/panic_notifier.h>
> #include <linux/pci.h>
> +#include <linux/random.h>
> #include <linux/root_dev.h>
> -#include <linux/hugetlb.h>
> -#include <linux/tboot.h>
> -#include <linux/usb/xhci-dbgp.h>
> #include <linux/static_call.h>
> #include <linux/swiotlb.h>
> -#include <linux/random.h>
> +#include <linux/tboot.h>
> +#include <linux/usb/xhci-dbgp.h>
> +#include <linux/vmalloc.h>
>
> #include <uapi/linux/mount.h>
>
> #include <xen/xen.h>
>
> #include <asm/apic.h>
> -#include <asm/efi.h>
> -#include <asm/numa.h>
> #include <asm/bios_ebda.h>
> #include <asm/bugs.h>
> #include <asm/cacheinfo.h>
> @@ -47,18 +46,16 @@
> #include <asm/mce.h>
> #include <asm/memtype.h>
> #include <asm/mtrr.h>
> -#include <asm/realmode.h>
> +#include <asm/nmi.h>
> +#include <asm/numa.h>
> #include <asm/olpc_ofw.h>
> #include <asm/pci-direct.h>
> #include <asm/prom.h>
> #include <asm/proto.h>
> +#include <asm/realmode.h>
> #include <asm/thermal.h>
> #include <asm/unwind.h>
> #include <asm/vsyscall.h>
> -#include <linux/vmalloc.h>
> -#if defined(CONFIG_X86_LOCAL_APIC)
> -#include <asm/nmi.h>
> -#endif
As far as headers are concerned only this change falls under the
"simplify nmi handling code" the others while nice cleanups should
ideally be in a separate patch.
>
> /*
> * max_low_pfn_mapped: highest directly mapped pfn < 4 GB
> @@ -150,6 +147,13 @@ static size_t ima_kexec_buffer_size;
> int bootloader_type, bootloader_version;
>
> static const struct ctl_table x86_sysctl_table[] = {
> + {
> + .procname = "unknown_nmi_panic",
> + .data = &unknown_nmi_panic,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = proc_dointvec,
> + },
> {
> .procname = "panic_on_unrecovered_nmi",
> .data = &panic_on_unrecovered_nmi,
> @@ -185,15 +189,6 @@ static const struct ctl_table x86_sysctl_table[] = {
> .mode = 0644,
> .proc_handler = proc_dointvec,
> },
> -#if defined(CONFIG_X86_LOCAL_APIC)
> - {
> - .procname = "unknown_nmi_panic",
> - .data = &unknown_nmi_panic,
> - .maxlen = sizeof(int),
> - .mode = 0644,
> - .proc_handler = proc_dointvec,
> - },
> -#endif
Why move the definition and not just delete the #ifdef ?
> #if defined(CONFIG_ACPI_SLEEP)
> {
> .procname = "acpi_video_flags",
Powered by blists - more mailing lists