linux-kernel - Re: [PATCH v4 14/19] KVM: SVM: Check that the current CPU supports SVM in kvm_is_svm

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: <ZL8O5BiKLbbfmYlU@google.com>
Date:   Mon, 24 Jul 2023 16:53:08 -0700
From:   Sean Christopherson <seanjc@...gle.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc:     Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        Paolo Bonzini <pbonzini@...hat.com>,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        Andrew Cooper <Andrew.Cooper3@...rix.com>,
        Kai Huang <kai.huang@...el.com>, Chao Gao <chao.gao@...el.com>
Subject: Re: [PATCH v4 14/19] KVM: SVM: Check that the current CPU supports
 SVM in kvm_is_svm_supported()

On Mon, Jul 24, 2023, Dmitry Torokhov wrote:
> Hi Sean,
> 
> On Fri, Jul 21, 2023 at 01:18:54PM -0700, Sean Christopherson wrote:
> > +static bool kvm_is_svm_supported(void)
> > +{
> > +	bool supported;
> > +
> > +	migrate_disable();
> > +	supported = __kvm_is_svm_supported();
> > +	migrate_enable();
> 
> I am typically very wary of the constructs like this, as the value
> returned is obsolete the moment migrate_enable() happens.

Yeah, I don't like this code, but there's no great solution in this case.  Or at
least, none that I've found.  At some point KVM has to enable migration/preemption
before "is KVM supported?" is ultimately consumed, as the real consumer is
userspace.

> Is value of "svm was supported at some time in the past but may or may not be
> supported right now" useful and if it is then could you add comment why?

No, because barring fatal silicon/ucode/kernel bugs, SVM support isn't expected
to disappear or (re)appear).

KVM defends against the "disappear" case as much as can be reasonably expected.
It's ugly, but functionally ok (not perfect, but ok).  KVM doesn't actually care
which CPU does the initial support check, because KVM will do fully protected support
checks on all CPUs before actually letting userspace create VMs.  This is why the
changelog states that ensuring a stable CPU is a non-goal, and also why the inner
helpers don't use the raw accessors.

The "(re)appear" case doesn't need to be handled, because userspace could simply
retry if it really wanted to (but that would be quite insane/nonsensical, and
just asking for problems).

I didn't add a comment because VMX uses the exact same pattern, and I didn't
want to copy+paste a non-trivial comment.  And this is a single use local helper,
so I'm not terribly concerned about it being misused.

That said, I'll see if I can find a common, intuitive location to document this.