Re: [Xen-devel] [PATCH 3/3] arm: vgic: fix race in vgic_vcpu_inject_irq

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Wed, 2013-04-17 at 13:52 +0100, Ian Campbell wrote:
> The initial check for a still pending interrupt (!list_empty(&n->inflight))
> needs to be covered by the vgic lock to avoid trying to insert the IRQ into 
> the
> inflight list simultaneously on 2 pCPUS. Expand the area covered by the lock
> appropriately.
> 
> Also consolidate the unlocks on the exit path into one location.
> 
> Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

Thanks, I've applied this series.

I got some rejects when applying this particular patch since it was
based on Stefano's "xen/arm: trap guest WFI", the rejects was down to
the lack of the out: label and vcpu_kick at the end of
vgic_vcpu_inject_irq. What actually got applied is:

commit e83d6b9432af603200f065b499b8e4b78e92842d
Author: Ian Campbell <ian.campbell@xxxxxxxxxx>
Date:   Wed Apr 17 13:52:34 2013 +0100

    arm: vgic: fix race in vgic_vcpu_inject_irq
    
    The initial check for a still pending interrupt (!list_empty(&n->inflight))
    needs to be covered by the vgic lock to avoid trying to insert the IRQ into 
the
    inflight list simultaneously on 2 pCPUS. Expand the area covered by the lock
    appropriately.
    
    Also consolidate the unlocks on the exit path into one location.
    
    Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
    Acked-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index d9ceaaa..4d8da02 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -584,9 +584,14 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int 
irq, int virtual)
     struct pending_irq *iter, *n = irq_to_pending(v, irq);
     unsigned long flags;
 
-    /* irq still pending */
+    spin_lock_irqsave(&v->arch.vgic.lock, flags);
+
+    /* irq already pending */
     if (!list_empty(&n->inflight))
+    {
+        spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
         return;
+    }
 
     priority = byte_read(rank->ipriority[REG_RANK_INDEX(8, idx)], 0, byte);
 
@@ -601,17 +606,16 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int 
irq, int virtual)
     if ( rank->ienable & (1 << (irq % 32)) )
         gic_set_guest_irq(v, irq, GICH_LR_PENDING, priority);
 
-    spin_lock_irqsave(&v->arch.vgic.lock, flags);
     list_for_each_entry ( iter, &v->arch.vgic.inflight_irqs, inflight )
     {
         if ( iter->priority > priority )
         {
             list_add_tail(&n->inflight, &iter->inflight);
-            spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
-            return;
+            goto out;
         }
     }
     list_add_tail(&n->inflight, &v->arch.vgic.inflight_irqs);
+out:
     spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
     /* we have a new higher priority irq, inject it into the guest */
 }



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel