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

[Radim KrÄmÃÅ]

2016-11-23 22:58+0100, Paolo Bonzini:
> On 23/11/2016 21:25, Radim KrÄmÃÅ wrote:
>> diff --git a/arch/x86/kvm/irq_comm.c b/arch/x86/kvm/irq_comm.c
>> index 25810b144b58..ddd63b8b176e 100644
>> --- a/arch/x86/kvm/irq_comm.c
>> +++ b/arch/x86/kvm/irq_comm.c
>> @@ -41,6 +41,15 @@ static int kvm_set_pic_irq(struct kvm_kernel_irq_routing_entry *e,
>>  			   bool line_status)
>>  {
>>  	struct kvm_pic *pic = pic_irqchip(kvm);
>> +
>> +	/*
>> +	 * XXX: rejecting pic routes when pic isn't in use would be better,
>> +	 * but the default routing table is installed while kvm->arch.vpic is
>> +	 * NULL and KVM_CREATE_IRQCHIP can race with KVM_SET_GSI_ROUTING.
>> +	 */
>> +	if (!pic)
>> +		return -1;
>> +
>>  	return kvm_pic_set_irq(pic, e->irqchip.pin, irq_source_id, level);
>>  }
>>  
> 
> Can you explain the race with the default routing table better?  It
> seems to me that it can only make the routing table go from invalid to
> valid (there is no KVM_DESTROY_IRQCHIP) so it's benign.

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).  And adding a new
variable for irqchip_in_kernel() would allow us to make the pic
condition reasonabled.

I'll do something like that for 4.10, but the current patch is better
suited for stable.

Would fixing the comment be enough?

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)