|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v11 01/34] ARM: vGIC: avoid rank lock when reading priority
On Mon, 12 Jun 2017, Julien Grall wrote:
> 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.przywara@xxxxxxx>
> > ---
> > 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.
I also prefer *_atomic, especially given what Jan wrote about
ACCESS_ONCE:
Plus ACCESS_ONCE() doesn't enforce a single instruction to be used in
the resulting assembly).
> > +
> > 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |