| Message ID | 20170314133450.13259-2-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
On Wed, Mar 15, 2017 at 10:19:53AM +0100, David Hildenbrand wrote: > 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. I see. > > > > > 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." Yeah it looks better. Thanks! -- peterx
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;
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(+)