| Message ID | 20161124124206.GA16974@potion (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-11-24 20:42 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > 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) If we should not go through irqfd if irqchip is split? Regards, Wanpeng Li -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-12-20 19:59+0800, Wanpeng Li: > 2016-11-24 20:42 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: >> 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) > > If we should not go through irqfd if irqchip is split? I also remember hearing about that -- do you remember where it was? The documentation does not say that and irqfd is mostly optimization for KVM_IRQ_LINE ... QEMU uses KVM_IRQ_LINE_STATUS with split irqchip, so we can't easily say the opposite now. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2016-12-21 20:44 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > 2016-12-20 19:59+0800, Wanpeng Li: >> 2016-11-24 20:42 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: >>> 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) >> >> If we should not go through irqfd if irqchip is split? > > I also remember hearing about that -- do you remember where it was? Not sure. :) > > The documentation does not say that and irqfd is mostly optimization for > KVM_IRQ_LINE ... QEMU uses KVM_IRQ_LINE_STATUS with split irqchip, so we > can't easily say the opposite now. How irqfd optimizes KVM_IRQ_LINE_STATUS? I didn't observe that they have relationship. Regards, Wanpeng Li -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)