linux-kernel - Re: [PATCH v3 2/3] KVM: x86: Use kvzalloc() to allocate VM struct

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: <aC4JZ4ztJiFGVMkB@google.com>
Date: Wed, 21 May 2025 10:12:07 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Kai Huang <kai.huang@...el.com>
Cc: "kvm@...r.kernel.org" <kvm@...r.kernel.org>, "pbonzini@...hat.com" <pbonzini@...hat.com>, 
	"vipinsh@...gle.com" <vipinsh@...gle.com>, 
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 2/3] KVM: x86: Use kvzalloc() to allocate VM struct

On Tue, May 20, 2025, Kai Huang wrote:
> On Tue, 2025-05-20 at 16:11 -0700, Sean Christopherson wrote:
> > On Tue, May 20, 2025, Kai Huang wrote:
> > > On Mon, 2025-05-19 at 08:39 -0700, Sean Christopherson wrote:
> > > > +static int tdx_sept_remove_private_spte(struct kvm *kvm, gfn_t gfn,
> > > > +					enum pg_level level, kvm_pfn_t pfn)
> > > >  {
> > > >  	struct page *page = pfn_to_page(pfn);
> > > >  	int ret;
> > > > @@ -3507,10 +3507,14 @@ int __init tdx_bringup(void)
> > > >  	r = __tdx_bringup();
> > > >  	if (r) {
> > > >  		/*
> > > > -		 * Disable TDX only but don't fail to load module if
> > > > -		 * the TDX module could not be loaded.  No need to print
> > > > -		 * message saying "module is not loaded" because it was
> > > > -		 * printed when the first SEAMCALL failed.
> > > > +		 * Disable TDX only but don't fail to load module if the TDX
> > > > +		 * module could not be loaded.  No need to print message saying
> > > > +		 * "module is not loaded" because it was printed when the first
> > > > +		 * SEAMCALL failed.  Don't bother unwinding the S-EPT hooks or
> > > > +		 * vm_size, as kvm_x86_ops have already been finalized (and are
> > > > +		 * intentionally not exported).  The S-EPT code is unreachable,
> > > > +		 * and allocating a few more bytes per VM in a should-be-rare
> > > > +		 * failure scenario is a non-issue.
> > > >  		 */
> > > >  		if (r == -ENODEV)
> > > >  			goto success_disable_tdx;
> > > > @@ -3524,3 +3528,19 @@ int __init tdx_bringup(void)
> > > >  	enable_tdx = 0;
> > > >  	return 0;
> > > >  }
> > > > +
> > > > +
> > > > +void __init tdx_hardware_setup(void)
> > > > +{
> > > > +	/*
> > > > +	 * Note, if the TDX module can't be loaded, KVM TDX support will be
> > > > +	 * disabled but KVM will continue loading (see tdx_bringup()).
> > > > +	 */
> > > 
> > > This comment seems a little bit weird to me.  I think what you meant here is the
> > > @vm_size and those S-EPT ops are not unwound while TDX cannot be brought up but
> > > KVM is still loaded.
> > 
> > This comment is weird?  Or the one in tdx_bringup() is weird?  
> > 
> 
> I definitely agree tdx_bringup() is weird :-)
> 
> > The sole intent of _this_ comment is to clarify that KVM could still end up
> > running load with TDX disabled.  
> > 
> 
> But this behaviour itself doesn't mean anything,

I disagree.  The overwhelming majority of code in KVM expects that either the
associated feature will be fully enabled, or KVM will abort the overall flow,
e.g. refuse to load, fail vCPU/VM creation, etc.

Continuing on is very exceptional IMO, and warrants a comment.

> e.g., if we export kvm_x86_ops, we could unwind them.

Maaaybe.  I mean, yes, we could fully unwind kvm_x86_ops, but doing so would make
the overall code far more brittle.  E.g. simply updating kvm_x86_ops won't suffice,
as the static_calls also need to be patched, and we would have to be very careful
not to touch anything in kvm_x86_ops that might have been consumed between here
and the call to tdx_bringup().

> So without mentioning "those are not unwound", it doesn't seem useful to me.
> 
> But it does have "(see tdx_bringup())" at the end, so OK to me.  I guess I just
> wish it could be more verbose.

Yeah, redirecting to another comment isn't a great experience for readers, but I
don't want to duplicate the explanation and details because that risks creating
stale and/or contradicting comments in the future, and in general increases the
maintenance cost (small though it should be in this case).