| 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: <0aee5cd1-eb8c-f778-c6e5-7f5ca3624d15@oracle.com>
Date: Tue, 26 Apr 2022 15:08:26 -0500
From: Eric DeVolder <eric.devolder@...cle.com>
To: Borislav Petkov <bp@...en8.de>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org,
kexec@...ts.infradead.org, ebiederm@...ssion.com,
dyoung@...hat.com, bhe@...hat.com, vgoyal@...hat.com,
tglx@...utronix.de, mingo@...hat.com, dave.hansen@...ux.intel.com,
hpa@...or.com, nramas@...ux.microsoft.com, thomas.lendacky@....com,
robh@...nel.org, efault@....de, rppt@...nel.org, david@...hat.com,
konrad.wilk@...cle.com, boris.ostrovsky@...cle.com
Subject: Re: [PATCH v7 2/8] x86/crash: Introduce new options to support cpu
and memory hotplug
Boris,
Many thanks for the time taken to illustrate for me!
I've one question below.
Eric
On 4/25/22 14:25, Borislav Petkov wrote:
> On Tue, Apr 19, 2022 at 04:58:47PM -0500, Eric DeVolder wrote:
>> So in taking this concept and looking at, in particular, patch 4/8
>> "crash: add generic infrastructure for crash hotplug support", I'm not
>> exactly sure how to apply this technique.
>
> So I took your patch 4 and maimed into what I think it should look like,
> see below.
>
> Now there's a single ifdef there and the registration routines are
> wrapped in IS_ENABLED() so that you register a callback only when the
> respective stuff - HOTPLUG_CPU or MEMORY_HOTPLUG - is enabled. Otherwise
> the couple of functions are unused but that's not that big of a deal.
>
> I've also fixed up some other issues I've encountered along the way.
>
> Holler if there are questions.
>
> HTH.
>
> ---
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index de62a722431e..b3c32e04d3f0 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -84,4 +84,12 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
> int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
> unsigned long long *crash_size, unsigned long long *crash_base);
>
> +#define KEXEC_CRASH_HP_REMOVE_CPU 0
> +#define KEXEC_CRASH_HP_ADD_CPU 1
> +#define KEXEC_CRASH_HP_REMOVE_MEMORY 2
> +#define KEXEC_CRASH_HP_ADD_MEMORY 3
> +
> +struct kimage;
> +void arch_crash_handle_hotplug_event(struct kimage *image, unsigned int hp_action,
> + unsigned int cpu);
> #endif /* LINUX_CRASH_CORE_H */
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 02daff1f47dd..d907a1f0d3da 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -307,13 +307,11 @@ struct kimage {
> /* Information for loading purgatory */
> struct purgatory_info purgatory_info;
>
> -#ifdef CONFIG_CRASH_HOTPLUG
> bool hotplug_event;
> unsigned int offlinecpu;
> bool elfcorehdr_index_valid;
> int elfcorehdr_index;
> #endif
> -#endif
>
> #ifdef CONFIG_IMA_KEXEC
> /* Virtual address of IMA measurement buffer for kexec syscall */
> @@ -329,15 +327,6 @@ struct kimage {
> unsigned long elf_load_addr;
> };
>
> -#ifdef CONFIG_CRASH_HOTPLUG
> -void arch_crash_hotplug_handler(struct kimage *image,
> - unsigned int hp_action, unsigned int cpu);
> -#define KEXEC_CRASH_HP_REMOVE_CPU 0
> -#define KEXEC_CRASH_HP_ADD_CPU 1
> -#define KEXEC_CRASH_HP_REMOVE_MEMORY 2
> -#define KEXEC_CRASH_HP_ADD_MEMORY 3
> -#endif /* CONFIG_CRASH_HOTPLUG */
> -
> /* kexec interface functions */
> extern void machine_kexec(struct kimage *image);
> extern int machine_kexec_prepare(struct kimage *image);
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index ecf746243ab2..a6c4ee1d5c86 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -497,57 +497,50 @@ static int __init crash_save_vmcoreinfo_init(void)
>
> subsys_initcall(crash_save_vmcoreinfo_init);
>
> -#ifdef CONFIG_CRASH_HOTPLUG
> -void __weak arch_crash_hotplug_handler(struct kimage *image,
> - unsigned int hp_action, unsigned int cpu)
> +void __weak arch_crash_handle_hotplug_event(struct kimage *image, unsigned int hp_action,
> + unsigned int cpu)
> {
> - pr_warn("crash hp: %s not implemented", __func__);
> + WARN(1, "crash hotplug handler not implemented");
> }
>
> -static void crash_hotplug_handler(unsigned int hp_action,
> - unsigned int cpu)
> +#if defined(CONFIG_HOTPLUG_CPU) || defined(CONFIG_MEMORY_HOTPLUG)
> +static void handle_hotplug_event(unsigned int hp_action, unsigned int cpu)
> {
> - /* Obtain lock while changing crash information */
> - if (!mutex_trylock(&kexec_mutex))
> + if (!kexec_crash_image)
> return;
Why is it safe to examine kexec_crash_image outside the mutex? As I understand it, there is still
the (very rare) opportunity for a kdump load/unload initiated via userland and this check to
collide. (Similarly, I believe the mutex entry is almost always assured/likely.)
>
> - /* Check kdump is loaded */
> - if (kexec_crash_image) {
> - pr_debug("crash hp: hp_action %u, cpu %u", hp_action, cpu);
> + mutex_lock(&kexec_mutex);
>
> - /* Needed in order for the segments to be updated */
> - arch_kexec_unprotect_crashkres();
> + pr_debug("crash hotplug: hp_action %u, cpu %u", hp_action, cpu);
>
> - /* Flag to differentiate between normal load and hotplug */
> - kexec_crash_image->hotplug_event = true;
> + /* Needed in order for the segments to be updated */
> + arch_kexec_unprotect_crashkres();
>
> - /* Now invoke arch-specific update handler */
> - arch_crash_hotplug_handler(kexec_crash_image, hp_action, cpu);
> + /* Flag to differentiate between normal load and hotplug */
> + kexec_crash_image->hotplug_event = true;
>
> - /* No longer handling a hotplug event */
> - kexec_crash_image->hotplug_event = false;
> + /* Now invoke arch-specific update handler */
> + arch_crash_handle_hotplug_event(kexec_crash_image, hp_action, cpu);
>
> - /* Change back to read-only */
> - arch_kexec_protect_crashkres();
> - }
> + /* No longer handling a hotplug event */
> + kexec_crash_image->hotplug_event = false;
> +
> + /* Change back to read-only */
> + arch_kexec_protect_crashkres();
>
> /* Release lock now that update complete */
> mutex_unlock(&kexec_mutex);
> }
>
> -#if defined(CONFIG_MEMORY_HOTPLUG)
> -static int crash_memhp_notifier(struct notifier_block *nb,
> - unsigned long val, void *v)
> +static int crash_memhp_notifier(struct notifier_block *nb, unsigned long val, void *v)
> {
> - struct memory_notify *mhp = v;
> -
> switch (val) {
> case MEM_ONLINE:
> - crash_hotplug_handler(KEXEC_CRASH_HP_ADD_MEMORY, -1U);
> + handle_hotplug_event(KEXEC_CRASH_HP_ADD_MEMORY, -1U);
> break;
>
> case MEM_OFFLINE:
> - crash_hotplug_handler(KEXEC_CRASH_HP_REMOVE_MEMORY, -1U);
> + handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_MEMORY, -1U);
> break;
> }
> return NOTIFY_OK;
> @@ -557,38 +550,33 @@ static struct notifier_block crash_memhp_nb = {
> .notifier_call = crash_memhp_notifier,
> .priority = 0
> };
> -#endif
>
> -#if defined(CONFIG_HOTPLUG_CPU)
> static int crash_cpuhp_online(unsigned int cpu)
> {
> - crash_hotplug_handler(KEXEC_CRASH_HP_ADD_CPU, cpu);
> + handle_hotplug_event(KEXEC_CRASH_HP_ADD_CPU, cpu);
> return 0;
> }
>
> static int crash_cpuhp_offline(unsigned int cpu)
> {
> - crash_hotplug_handler(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
> + handle_hotplug_event(KEXEC_CRASH_HP_REMOVE_CPU, cpu);
> return 0;
> }
> -#endif
>
> static int __init crash_hotplug_init(void)
> {
> int result = 0;
>
> -#if defined(CONFIG_MEMORY_HOTPLUG)
> - register_memory_notifier(&crash_memhp_nb);
> -#endif
> + if (IS_ENABLED(CONFIG_MEMORY_HOTPLUG))
> + register_memory_notifier(&crash_memhp_nb);
>
> -#if defined(CONFIG_HOTPLUG_CPU)
> - result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> - "crash/cpuhp",
> - crash_cpuhp_online, crash_cpuhp_offline);
> -#endif
> + if (IS_ENABLED(CONFIG_HOTPLUG_CPU))
> + result = cpuhp_setup_state_nocalls(CPUHP_AP_ONLINE_DYN,
> + "crash/cpuhp",
> + crash_cpuhp_online,
> + crash_cpuhp_offline);
>
> return result;
> }
> -
> subsys_initcall(crash_hotplug_init);
> -#endif /* CONFIG_CRASH_HOTPLUG */
> +#endif
>
Powered by blists - more mailing lists