linux-kernel - RE: [RFC PATCH v2 4/5] KVM: x86: Remove KVM_X86_OP

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: <DS0PR11MB6373D059F2BB9F196AA9D503DC0D2@DS0PR11MB6373.namprd11.prod.outlook.com>
Date: Fri, 19 Apr 2024 15:12:27 +0000
From: "Wang, Wei W" <wei.w.wang@...el.com>
To: Sean Christopherson <seanjc@...gle.com>
CC: "pbonzini@...hat.com" <pbonzini@...hat.com>, "kvm@...r.kernel.org"
	<kvm@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [RFC PATCH v2 4/5] KVM: x86: Remove KVM_X86_OP_OPTIONAL

On Friday, April 19, 2024 9:42 PM, Sean Christopherson wrote:
> On Fri, Apr 19, 2024, Wei Wang wrote:
> > KVM_X86_OP and KVM_X86_OP_OPTIONAL were utilized to define and
> execute
> > static_call_update() calls on mandatory and optional hooks, respectively.
> > Mandatory hooks were invoked via static_call() and necessitated
> > definition due to the presumption that an undefined hook (i.e., NULL)
> > would cause
> > static_call() to fail. This assumption no longer holds true as
> > static_call() has been updated to treat a "NULL" hook as a NOP on x86.
> > Consequently, the so-called mandatory hooks are no longer required to
> > be defined, rendering them non-mandatory.
> 
> This is wrong.  They absolutely are mandatory.  The fact that static_call()
> doesn't blow up doesn't make them optional.  If a vendor neglects to
> implement a mandatory hook, KVM *will* break, just not immediately on the
> static_call().
> 
> The static_call() behavior is actually unfortunate, as KVM at least would prefer
> that it does explode on a NULL point.  I.e. better to crash the kernel (hopefully
> before getting to production) then to have a lurking bug just waiting to cause
> problems.
> 
> > This eliminates the need to differentiate between mandatory and
> > optional hooks, allowing a single KVM_X86_OP to suffice.
> >
> > So KVM_X86_OP_OPTIONAL and the WARN_ON() associated with
> KVM_X86_OP
> > are removed to simplify usage,
> 
> Just in case it isn't clear, I am very strongly opposed to removing
> KVM_X86_OP_OPTIONAL() and the WARN_ON() protection to ensure
> mandatory ops are implemented.

OK, we can drop patch 4 and 5.

Btw, may I know what is the boundary between mandatory and optional hooks?
For example, when adding a new hook, what criteria should we use to determine
whether it's mandatory, thereby requiring both SVM and VMX to implement it (and
seems need to be merged them together?)
(I searched a bit, but didn't find it)