Re: [PATCH v1 01/22] KVM: x86: race between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP

Peter Xu
2017-03-15T06:24:48Z

On Tue, Mar 14, 2017 at 02:34:29PM +0100, David Hildenbrand wrote:
> Avoid races between KVM_SET_GSI_ROUTING and KVM_CREATE_IRQCHIP by taking
> the kvm->lock when setting up routes.
>
> If KVM_CREATE_IRQCHIP fails, KVM_SET_GSI_ROUTING could have already set
> up routes pointing at pic/ioapic, being silently removed already.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>
> ---
> virt/kvm/kvm_main.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index a17d787..ad0f8b2 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -3079,8 +3079,11 @@ static long kvm_vm_ioctl(struct file *filp,
> routing.nr * sizeof(*entries)))
> goto out_free_irq_routing;
> }
> + /* avoid races with KVM_CREATE_IRQCHIP on x86 */
> + mutex_lock(&kvm->lock);
> r = kvm_set_irq_routing(kvm, entries, routing.nr,
> routing.flags);
> + mutex_unlock(&kvm->lock);
> out_free_irq_routing:
> vfree(entries);
> break;
> --
> 2.9.3
>

Out of my curiousity: do we have a use case that these two operations
might collapse (KVM_CREATE_IRQCHIP and KVM_SET_GSI_ROUTING)? Or is
this patch only for the sake of security?

Another thing to mention is that, I guess adding this lock will
benefit KVM_CAP_SPLIT_IRQCHIP as well, maybe nice to mention it too in
the commit message. No worth a repost for this single reason though.

Thanks,

-- peterx