Re: [PATCH] KVM: x86: check for pic and ioapic presence before use

[Paolo Bonzini]

> Oops, I wrote the race with wrong IOCTL -- it should be KVM_IRQ_LINE.
> 
>  1) set KVM_CAP_SPLIT_IRQCHIP (unlocks KVM_IRQ_LINE)
>  a) call KVM_CREATE_IRQCHIP (creates routes while !kvm->arch.vpic)
>  b) concurrently call KVM_IRQ_LINE for PIO routes (dereferences NULL)
> 
> The problem is that we use pic_in_kernel() as irqchip_in_kernel(), so it
> cannot be set before we set up routes, but we then cannot reject routes
> when pic is not in use.  The best effort is to do this for pic routes in
> kvm_set_routing_entry():
> 
>  // initialization is the only place where pic_in_kernel() !=
>  ioapic_in_kernel()
>  if (!pic_in_kernel(kvm) && !ioapic_in_kernel(kvm))
>  	goto out;
> 
> and similar for ioapic routes:
> 
>  if (!ioapic_in_kernel(kvm))
>  	goto out;
> 
> I think it would work if we forbade KVM_CREATE_IRQCHIP after
> KVM_CAP_SPLIT_IRQCHIP (which we want to do anyway).

Yeah, definitely.

> And adding a new
> variable for irqchip_in_kernel() would allow us to make the pic
> condition reasonabled.

Or change kvm->arch.irqchip_split to an enum.

> I'll do something like that for 4.10, but the current patch is better
> suited for stable.
> 
> Would fixing the comment be enough?

Yes, fine!

> Do you want the following hunk already in 4.9?
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 6f9c9ad13f88..dbed51045c37 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -3901,7 +3901,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
>  
>  		mutex_lock(&kvm->lock);
>  		r = -EEXIST;
> -		if (kvm->arch.vpic)
> +		if (irqchip_in_kernel(kvm))
>  			goto create_irqchip_unlock;
>  		r = -EINVAL;
>  		if (kvm->created_vcpus)

No, it's unnecessary.

Paolo