linux-kernel - Re: [tip: x86/urgent] x86/sev: Do not touch VMSA pages during SNP guest memory kdump

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: <aCRfPTxaPvoqILq8@gmail.com>
Date: Wed, 14 May 2025 11:15:41 +0200
From: Ingo Molnar <mingo@...nel.org>
To: Borislav Petkov <bp@...en8.de>
Cc: linux-kernel@...r.kernel.org, linux-tip-commits@...r.kernel.org,
	Ashish Kalra <ashish.kalra@....com>,
	Pankaj Gupta <pankaj.gupta@....com>,
	Tom Lendacky <thomas.lendacky@....com>,
	Srikanth Aithal <sraithal@....com>, stable@...r.kernel.org,
	x86@...nel.org
Subject: Re: [tip: x86/urgent] x86/sev: Do not touch VMSA pages during SNP
 guest memory kdump

* Borislav Petkov <bp@...en8.de> wrote:

> On Wed, May 14, 2025 at 09:20:58AM +0200, Ingo Molnar wrote:
> > Boris, please don't rush these SEV patches without proper review first! ;-)
> 
> You didn't read the R-by and SOB tags at the beginning?

Reviewed-by tags and SOB tags don't necessarily imply a proper review, 
as my review feedback here amply demonstrates.

> Feel free to propose fixes, Tom and I will review them and even test 
> them for you!
> 
> But ontop of those: those are fixes and the "issues" you've pointed 
> out are to existing code which this patch only moves.

Firstly, while you may be inclined to ignore the half dozen typos in 
the changelog and the comments as inconsequential, do your scare-quotes 
around 'issues' imply that you don't accept the other issues my review 
identified, such as the messy type conversions and the inconsistent 
handling of svsm_caa_pa as valid? That would be sad.

Secondly, the fact that half of the patch is moving/refactoring code, 
while the other half is adding new code is no excuse to ignore review 
feedback for the code that gets moved/refactored - reviewers obviously 
need to read and understand the code that gets moved too. This is 
kernel maintenance 101.

And the new functionality introduced obviously expands on the bad 
practices & fragile code I outlined.

This is a basic requirement when implementing new functionality (and 
kdump never really worked on SEV-SNP I suppose, at least since August 
laste year, so it's basically new functionality), is to have a clean 
codebase it is extending, especially if the changes are so large:

   1 file changed, 158 insertions(+), 86 deletions(-)

All these problems accumulate and may result in fragility and bugs.

Third, this patch should have been split into two parts to begin with: 
the first one refactors the code into vmgexit_ap_control() and moves 
snp_set_vmsa() and snp_cleanup_vmsa() - and a second, smaller, easier 
to review patch that does the real changes. Right now the actual 
changes are hidden within the noise of code movement and refactoring.

> I would usually say "Thx" here but not this time.

Oh wow, you really don't take constructive criticism of patches very 
well. Review feedback isn't a personal attack against you. Please don't 
shoot the messenger.

Thanks,

	Ingo