diff mbox

[v12,01/34] ARM: vGIC: avoid rank lock when reading priority

Message ID 20170614165223.7543-2-andre.przywara@arm.com (mailing list archive)
State New, archived
Headers show

Commit Message

Andre Przywara June 14, 2017, 4:51 p.m. UTC
When reading the priority value of a virtual interrupt, we were taking
the respective rank lock so far.
However for forwarded interrupts (Dom0 only so far) this may lead to a
deadlock with the following call chain:
- MMIO access to change the IRQ affinity, calling the ITARGETSR handler
- this handler takes the appropriate rank lock and calls vgic_store_itargetsr()
- vgic_store_itargetsr() will eventually call vgic_migrate_irq()
- if this IRQ is already in-flight, it will remove it from the old
  VCPU and inject it into the new one, by calling vgic_vcpu_inject_irq()
- vgic_vcpu_inject_irq will call vgic_get_virq_priority()
- vgic_get_virq_priority() tries to take the rank lock - again!
It seems like this code path has never been exercised before.

Fix this by avoiding taking the lock in vgic_get_virq_priority() (like we
do in vgic_get_target_vcpu()).
Actually we are just reading one byte, and priority changes while
interrupts are handled are a benign race that can happen on real hardware
too. So it is safe to just prevent the compiler from reading from the
struct more than once.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 xen/arch/arm/vgic-v2.c | 13 ++++++++-----
 xen/arch/arm/vgic-v3.c | 11 +++++++----
 xen/arch/arm/vgic.c    |  8 +-------
 3 files changed, 16 insertions(+), 16 deletions(-)

Comments

Julien Grall June 14, 2017, 5:55 p.m. UTC | #1
Hi Andre,

On 06/14/2017 05:51 PM, Andre Przywara wrote:
> When reading the priority value of a virtual interrupt, we were taking
> the respective rank lock so far.
> However for forwarded interrupts (Dom0 only so far) this may lead to a
> deadlock with the following call chain:
> - MMIO access to change the IRQ affinity, calling the ITARGETSR handler
> - this handler takes the appropriate rank lock and calls vgic_store_itargetsr()
> - vgic_store_itargetsr() will eventually call vgic_migrate_irq()
> - if this IRQ is already in-flight, it will remove it from the old
>    VCPU and inject it into the new one, by calling vgic_vcpu_inject_irq()
> - vgic_vcpu_inject_irq will call vgic_get_virq_priority()
> - vgic_get_virq_priority() tries to take the rank lock - again!
> It seems like this code path has never been exercised before.
> 
> Fix this by avoiding taking the lock in vgic_get_virq_priority() (like we
> do in vgic_get_target_vcpu()).
> Actually we are just reading one byte, and priority changes while
> interrupts are handled are a benign race that can happen on real hardware
> too. So it is safe to just prevent the compiler from reading from the
> struct more than once.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>

Reviewed-by: Julien Grall <julien.grall@arm.com>

Cheers,
Julien Grall June 19, 2017, 9:56 a.m. UTC | #2
Hi,

On 14/06/17 18:55, Julien Grall wrote:
> Hi Andre,
>
> On 06/14/2017 05:51 PM, Andre Przywara wrote:
>> When reading the priority value of a virtual interrupt, we were taking
>> the respective rank lock so far.
>> However for forwarded interrupts (Dom0 only so far) this may lead to a
>> deadlock with the following call chain:
>> - MMIO access to change the IRQ affinity, calling the ITARGETSR handler
>> - this handler takes the appropriate rank lock and calls
>> vgic_store_itargetsr()
>> - vgic_store_itargetsr() will eventually call vgic_migrate_irq()
>> - if this IRQ is already in-flight, it will remove it from the old
>>    VCPU and inject it into the new one, by calling vgic_vcpu_inject_irq()
>> - vgic_vcpu_inject_irq will call vgic_get_virq_priority()
>> - vgic_get_virq_priority() tries to take the rank lock - again!
>> It seems like this code path has never been exercised before.
>>
>> Fix this by avoiding taking the lock in vgic_get_virq_priority() (like we
>> do in vgic_get_target_vcpu()).
>> Actually we are just reading one byte, and priority changes while
>> interrupts are handled are a benign race that can happen on real hardware
>> too. So it is safe to just prevent the compiler from reading from the
>> struct more than once.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>
> Reviewed-by: Julien Grall <julien.grall@arm.com>

As you mentioned in the cover letter, I think it would be good to get 
this fixed in Xen 4.9. Stefano what do you think?

Cheers,
Stefano Stabellini June 19, 2017, 10:44 p.m. UTC | #3
On Mon, 19 Jun 2017, Julien Grall wrote:
> Hi,
> 
> On 14/06/17 18:55, Julien Grall wrote:
> > Hi Andre,
> > 
> > On 06/14/2017 05:51 PM, Andre Przywara wrote:
> > > When reading the priority value of a virtual interrupt, we were taking
> > > the respective rank lock so far.
> > > However for forwarded interrupts (Dom0 only so far) this may lead to a
> > > deadlock with the following call chain:
> > > - MMIO access to change the IRQ affinity, calling the ITARGETSR handler
> > > - this handler takes the appropriate rank lock and calls
> > > vgic_store_itargetsr()
> > > - vgic_store_itargetsr() will eventually call vgic_migrate_irq()
> > > - if this IRQ is already in-flight, it will remove it from the old
> > >    VCPU and inject it into the new one, by calling vgic_vcpu_inject_irq()
> > > - vgic_vcpu_inject_irq will call vgic_get_virq_priority()
> > > - vgic_get_virq_priority() tries to take the rank lock - again!
> > > It seems like this code path has never been exercised before.
> > > 
> > > Fix this by avoiding taking the lock in vgic_get_virq_priority() (like we
> > > do in vgic_get_target_vcpu()).
> > > Actually we are just reading one byte, and priority changes while
> > > interrupts are handled are a benign race that can happen on real hardware
> > > too. So it is safe to just prevent the compiler from reading from the
> > > struct more than once.
> > > 
> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > 
> > Reviewed-by: Julien Grall <julien.grall@arm.com>
> 
> As you mentioned in the cover letter, I think it would be good to get this
> fixed in Xen 4.9. Stefano what do you think?

Yes, I think it makes sense. Do you release-ack it?
Julien Grall June 20, 2017, 10:38 a.m. UTC | #4
Hi,

On 06/19/2017 11:44 PM, Stefano Stabellini wrote:
> On Mon, 19 Jun 2017, Julien Grall wrote:
>> Hi,
>>
>> On 14/06/17 18:55, Julien Grall wrote:
>>> Hi Andre,
>>>
>>> On 06/14/2017 05:51 PM, Andre Przywara wrote:
>>>> When reading the priority value of a virtual interrupt, we were taking
>>>> the respective rank lock so far.
>>>> However for forwarded interrupts (Dom0 only so far) this may lead to a
>>>> deadlock with the following call chain:
>>>> - MMIO access to change the IRQ affinity, calling the ITARGETSR handler
>>>> - this handler takes the appropriate rank lock and calls
>>>> vgic_store_itargetsr()
>>>> - vgic_store_itargetsr() will eventually call vgic_migrate_irq()
>>>> - if this IRQ is already in-flight, it will remove it from the old
>>>>     VCPU and inject it into the new one, by calling vgic_vcpu_inject_irq()
>>>> - vgic_vcpu_inject_irq will call vgic_get_virq_priority()
>>>> - vgic_get_virq_priority() tries to take the rank lock - again!
>>>> It seems like this code path has never been exercised before.
>>>>
>>>> Fix this by avoiding taking the lock in vgic_get_virq_priority() (like we
>>>> do in vgic_get_target_vcpu()).
>>>> Actually we are just reading one byte, and priority changes while
>>>> interrupts are handled are a benign race that can happen on real hardware
>>>> too. So it is safe to just prevent the compiler from reading from the
>>>> struct more than once.
>>>>
>>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
>>>
>>> Reviewed-by: Julien Grall <julien.grall@arm.com>
>>
>> As you mentioned in the cover letter, I think it would be good to get this
>> fixed in Xen 4.9. Stefano what do you think?
> 
> Yes, I think it makes sense. Do you release-ack it?

