On Wed, Dec 10, 2014 at 01:35:08PM +0100, Eric Auger wrote: > On 12/09/2014 04:44 PM, Christoffer Dall wrote: > > When the vgic initializes its internal state it does so based on the > > number of VCPUs available at the time. If we allow KVM to create more > > VCPUs after the VGIC has been initialized, we are likely to error out in > > unfortunate ways later, perform buffer overflows etc. > > > > Cc: Eric Auger <eric.auger@xxxxxxxxxx> > > Signed-off-by: Christoffer Dall <christoffer.dall@xxxxxxxxxx> > > --- > > This replaces Eric Auger's previous patch > > (https://lists.cs.columbia.edu/pipermail/kvmarm/2014-December/012646.html), > > because it fits better with testing to include it in this series and I > > realized that we need to add a check against irqchip_in_kernel() as > > well. > > > > arch/arm/kvm/arm.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c > > index a9d005f..d4da244 100644 > > --- a/arch/arm/kvm/arm.c > > +++ b/arch/arm/kvm/arm.c > > @@ -213,6 +213,11 @@ struct kvm_vcpu *kvm_arch_vcpu_create(struct kvm *kvm, unsigned int id) > > int err; > > struct kvm_vcpu *vcpu; > > > > + if (irqchip_in_kernel(kvm) && vgic_initialized(kvm)) { > Reviewed-by: Eric Auger <eric.auger@xxxxxxxxxx> > a question about that irqchip_in_kernel(kvm): > kvm->arch.vgic.in_kernel is set in kvm_vgic_create but nobody resets it, > especially in destroy, am i wrong? no, because we don't allow creating a vgic in the kernel for a VM and then letting the VM go back to having a userspace driven gic. > if the vgic is initialized shouldn't it be also created? Shouldn't we > test irqchip_in_kernel in vgic_init instead? no, vgic_init will never be called if you didn't create a vgic, and irqchip_in_kernel() should always return false in that case. If you can find a flow where this breaks, please let me know, because then it's a bug, but it looks right to me. > Also in case we need irqchip_in_kernel(kvm) here we might need it also > in kvm_vgic_inject_irq because dist->lock is grabbed in > vgic_update_irq_pending. > Huh, you're right about that. In fact, I don't think we should allow initializing the arch timers if userspace didn't create an in-kernel irqchip, avoiding the call path alltogether. We probaby need to add that to this series. Unless I missed something obvious here: Nice catch! Thanks, -Christoffer -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
