Hi Andre,
On 09/06/17 18:41, 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.przyw...@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(-)
diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index dc9f95b..5370020 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -258,9 +258,9 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v,
mmio_info_t *info,
if ( rank == NULL ) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8,
- gicd_reg -
GICD_IPRIORITYR,
- DABT_WORD)];
+ ipriorityr = ACCESS_ONCE(rank->ipriorityr[REG_RANK_INDEX(8,
+ gicd_reg - GICD_IPRIORITYR,
+ DABT_WORD)]);
The indentation is a bit odd. Can you introduce a temporary variable here?
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;
This is a bit odd to read because of the dereferencing. I admit that I
would prefer if you use read_atomic/write_atomic which are easier to
understand (though the naming is confusing).
Let see what Stefano thinks here.
+
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..8abc069 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -521,8 +521,9 @@ static int __vgic_v3_distr_common_mmio_read(const char
*name, struct vcpu *v,
if ( rank == NULL ) goto read_as_zero;
vgic_lock_rank(v, rank, flags);
- ipriorityr = rank->ipriorityr[REG_RANK_INDEX(8, reg - GICD_IPRIORITYR,
- DABT_WORD)];
+ ipriorityr = ACCESS_ONCE(rank->ipriorityr[REG_RANK_INDEX(8,
+ reg - GICD_IPRIORITYR,
+ DABT_WORD)]);
Ditto.
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;
Ditto.
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)
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
