lkml.org 
[lkml]   [2009]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [KVM PATCH v3 1/3] KVM: fix race in irq_routing logic
    On Mon, Oct 26, 2009 at 12:21:57PM -0400, Gregory Haskins wrote:
    > The current code suffers from the following race condition:
    >
    > thread-1 thread-2
    > -----------------------------------------------------------
    >
    > kvm_set_irq() {
    > rcu_read_lock()
    > irq_rt = rcu_dereference(table);
    > rcu_read_unlock();
    >
    > kvm_set_irq_routing() {
    > mutex_lock();
    > irq_rt = table;
    > rcu_assign_pointer();
    > mutex_unlock();
    > synchronize_rcu();
    >
    > kfree(irq_rt);
    >
    > irq_rt->entry->set(); /* bad */
    >
    This is not what happens. irq_rt is never accessed outside read-side
    critical section. Data is copied from irq_rt onto the stack and this
    copy is accessed outside critical section.

    > -------------------------------------------------------------
    >
    > Because the pointer is accessed outside of the read-side critical
    > section. There are two basic patterns we can use to fix this bug:
    >
    > 1) Switch to sleeping-rcu and encompass the ->set() access within the
    > read-side critical section,
    >
    > OR
    >
    > 2) Add reference counting to the irq_rt structure, and simply acquire
    > the reference from within the RSCS.
    >
    > This patch implements solution (1).
    >
    > Signed-off-by: Gregory Haskins <ghaskins@novell.com>
    > ---
    >
    > include/linux/kvm_host.h | 6 +++++-
    > virt/kvm/irq_comm.c | 50 +++++++++++++++++++++++++++-------------------
    > virt/kvm/kvm_main.c | 1 +
    > 3 files changed, 35 insertions(+), 22 deletions(-)
    >
    > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
    > index bd5a616..1fe135d 100644
    > --- a/include/linux/kvm_host.h
    > +++ b/include/linux/kvm_host.h
    > @@ -185,7 +185,10 @@ struct kvm {
    >
    > struct mutex irq_lock;
    > #ifdef CONFIG_HAVE_KVM_IRQCHIP
    > - struct kvm_irq_routing_table *irq_routing;
    > + struct {
    > + struct srcu_struct srcu;
    > + struct kvm_irq_routing_table *table;
    > + } irq_routing;
    > struct hlist_head mask_notifier_list;
    > struct hlist_head irq_ack_notifier_list;
    > #endif
    > @@ -541,6 +544,7 @@ int kvm_set_irq_routing(struct kvm *kvm,
    > const struct kvm_irq_routing_entry *entries,
    > unsigned nr,
    > unsigned flags);
    > +void kvm_init_irq_routing(struct kvm *kvm);
    > void kvm_free_irq_routing(struct kvm *kvm);
    >
    > #else
    > diff --git a/virt/kvm/irq_comm.c b/virt/kvm/irq_comm.c
    > index 00c68d2..db2553f 100644
    > --- a/virt/kvm/irq_comm.c
    > +++ b/virt/kvm/irq_comm.c
    > @@ -144,10 +144,11 @@ static int kvm_set_msi(struct kvm_kernel_irq_routing_entry *e,
    > */
    > int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level)
    > {
    > - struct kvm_kernel_irq_routing_entry *e, irq_set[KVM_NR_IRQCHIPS];
    > - int ret = -1, i = 0;
    > + struct kvm_kernel_irq_routing_entry *e;
    > + int ret = -1;
    > struct kvm_irq_routing_table *irq_rt;
    > struct hlist_node *n;
    > + int idx;
    >
    > trace_kvm_set_irq(irq, level, irq_source_id);
    >
    > @@ -155,21 +156,19 @@ int kvm_set_irq(struct kvm *kvm, int irq_source_id, u32 irq, int level)
    > * IOAPIC. So set the bit in both. The guest will ignore
    > * writes to the unused one.
    > */
    > - rcu_read_lock();
    > - irq_rt = rcu_dereference(kvm->irq_routing);
    > + idx = srcu_read_lock(&kvm->irq_routing.srcu);
    > + irq_rt = rcu_dereference(kvm->irq_routing.table);
    > if (irq < irq_rt->nr_rt_entries)
    > - hlist_for_each_entry(e, n, &irq_rt->map[irq], link)
    > - irq_set[i++] = *e;
    > - rcu_read_unlock();
    > + hlist_for_each_entry(e, n, &irq_rt->map[irq], link) {
    > + int r;
    >
    > - while(i--) {
    > - int r;
    > - r = irq_set[i].set(&irq_set[i], kvm, irq_source_id, level);
    > - if (r < 0)
    > - continue;
    > + r = e->set(e, kvm, irq_source_id, level);
    > + if (r < 0)
    > + continue;
    >
    > - ret = r + ((ret < 0) ? 0 : ret);
    > - }
    > + ret = r + ((ret < 0) ? 0 : ret);
    > + }
    > + srcu_read_unlock(&kvm->irq_routing.srcu, idx);
    >
    > return ret;
    > }
    > @@ -179,17 +178,18 @@ void kvm_notify_acked_irq(struct kvm *kvm, unsigned irqchip, unsigned pin)
    > struct kvm_irq_ack_notifier *kian;
    > struct hlist_node *n;
    > int gsi;
    > + int idx;
    >
    > trace_kvm_ack_irq(irqchip, pin);
    >
    > - rcu_read_lock();
    > - gsi = rcu_dereference(kvm->irq_routing)->chip[irqchip][pin];
    > + idx = srcu_read_lock(&kvm->irq_routing.srcu);
    > + gsi = rcu_dereference(kvm->irq_routing.table)->chip[irqchip][pin];
    > if (gsi != -1)
    > hlist_for_each_entry_rcu(kian, n, &kvm->irq_ack_notifier_list,
    > link)
    > if (kian->gsi == gsi)
    > kian->irq_acked(kian);
    > - rcu_read_unlock();
    > + srcu_read_unlock(&kvm->irq_routing.srcu, idx);
    > }
    >
    > void kvm_register_irq_ack_notifier(struct kvm *kvm,
    > @@ -287,11 +287,19 @@ void kvm_fire_mask_notifiers(struct kvm *kvm, int irq, bool mask)
    > rcu_read_unlock();
    > }
    >
    > +void kvm_init_irq_routing(struct kvm *kvm)
    > +{
    > + init_srcu_struct(&kvm->irq_routing.srcu);
    > +}
    > +
    > void kvm_free_irq_routing(struct kvm *kvm)
    > {
    > /* Called only during vm destruction. Nobody can use the pointer
    > at this stage */
    > - kfree(kvm->irq_routing);
    > + synchronize_srcu(&kvm->irq_routing.srcu);
    > + cleanup_srcu_struct(&kvm->irq_routing.srcu);
    > +
    > + kfree(kvm->irq_routing.table);
    > }
    >
    > static int setup_routing_entry(struct kvm_irq_routing_table *rt,
    > @@ -396,10 +404,10 @@ int kvm_set_irq_routing(struct kvm *kvm,
    > }
    >
    > mutex_lock(&kvm->irq_lock);
    > - old = kvm->irq_routing;
    > - rcu_assign_pointer(kvm->irq_routing, new);
    > + old = kvm->irq_routing.table;
    > + rcu_assign_pointer(kvm->irq_routing.table, new);
    > mutex_unlock(&kvm->irq_lock);
    > - synchronize_rcu();
    > + synchronize_srcu(&kvm->irq_routing.srcu);
    >
    > new = old;
    > r = 0;
    > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
    > index cac69c4..ba94159 100644
    > --- a/virt/kvm/kvm_main.c
    > +++ b/virt/kvm/kvm_main.c
    > @@ -388,6 +388,7 @@ static struct kvm *kvm_create_vm(void)
    > atomic_inc(&kvm->mm->mm_count);
    > spin_lock_init(&kvm->mmu_lock);
    > spin_lock_init(&kvm->requests_lock);
    > + kvm_init_irq_routing(kvm);
    > kvm_io_bus_init(&kvm->pio_bus);
    > kvm_eventfd_init(kvm);
    > mutex_init(&kvm->lock);
    >
    > --
    > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    > the body of a message to majordomo@vger.kernel.org
    > More majordomo info at http://vger.kernel.org/majordomo-info.html
    > Please read the FAQ at http://www.tux.org/lkml/

    --
    Gleb.


    \
     
     \ /
      Last update: 2009-10-27 07:49    [W:0.044 / U:91.948 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site