linux-kernel - Re: [PATCH v18 5/7] kexec: exclude hot remove cpu from elfcorehdr notes

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 01 Feb 2023 12:33:17 +0100
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Eric DeVolder <eric.devolder@...cle.com>,
        linux-kernel@...r.kernel.org, x86@...nel.org,
        kexec@...ts.infradead.org, ebiederm@...ssion.com,
        dyoung@...hat.com, bhe@...hat.com, vgoyal@...hat.com
Cc:     mingo@...hat.com, bp@...en8.de, 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,
        sourabhjain@...ux.ibm.com, konrad.wilk@...cle.com,
        boris.ostrovsky@...cle.com, eric.devolder@...cle.com
Subject: Re: [PATCH v18 5/7] kexec: exclude hot remove cpu from elfcorehdr
 notes

Eric!

On Tue, Jan 31 2023 at 17:42, Eric DeVolder wrote:
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -366,6 +366,14 @@ int crash_prepare_elf64_headers(struct kimage *image, struct crash_mem *mem,
>  
>  	/* Prepare one phdr of type PT_NOTE for each present CPU */
>  	for_each_present_cpu(cpu) {
> +#ifdef CONFIG_CRASH_HOTPLUG
> +		if (IS_ENABLED(CONFIG_HOTPLUG_CPU)) {
> +			/* Skip the soon-to-be offlined cpu */
> +			if ((image->hp_action == KEXEC_CRASH_HP_REMOVE_CPU) &&
> +				(cpu == image->offlinecpu))
> +				continue;
> +		}
> +#endif

I'm failing to see how the above is correct in any way. Look at the
following sequence of events:

     1) Offline CPU$N

        -> Prepare elf headers with CPU$N excluded

     2) Another hotplug operation != 'Online CPU$N'

        -> Prepare elf headers with CPU$N included

Also in case of loading the crash kernel in the situation where not all
present CPUs are online (think boot time SMT disable) then your
resulting crash image will contain all present CPUs and none of the
offline CPUs are excluded.

How does that make any sense at all?

This image->hp_action and image->offlinecpu dance is engineering
voodoo. You just can do:

        for_each_present_cpu(cpu) {
            if (!cpu_online(cpu))
            	continue;
            do_stuff(cpu);

which does the right thing in all situations and can be further
simplified to:

        for_each_online_cpu(cpu) {
            do_stuff(cpu);

without the need for ifdefs or whatever.

No?

Thanks,

        tglx