From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <stable-owner@vger.kernel.org>
Received: from mx4-phx2.redhat.com ([209.132.183.25]:44768 "EHLO
        mx4-phx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1751644AbcKXQes (ORCPT
        <rfc822;stable@vger.kernel.org>); Thu, 24 Nov 2016 11:34:48 -0500
Date: Thu, 24 Nov 2016 11:34:46 -0500 (EST)
From: Paolo Bonzini <pbonzini@redhat.com>
To: Radim =?utf-8?B?S3LEjW3DocWZ?= <rkrcmar@redhat.com>
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        stable@vger.kernel.org, Dmitry Vyukov <dvyukov@google.com>,
        Steve Rutherford <srutherford@google.com>
Message-ID: <2119448151.1732997.1480005286539.JavaMail.zimbra@redhat.com>
In-Reply-To: <20161124124206.GA16974@potion>
References: <20161123202548.29324-1-rkrcmar@redhat.com> <06c15e9d-43fc-0d08-57e4-26440c441eb0@redhat.com> <20161124124206.GA16974@potion>
Subject: Re: [PATCH] KVM: x86: check for pic and ioapic presence before use
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Sender: stable-owner@vger.kernel.org
List-ID: <stable.vger.kernel.org>


> 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