From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mark Kanda Subject: Re: [PATCH] KVM: nVMX: Eliminate vmcs02 pool Date: Mon, 27 Nov 2017 14:04:54 -0600 Message-ID: References: <0dd4862d-f1dd-bd08-a91d-97060c7b5f42@oracle.com> <3f88befc-a296-a8a7-f2d4-28afd15d632e@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Cc: rkrcmar@redhat.com, ameya.more@oracle.com, Jim Mattson To: Paolo Bonzini , kvm@vger.kernel.org Return-path: Received: from aserp1040.oracle.com ([141.146.126.69]:31818 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751681AbdK0UEH (ORCPT ); Mon, 27 Nov 2017 15:04:07 -0500 In-Reply-To: <3f88befc-a296-a8a7-f2d4-28afd15d632e@redhat.com> Content-Language: en-US Sender: kvm-owner@vger.kernel.org List-ID: On 11/27/2017 11:51 AM, Paolo Bonzini wrote: > On 27/11/2017 18:43, Mark Kanda wrote: >> On 11/23/2017 5:46 PM, Paolo Bonzini wrote: >>> On 21/11/2017 18:22, Mark Kanda wrote: >>>> -    nested_free_all_saved_vmcss(vmx); >>>> +    free_loaded_vmcs(&vmx->nested.vmcs02); >>> >>> Please add to free_loaded_vmcs a WARN that the passed vmcs is not >>> vmx->loaded_vmcs. >> >> Sure. >> >> However, I don't see a way to access vmx from the passed in vmcs, which >> would necessitate passing in vmx for the WARN (multiple callers) - I may >> be missing something.. > > free_loaded_vmcs is only ever used on VMCS02's, so you can change it to Perhaps I'm missing something, but it seems the free_loaded_vmcs() use in vmx_create_vcpu() (and perhaps vmx_free_vcpu()) is for VMCS01 ..correct? If so, I guess I shouldn't rename the routine to be VMCS02 specific (but I think it's fine otherwise). Thanks, -Mark > static void vmx_nested_free_vmcs02(struct vcpu_vmx *vmx) > { > struct loaded_vmcs *loaded_vmcs = &vmx->nested.vmcs02; > > /* > * Just leak the VMCS02 if the WARN triggers. Better than > * a use-after-free. > */ > if (WARN_ON(vmx->loaded_vmcs == loaded_vmcs)) > return; > ... > } > >> I'm happy to do this, but it seems possibly excessive for the sole >> purpose of adding the WARN. Thoughts? > > We've had this kind of bug in the past, so I prefer to err on the side > of safety. > > Paolo >