linux-kernel - Re: [PATCH v2 01/19] KVM: x86: Drop kvm_user_return_msr_cpu

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]

Date:   Thu, 1 Sep 2022 10:49:53 -0700
From:   Isaku Yamahata <isaku.yamahata@...il.com>
To:     Sean Christopherson <seanjc@...gle.com>
Cc:     Chao Gao <chao.gao@...el.com>, isaku.yamahata@...el.com,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
        isaku.yamahata@...il.com, Paolo Bonzini <pbonzini@...hat.com>,
        Kai Huang <kai.huang@...el.com>, Will Deacon <will@...nel.org>
Subject: Re: [PATCH v2 01/19] KVM: x86: Drop kvm_user_return_msr_cpu_online()

On Thu, Sep 01, 2022 at 02:12:56PM +0000,
Sean Christopherson <seanjc@...gle.com> wrote:

> On Thu, Sep 01, 2022, Chao Gao wrote:
> > On Tue, Aug 30, 2022 at 05:01:16AM -0700, isaku.yamahata@...el.com wrote:
> > >From: Isaku Yamahata <isaku.yamahata@...el.com>
> > >
> > >KVM/X86 uses user return notifier to switch MSR for guest or user space.
> > >Snapshot host values on CPU online, change MSR values for guest, and
> > >restore them on returning to user space.  The current code abuses
> > >kvm_arch_hardware_enable() which is called on kvm module initialization or
> > >CPU online.
> > >
> > >Remove such the abuse of kvm_arch_hardware_enable by capturing the host
> > >value on the first change of the MSR value to guest VM instead of CPU
> > >online.
> > >
> > >Suggested-by: Sean Christopherson <seanjc@...gle.com>
> > >Signed-off-by: Isaku Yamahata <isaku.yamahata@...el.com>
> > >---
> > > arch/x86/kvm/x86.c | 43 ++++++++++++++++++++++++-------------------
> > > 1 file changed, 24 insertions(+), 19 deletions(-)
> > >
> > >diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > >index 205ebdc2b11b..16104a2f7d8e 100644
> > >--- a/arch/x86/kvm/x86.c
> > >+++ b/arch/x86/kvm/x86.c
> > >@@ -200,6 +200,7 @@ struct kvm_user_return_msrs {
> > > 	struct kvm_user_return_msr_values {
> > > 		u64 host;
> > > 		u64 curr;
> > >+		bool initialized;
> > > 	} values[KVM_MAX_NR_USER_RETURN_MSRS];
> > 
> > The benefit of having an "initialized" state for each user return MSR on
> > each CPU is small. A per-cpu state looks suffice. With it, you can keep
> > kvm_user_return_msr_cpu_online() and simply call the function from
> > kvm_set_user_return_msr() if initialized is false on current CPU.
> 
> Yep, a per-CPU flag is I intended.  This is the completely untested patch that's
> sitting in a development branch of mine.

With the following fix, it worked.  I'll replace this patch with yours.

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 205ebdc2b11b..0e200fe44b35 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9212,7 +9217,12 @@ int kvm_arch_init(void *opaque)
                return -ENOMEM;
        }
 
-       user_return_msrs = alloc_percpu(struct kvm_user_return_msrs);
+       /*
+        * __GFP_ZERO to ensure user_return_msrs.values[].initialized = false.
+        * See kvm_user_return_msr_init_cpu().
+        */
+       user_return_msrs = alloc_percpu_gfp(struct kvm_user_return_msrs,
+                                           GFP_KERNEL | __GFP_ZERO);
        if (!user_return_msrs) {
                printk(KERN_ERR "kvm: failed to allocate percpu kvm_user_return_msrs\n");
                r = -ENOMEM;

-- 
Isaku Yamahata <isaku.yamahata@...il.com>