linux-kernel - Re: [PATCH] KVM: VMX: Make setup_vmcs

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: <3aec157afb6727e603d80c2232b3718033295f13.camel@intel.com>
Date:   Thu, 2 Mar 2023 08:39:36 +0000
From:   "Huang, Kai" <kai.huang@...el.com>
To:     "Gao, Chao" <chao.gao@...el.com>
CC:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] KVM: VMX: Make setup_vmcs_config() preemption disabled

On Thu, 2023-03-02 at 13:36 +0800, Gao, Chao wrote:
> On Wed, Mar 01, 2023 at 11:54:38PM +1300, Kai Huang wrote:
> > Make setup_vmcs_config() preemption disabled so it always performs on
> > the same local cpu.
> > 
> > During module loading time, KVM intends to call setup_vmcs_config() to
> > set up the global VMCS configurations on _one_ cpu in hardware_setup(),
> > and then calls setup_vmcs_config() on all other online cpus via sending
> 
> *all other* is misleading. The compatibility check is actually done on
> *all* online cpus.
> 
>         for_each_online_cpu(cpu) {
>                 smp_call_function_single(cpu, kvm_x86_check_cpu_compat, &r, 1);
>                 if (r < 0)
>                         goto out_unwind_ops;
>         }
> 
> Given this, it probably is ok to not disable preemption because all CPUs
> are guaranteed to be compatible later in the flow in terms of VMCS
> capabilities. But we don't want to have such a subtle dependency.
> 
> Do you see any real problem with preemption enabled?

No.  Just thought it is the right thing to do.

> 
> > IPI to perform VMX compatibility check.  Further more, KVM has CPU
> > hotplug callback to call setup_vmcs_config() to do compatibility check
> > on the "new-online" cpu to make sure it is compatible too.
> > 
> > setup_vmcs_config() is supposed to be done on the same cpu.  This is
> > true in the compatibility check code path as setup_vmcs_config() is
> > called either via IPI or in per-cpu CPU hotplug thread.  However, the
> > first call from hardware_setup() isn't as it is called when preemption
> > is enabled.
> > 
> > Change the existing setup_vmcs_config() to __setup_vmcs_config() and
> > call the latter directly in the compatibility check code path.  Change
> > setup_vmcs_config() to call __setup_vmcs_config() with preemption
> > disabled so __setup_vmcs_config() is always done on the same cpu.
> 
> Maybe you can simply disable preemption in hardware_setup() although I
> don't have a strong preference.
> 
> nested_vmx_setup_ctls_msrs() also reads some MSRs and sets up part of
> vmcs_conf, should it be called on the same CPU as setup_vmcs_config()?

Yes I think so.  I missed this :)

Not sure whether there are other similar places too even outside of
hardware_setup().

But compatibility check only checks things calculated via setup_vmcs_config()
and nested_vmx_setup_ctls_msrs(), so I think it's fair to only put
hardware_setup() inside preemption disabled.