linux-kernel - Re: [PATCH v3] x86/acpi: fix panic while AP online later with kernel parameter maxcpus=1

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: Tue, 2 Jul 2024 14:45:24 +0200
From: Borislav Petkov <bp@...en8.de>
To: "Huang, Kai" <kai.huang@...el.com>
Cc: "tglx@...utronix.de" <tglx@...utronix.de>,
	"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
	"Li, Zhiquan1" <zhiquan1.li@...el.com>,
	"mingo@...hat.com" <mingo@...hat.com>,
	"x86@...nel.org" <x86@...nel.org>,
	"dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
	"hpa@...or.com" <hpa@...or.com>,
	"rafael@...nel.org" <rafael@...nel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH v3] x86/acpi: fix panic while AP online later with kernel
 parameter maxcpus=1

On Tue, Jul 02, 2024 at 12:05:38PM +0000, Huang, Kai wrote:
> On Tue, 2024-07-02 at 08:58 +0800, Zhiquan Li wrote:
> > The issue was found on the platform that using "Multiprocessor Wakeup
> > Structure"[1] to startup secondary CPU, which is usually used by
> > encrypted guest.  When restrict boot time CPU to 1 with the kernel
> > parameter "maxcpus=1" and bring other CPUs online later, there will be
> > a kernel panic.
> > 
> > The variable acpi_mp_wake_mailbox, which holds the virtual address of
> > the MP Wakeup Structure mailbox, will be set as read-only after init.
> > If the first AP gets online later, after init, the attempt to update
> > the variable results in panic.
> > 
> > The memremap() call that initializes the variable cannot be moved into
> > acpi_parse_mp_wake() because memremap() is not functional at that point
> > in the boot process.
> > 
> > [1] Details about the MP Wakeup structure can be found in ACPI v6.4, in
> >     the "Multiprocessor Wakeup Structure" section.
> > 
> > Signed-off-by: Zhiquan Li <zhiquan1.li@...el.com>
> > Reviewed-by: Kirill A. Shutemov <kirill.shutemov@...ux.intel.com>
> 
> Seems this changelog only mentions the problem, but doesn't say how to fix:
> 
>   Remove the __ro_after_init annotation of acpi_mp_wake_mailbox to fix.

Do not talk about *what* the patch is doing in the commit message - that
should be obvious from the diff itself. Rather, concentrate on the *why*
it needs to be done.

Imagine one fine day you're doing git archeology, you find the place in
the code about which you want to find out why it was changed the way it 
is now.

You do git annotate <filename> ... find the line, see the commit id and
you do:

git show <commit id>

You read the commit message and there's just gibberish and nothing's
explaining *why* that change was done. And you start scratching your
head, trying to figure out why. Because the damn commit message is worth
sh*t.

This happens to us maintainers at least once a week. Well, I don't want
that to happen in my tree anymore.

So none of this text above still doesn't explain to me *why* this is
happening.

Why do APs need to update acpi_mp_wake_mailbox?

Which patch is this fixing?

See Documentation/process/submitting-patches.rst

Questions over questions...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette