From patchwork Thu Nov 24 12:42:06 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?b?UmFkaW0gS3LEjW3DocWZ?= X-Patchwork-Id: 9445417 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 46D206071C for ; Thu, 24 Nov 2016 12:43:51 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 478A6279E0 for ; Thu, 24 Nov 2016 12:43:51 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 35EEA27EED; Thu, 24 Nov 2016 12:43:51 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.9 required=2.0 tests=BAYES_00,HK_RANDOM_FROM, RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C53F52780C for ; Thu, 24 Nov 2016 12:43:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S941059AbcKXMnA (ORCPT ); Thu, 24 Nov 2016 07:43:00 -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 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 16EDC91D2F; Thu, 24 Nov 2016 12:42:10 +0000 (UTC) Received: from potion (dhcp-1-116.brq.redhat.com [10.34.1.116]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id uAOCg7KO009988; Thu, 24 Nov 2016 07:42:07 -0500 Received: by potion (sSMTP sendmail emulation); Thu, 24 Nov 2016 13:42:06 +0100 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-Disposition: inline In-Reply-To: <06c15e9d-43fc-0d08-57e4-26440c441eb0@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 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: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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? --- 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)