| Message ID | 20170407085041.5257-2-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Sorry to resurrect this thread. There's an srcu_synchronize_expedited call in kvm_set_irq_routing, so this lock might get held for a fairly long while. Wouldn't it be a better idea to check the irqchip mode? On Fri, Apr 7, 2017 at 1:50 AM, David Hildenbrand <david@redhat.com> 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. > > Also, as a side effect, this patch makes sure that KVM_SET_GSI_ROUTING > and KVM_CAP_SPLIT_IRQCHIP cannot run in parallel. > > 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 88257b3..a80e400 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -3082,8 +3082,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 >
On 25.04.2017 00:39, Steve Rutherford wrote: > Sorry to resurrect this thread. There's an srcu_synchronize_expedited > call in kvm_set_irq_routing, so this lock might get held for a fairly > long while. Wouldn't it be a better idea to check the irqchip mode? > We could maybe check for kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE here under lock instead of in the function itself. Maybe we then even don't need patch NR 2. Will have a look! Thanks! > On Fri, Apr 7, 2017 at 1:50 AM, David Hildenbrand <david@redhat.com> 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. >> >> Also, as a side effect, this patch makes sure that KVM_SET_GSI_ROUTING >> and KVM_CAP_SPLIT_IRQCHIP cannot run in parallel. >> >> 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 88257b3..a80e400 100644 >> --- a/virt/kvm/kvm_main.c >> +++ b/virt/kvm/kvm_main.c >> @@ -3082,8 +3082,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 >>
2017-04-25 14:34+0200, David Hildenbrand: > On 25.04.2017 00:39, Steve Rutherford wrote: >> Sorry to resurrect this thread. There's an srcu_synchronize_expedited >> call in kvm_set_irq_routing, so this lock might get held for a fairly >> long while. Wouldn't it be a better idea to check the irqchip mode? >> > > We could maybe check for kvm->arch.irqchip_mode == KVM_IRQCHIP_NONE > here under lock instead of in the function itself. Maybe we then even > don't need patch NR 2. Will have a look! Sounds good and the extra mode would be obsolete. Checking in kvm_main.c will just need a new arch hook. The patches are already in kvm/next, though, so please prepare it as a fix. Thanks.
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 88257b3..a80e400 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -3082,8 +3082,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;
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. Also, as a side effect, this patch makes sure that KVM_SET_GSI_ROUTING and KVM_CAP_SPLIT_IRQCHIP cannot run in parallel. Signed-off-by: David Hildenbrand <david@redhat.com> --- virt/kvm/kvm_main.c | 3 +++ 1 file changed, 3 insertions(+)