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

David Hildenbrand
2017-03-15T09:19:57Z

Am 15.03.2017 um 07:24 schrieb Peter Xu:
> 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?

Just for the sake of security. I think, in general such calls are not
supposed to be done in parallel. But of course, user space can.

>
> 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.
>

I added

"Also, as a side effect, this patch makes sure that KVM_SET_GSI_ROUTING
and KVM_CAP_SPLIT_IRQCHIP cannot run in parallel."

Thanks!

> Thanks,
>
> -- peterx
>

--
Thanks,

David