From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941073AbcKXMnB (ORCPT ); Thu, 24 Nov 2016 07:43:01 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60610 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S938625AbcKXMm5 (ORCPT ); Thu, 24 Nov 2016 07:42:57 -0500 Date: Thu, 24 Nov 2016 13:42:06 +0100 From: Radim =?utf-8?B?S3LEjW3DocWZ?= To: Paolo Bonzini Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, stable@vger.kernel.org, Dmitry Vyukov , Steve Rutherford Subject: Re: [PATCH] KVM: x86: check for pic and ioapic presence before use Message-ID: <20161124124206.GA16974@potion> References: <20161123202548.29324-1-rkrcmar@redhat.com> <06c15e9d-43fc-0d08-57e4-26440c441eb0@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <06c15e9d-43fc-0d08-57e4-26440c441eb0@redhat.com> X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Thu, 24 Nov 2016 12:42:10 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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)