Re: [PATCH 07/29] nVMX: Hold a vmcs02 for each vmcs12

From: Avi Kivity <avi@redhat.com>
To: "Nadav Har'El" <nyh@math.technion.ac.il>
Cc: kvm@vger.kernel.org, gleb@redhat.com
Subject: Re: [PATCH 07/29] nVMX: Hold a vmcs02 for each vmcs12
Date: Mon, 31 Jan 2011 11:41:19 +0200	[thread overview]
Message-ID: <4D4683BF.8050702@redhat.com> (raw)
In-Reply-To: <20110131092629.GB23022@fermat.math.technion.ac.il>

On 01/31/2011 11:26 AM, Nadav Har'El wrote:
> Hi,
>
> On Sun, Jan 30, 2011, Avi Kivity wrote about "Re: [PATCH 07/29] nVMX: Hold a vmcs02 for each vmcs12":
> >  >+/*
> >  >+ * Allocate an L0 VMCS (vmcs02) for the current L1 VMCS (vmcs12), if one
> >  >+ * does not already exist. The allocation is done in L0 memory, so to
> >  >avoid
> >  >+ * denial-of-service attack by guests, we limit the number of
> >  >concurrently-
> >  >+ * allocated vmcss. A well-behaving L1 will VMCLEAR unused vmcs12s and not
> >  >+ * trigger this limit.
> >
> >  No, it won't.  If you run N guests on a single-cpu kvm host, you'll have
> >  N active VMCSs.
>
> Of course. What I said was that *unused* vmcs12s (in the sense that they
> don't describe any active guest) will normally be unloaded (VMCLEARed) by L1
> and so will not take up space. Only VMCSs actually being used to run guests
> will take up space. If you have N running guests, then right, you'll have
> N VMCSs. I put the limit at 256, which on one hand allows L1 to run 256 L2s
> (which I think is well above what people will normally run on one CPU), and
> on the other hand limits the amount of damage that a malicious L1 can do:
> At worst it can cause the host to allocate (and pin) 256 extra VMCSs, which
> sum up to 1 MB.
>
> I thought that this compromise was good enough, and you didn't, and although
> I still don't understand why, I promise I will change it. I'll make this
> change my top priority now.

VM crashes on legal guest behaviour are bad; and since it's easy to 
avoid, why do it?

> >  >+static void __nested_free_saved_vmcs(void *arg)
> >  >+{
> >  >+	struct saved_vmcs *saved_vmcs = arg;
> >  >+	int cpu = raw_smp_processor_id();
> >  >+
> >  >+	if (saved_vmcs->cpu == cpu) /* TODO: how can this not be the case? */
> >  >+		vmcs_clear(saved_vmcs->vmcs);
> >
> >  This check will always be true.
>
> This is what I thought too... I call this function on the saved_vmcs->cpu
> cpu, so there's no reason why it would find itself being called on a different
> cpu.
>
> The only reason I added this sanity check was that __vcpu_clear has the same
> one, and there too it seemed redundant, and I thought maybe I was missing
> something. Do you know why __vcpu_clear needs this test?

__vcpu_clear() can race with itself (called from the cpu migration path 
vs. cpu offline path)

-- 
error compiling committee.c: too many arguments to function