linux-kernel - Re: [PATCH v3 1/5] x86/kexec: do unconditional WBINVD for bare-metal in stop_this

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]

Message-ID: <cdcae76739811b452d66e17554c9b324433a2f1e.camel@intel.com>
Date: Thu, 11 Apr 2024 13:53:38 +0000
From: "Huang, Kai" <kai.huang@...el.com>
To: "kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>
CC: "luto@...nel.org" <luto@...nel.org>, "ashish.kalra@....com"
	<ashish.kalra@....com>, "Hansen, Dave" <dave.hansen@...el.com>,
	"thomas.lendacky@....com" <thomas.lendacky@....com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"mingo@...hat.com" <mingo@...hat.com>, "seanjc@...gle.com"
	<seanjc@...gle.com>, "pbonzini@...hat.com" <pbonzini@...hat.com>,
	"bhe@...hat.com" <bhe@...hat.com>, "tglx@...utronix.de" <tglx@...utronix.de>,
	"nik.borisov@...e.com" <nik.borisov@...e.com>, "hpa@...or.com"
	<hpa@...or.com>, "peterz@...radead.org" <peterz@...radead.org>, "Gao, Chao"
	<chao.gao@...el.com>, "bp@...en8.de" <bp@...en8.de>, "Edgecombe, Rick P"
	<rick.p.edgecombe@...el.com>, "x86@...nel.org" <x86@...nel.org>
Subject: Re: [PATCH v3 1/5] x86/kexec: do unconditional WBINVD for bare-metal
 in stop_this_cpu()

On Thu, 2024-04-11 at 16:31 +0300, Kirill A. Shutemov wrote:
> On Thu, Apr 11, 2024 at 09:54:13AM +1200, Huang, Kai wrote:
> > 
> > 
> > On 11/04/2024 2:12 am, Kirill A. Shutemov wrote:
> > > On Mon, Apr 08, 2024 at 12:44:54AM +1200, Kai Huang wrote:
> > > > TL;DR:
> > > 
> > > The commit message is waaay too verbose for no good reason. You don't
> > > really need to repeat all the history around this code.
> > 
> > Could you be more specific?
> > 
> > I was following Boris's suggestion to summerize all the discussion around
> > the "unconditional WBINVD" issue.
> > 
> > https://lore.kernel.org/linux-kernel/20240228110207.GCZd8Sr8mXHA2KTiLz@fat_crate.local/
> > 
> > I can try to improve if I can know specifically what should be trimmed down.
> 
> What about something like this:
> 
>   x86/mm: Do unconditional WBINVD in stop_this_cpu() for bare metal
> 
>   Both AMD SME and Intel TDX can leave caches in an incoherent state due to
>   memory encryption, which can lead to silent memory corruption during kexec. To
>   address this issue, it is necessary to flush the caches before jumping to the
>   second kernel.
> 
>   Previously, the kernel only performed WBINVD in stop_this_cpu() when SME
>   support was detected. To support TDX as well, instead of adding vendor-specific
>   checks, it is proposed to unconditionally perform WBINVD. Kexec() is a slow
>   path, and the additional WBINVD is acceptable for the sake of simplicity and
>   maintainability.
> 
>   It is important to note that WBINVD should only be done for bare-metal
>   scenarios, as TDX guests and SEV-ES/SEV-SNP guests may not handle unexpected
>   exceptions (#VE or #VC) caused by WBINVD.
> 
>   Historically, there were issues with unconditional WBINVD, leading to system
>   hangs or resets on different Intel systems. These issues were addressed by a
>   series of commits, culminating in the fix provided by commit 1f5e7eb7868e
>   ("x86/smp: Make stop_other_cpus() more robust").
> 
>   Further testing on problematic machines confirmed that the issues could not be
>   reproduced after applying the fix. Therefore, it is now safe to unconditionally
>   perform WBINVD in stop_this_cpu().
> 
> You can also add links to relevant threads as Link: tags.
> 

Hmm.. The last two paragraphs doesn't tell the background that the
"unconditional WBINVD" was the original way to do etc.  The changelog of commit
1f5e7eb7868e ("x86/smp: Make stop_other_cpus() more robust" (and the commit IDs
that it mentions) doesn't tell the full story either.

That means people will need to open all the Links to get the full information. 
I think it is against what Boris suggested.

Yeah I agree having a lengthy changelog is annoying sometimes, but for this
particular case we have a "TL;DR" so doesn't seem that bad to me. :-)

So for now I would like to keep the text after the "Note:" in my original
changelog, but I will use your first 3 paragraphs above to replace mine.