Release-acked-by: Julien Grall <julien.grall@arm.com>

Cheers,
diff mbox

Patch

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index dc9f95b..9fa42e1 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -252,15 +252,15 @@  static int vgic_v2_distr_mmio_read(struct vcpu *v, mmio_info_t *info,
     case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
     {
         uint32_t ipriorityr;
+        uint8_t rank_index;
 
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD);
         if ( rank == NULL ) goto read_as_zero;
+        rank_index = REG_RANK_INDEX(8, gicd_reg - GICD_IPRIORITYR, DABT_WORD);
 
         vgic_lock_rank(v, rank, flags);
-        ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8,
-                                                     gicd_reg - GICD_IPRIORITYR,
-                                                     DABT_WORD)];
+        ipriorityr = ACCESS_ONCE(rank->ipriorityr[rank_index]);
         vgic_unlock_rank(v, rank, flags);
         *r = vgic_reg32_extract(ipriorityr, info);
 
@@ -499,7 +499,7 @@  static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
 
     case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
     {
-        uint32_t *ipriorityr;
+        uint32_t *ipriorityr, priority;
 
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 8, gicd_reg - GICD_IPRIORITYR, DABT_WORD);
@@ -508,7 +508,10 @@  static int vgic_v2_distr_mmio_write(struct vcpu *v, mmio_info_t *info,
         ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8,
                                                       gicd_reg - GICD_IPRIORITYR,
                                                       DABT_WORD)];
-        vgic_reg32_update(ipriorityr, r, info);
+        priority = ACCESS_ONCE(*ipriorityr);
+        vgic_reg32_update(&priority, r, info);
+        ACCESS_ONCE(*ipriorityr) = priority;
+
         vgic_unlock_rank(v, rank, flags);
         return 1;
     }
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index d10757a..9018ddc 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -515,14 +515,15 @@  static int __vgic_v3_distr_common_mmio_read(const char *name, struct vcpu *v,
     case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
     {
         uint32_t ipriorityr;
+        uint8_t rank_index;
 
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
         if ( rank == NULL ) goto read_as_zero;
+        rank_index = REG_RANK_INDEX(8, reg - GICD_IPRIORITYR, DABT_WORD);
 
         vgic_lock_rank(v, rank, flags);
-        ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
-                                                     DABT_WORD)];
+        ipriorityr = ACCESS_ONCE(rank->ipriorityr[rank_index]);
         vgic_unlock_rank(v, rank, flags);
 
         *r = vgic_reg32_extract(ipriorityr, info);
@@ -630,7 +631,7 @@  static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
 
     case VRANGE32(GICD_IPRIORITYR, GICD_IPRIORITYRN):
     {
-        uint32_t *ipriorityr;
+        uint32_t *ipriorityr, priority;
 
         if ( dabt.size != DABT_BYTE && dabt.size != DABT_WORD ) goto bad_width;
         rank = vgic_rank_offset(v, 8, reg - GICD_IPRIORITYR, DABT_WORD);
@@ -638,7 +639,9 @@  static int __vgic_v3_distr_common_mmio_write(const char *name, struct vcpu *v,
         vgic_lock_rank(v, rank, flags);
         ipriorityr = &rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
                                                       DABT_WORD)];
-        vgic_reg32_update(ipriorityr, r, info);
+        priority = ACCESS_ONCE(*ipriorityr);
+        vgic_reg32_update(&priority, r, info);
+        ACCESS_ONCE(*ipriorityr) = priority;
         vgic_unlock_rank(v, rank, flags);
         return 1;
     }
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 83569b0..18fe420 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -227,14 +227,8 @@  struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq)
 static int vgic_get_virq_priority(struct vcpu *v, unsigned int virq)
 {
     struct vgic_irq_rank *rank = vgic_rank_irq(v, virq);
-    unsigned long flags;
-    int priority;
-
-    vgic_lock_rank(v, rank, flags);
-    priority = rank->priority[virq & INTERRUPT_RANK_MASK];
-    vgic_unlock_rank(v, rank, flags);
 
-    return priority;
+    return ACCESS_ONCE(rank->priority[virq & INTERRUPT_RANK_MASK]);
 }
 
 bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)