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
| ||
|
Date: Fri, 15 Apr 2022 15:00:34 +0000 From: Sean Christopherson <seanjc@...gle.com> To: Zeng Guang <guang.zeng@...el.com> Cc: Paolo Bonzini <pbonzini@...hat.com>, Vitaly Kuznetsov <vkuznets@...hat.com>, Wanpeng Li <wanpengli@...cent.com>, Jim Mattson <jmattson@...gle.com>, Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org, Dave Hansen <dave.hansen@...ux.intel.com>, Tony Luck <tony.luck@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>, "H. Peter Anvin" <hpa@...or.com>, Kim Phillips <kim.phillips@....com>, Jarkko Sakkinen <jarkko@...nel.org>, Jethro Beekman <jethro@...tanix.com>, Kai Huang <kai.huang@...el.com>, x86@...nel.org, linux-kernel@...r.kernel.org, Robert Hu <robert.hu@...el.com>, Gao Chao <chao.gao@...el.com>, Marc Zyngier <maz@...nel.org>, James Morse <james.morse@....com>, Alexandru Elisei <alexandru.elisei@....com>, Suzuki K Poulose <suzuki.poulose@....com>, Christian Borntraeger <borntraeger@...ux.ibm.com>, Janosch Frank <frankja@...ux.ibm.com>, Claudio Imbrenda <imbrenda@...ux.ibm.com>, David Hildenbrand <david@...hat.com> Subject: Re: [PATCH v8 7/9] KVM: Move kvm_arch_vcpu_precreate() under kvm->lock Heh, lot's of people cc'd, but none of the people who's code this affects. +s390 and arm folks On Mon, Apr 11, 2022, Zeng Guang wrote: > Arch specific KVM common data may require pre-allocation or other > preprocess ready before vCPU creation at runtime. Please provide the specific motivation for the move, i.e. explain the desire to do per-VM allocations based on max_vcpu_ids at the first vCPU creation. > It's safe to invoke kvm_arch_vcpu_precreate() within the protection of > kvm->lock directly rather than take into account in the implementation for > each architecture. This absolutely needs to explain _why_ it's safe, e.g. only arm64, x86, and s390 have non-nop implementations and they're all simple and short with no tendrils into other code that might take kvm->lock. And as before, I suspect arm64 needs this protection, the vgic_initialized() check looks racy. Though it's hard to tell if doing the check under kvm->lock actually fixes anything. > Suggested-by: Sean Christopherson <seanjc@...gle.com> > Signed-off-by: Zeng Guang <guang.zeng@...el.com> > --- > arch/s390/kvm/kvm-s390.c | 2 -- > virt/kvm/kvm_main.c | 2 +- I think it's also worth changing x86's implementation to check created_vcpus instead of online_vcpus. That'll fix a race where userspace could never see the pr_warn() (which is arguably useless, but whatever), e.g. if it creates a VM with 2 vCPUs and both simultaneously go through kvm_arch_vcpu_precreate(). > 2 files changed, 1 insertion(+), 3 deletions(-) > > diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c > index 156d1c25a3c1..5c795bbcf1ea 100644 > --- a/arch/s390/kvm/kvm-s390.c > +++ b/arch/s390/kvm/kvm-s390.c > @@ -3042,9 +3042,7 @@ static int sca_can_add_vcpu(struct kvm *kvm, unsigned int id) > if (!sclp.has_esca || !sclp.has_64bscao) > return false; > > - mutex_lock(&kvm->lock); > rc = kvm->arch.use_esca ? 0 : sca_switch_to_extended(kvm); > - mutex_unlock(&kvm->lock); > > return rc == 0 && id < KVM_S390_ESCA_CPU_SLOTS; > } > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index 70e05af5ebea..a452e678a015 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -3732,9 +3732,9 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id) > } > > kvm->created_vcpus++; > + r = kvm_arch_vcpu_precreate(kvm, id); Hmm, so I think I'd prefer this to be invoked before bumping created_vcpus. The existing implementation don't reference created_vcpus, so there's no change needed to existing code. Logically, a pre-create helper should not see a non-zero count as the "pre" part strongly implies it's being called _before_ creating the first vCPU. Then switching from online_vcpus to created_vcpus in the x86 implementation doesn't need to have a wierd change from "> 0" => "> 1". Ah, and then it also wouldn't have goofy behavior where it drops and reacquires kvm->lock on failure just to decrement created_vcpus. > mutex_unlock(&kvm->lock); > > - r = kvm_arch_vcpu_precreate(kvm, id); > if (r) > goto vcpu_decrement; > > -- > 2.27.0 >
Powered by blists - more mailing lists