| Message ID | 20170526173540.10066-2-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andre, On 26/05/17 18:35, 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 looks safe to just use read_atomic() instead. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/vgic.c | 8 +------- > 1 file changed, 1 insertion(+), 7 deletions(-) > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index 83569b0..54b2aad 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 read_atomic(&rank->priority[virq & INTERRUPT_RANK_MASK]); The write in rank->priority will not be atomic (see vgic_reg_update implementation): the register is first masked, the the priority set. So you may end up to read 0 (which is the higher priority) by mistake. We should probably think to make vgic_reg_* helper atomic. > } > > bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) > Cheers,
On Tue, 30 May 2017, Julien Grall wrote: > Hi Andre, > > On 26/05/17 18:35, 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 looks safe to just use read_atomic() instead. > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > xen/arch/arm/vgic.c | 8 +------- > > 1 file changed, 1 insertion(+), 7 deletions(-) > > > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > > index 83569b0..54b2aad 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 read_atomic(&rank->priority[virq & INTERRUPT_RANK_MASK]); > > The write in rank->priority will not be atomic (see vgic_reg_update > implementation): the register is first masked, the the priority set. > > So you may end up to read 0 (which is the higher priority) by mistake. > > We should probably think to make vgic_reg_* helper atomic. Right! That's why I wrote alpine.DEB.2.10.1705231135130.18759@sstabellini-ThinkPad-X260 > > } > > > > bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq) > > > > Cheers, > > -- > Julien Grall >
Hi Stefano, On 30/05/17 22:39, Stefano Stabellini wrote: > On Tue, 30 May 2017, Julien Grall wrote: >> Hi Andre, >> >> On 26/05/17 18:35, 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 looks safe to just use read_atomic() instead. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> xen/arch/arm/vgic.c | 8 +------- >>> 1 file changed, 1 insertion(+), 7 deletions(-) >>> >>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >>> index 83569b0..54b2aad 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 read_atomic(&rank->priority[virq & INTERRUPT_RANK_MASK]); >> >> The write in rank->priority will not be atomic (see vgic_reg_update >> implementation): the register is first masked, the the priority set. >> >> So you may end up to read 0 (which is the higher priority) by mistake. >> >> We should probably think to make vgic_reg_* helper atomic. > > Right! That's why I wrote > alpine.DEB.2.10.1705231135130.18759@sstabellini-ThinkPad-X260 It was not obvious from this e-mail why you wanted this. It looked more a request for improvement rather than a potential bug. Hence my suggestion to delay it. Cheers,
On 05/31/2017 11:42 AM, Julien Grall wrote: > Hi Stefano, > > On 30/05/17 22:39, Stefano Stabellini wrote: >> On Tue, 30 May 2017, Julien Grall wrote: >>> Hi Andre, >>> >>> On 26/05/17 18:35, 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 looks safe to just use read_atomic() instead. >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>> --- >>>> xen/arch/arm/vgic.c | 8 +------- >>>> 1 file changed, 1 insertion(+), 7 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >>>> index 83569b0..54b2aad 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 read_atomic(&rank->priority[virq & INTERRUPT_RANK_MASK]); >>> >>> The write in rank->priority will not be atomic (see vgic_reg_update >>> implementation): the register is first masked, the the priority set. >>> >>> So you may end up to read 0 (which is the higher priority) by mistake. >>> >>> We should probably think to make vgic_reg_* helper atomic. >> >> Right! That's why I wrote >> alpine.DEB.2.10.1705231135130.18759@sstabellini-ThinkPad-X260 > > It was not obvious from this e-mail why you wanted this. It looked more > a request for improvement rather than a potential bug. Hence my > suggestion to delay it. FIY, I wrote down a couple of patch to make vgic_reg* atomic. It is not too invasive and would solve write atomically in the register. However, it will not prevent two write racing together. So the lock would still be need in some places. I am still doing some testing. I will send a version hopefully Monday. Cheers,
Hi, On 30/05/17 11:47, Julien Grall wrote: > Hi Andre, > > On 26/05/17 18:35, 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 looks safe to just use read_atomic() instead. >> >> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >> --- >> xen/arch/arm/vgic.c | 8 +------- >> 1 file changed, 1 insertion(+), 7 deletions(-) >> >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >> index 83569b0..54b2aad 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 read_atomic(&rank->priority[virq & INTERRUPT_RANK_MASK]); > > The write in rank->priority will not be atomic (see vgic_reg_update > implementation): the register is first masked, the the priority set. > > So you may end up to read 0 (which is the higher priority) by mistake. > > We should probably think to make vgic_reg_* helper atomic. The patch you sent yesterday may be a bit over the top for this. What about this one (in xen/arch/arm/vgic-v[23].c): static int vgic_v2_distr_mmio_write(struct vcpu *v, ... { - uint32_t *ipriorityr; + uint32_t *ipriorityr, priority; .... vgic_lock_rank(v, rank, flags); 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); .... Concurrent writes are protected by the rank lock, and reads are guaranteed to be atomic due to ACCESS_ONCE and the architectural guarantee of an aligned uint32_t access being atomic. We can't do anything about the benign race when the priority gets changed while we read it, but the above should make sure that we either read the old or the new value and nothing in between. The other read accesses (in vgic_v[23]_distr_mmio_read() and in vgic.c here in this patch) get protected by an ACCESS_ONCE(). Does that make sense? Cheers, Andre. > >> } >> >> bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned >> int irq) >> > > Cheers, >
On 06/06/17 18:06, Andre Przywara wrote: > On 30/05/17 11:47, Julien Grall wrote: >> Hi Andre, >> >> On 26/05/17 18:35, 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 looks safe to just use read_atomic() instead. >>> >>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>> --- >>> xen/arch/arm/vgic.c | 8 +------- >>> 1 file changed, 1 insertion(+), 7 deletions(-) >>> >>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >>> index 83569b0..54b2aad 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 read_atomic(&rank->priority[virq & INTERRUPT_RANK_MASK]); >> >> The write in rank->priority will not be atomic (see vgic_reg_update >> implementation): the register is first masked, the the priority set. >> >> So you may end up to read 0 (which is the higher priority) by mistake. >> >> We should probably think to make vgic_reg_* helper atomic. > > The patch you sent yesterday may be a bit over the top for this. Patch which I only sent internally to get feedback. ;) > > What about this one (in xen/arch/arm/vgic-v[23].c): > > static int vgic_v2_distr_mmio_write(struct vcpu *v, ... > { > - uint32_t *ipriorityr; > + uint32_t *ipriorityr, priority; > .... > vgic_lock_rank(v, rank, flags); > 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); > .... What you do is very similar to my patch but open-code it. It might be possible to have other place which could take advantage of the a generic solution, so I am not convinced this would be the right way. > > Concurrent writes are protected by the rank lock, and reads are > guaranteed to be atomic due to ACCESS_ONCE and the architectural > guarantee of an aligned uint32_t access being atomic. > We can't do anything about the benign race when the priority gets > changed while we read it, but the above should make sure that we either > read the old or the new value and nothing in between. > The other read accesses (in vgic_v[23]_distr_mmio_read() and in vgic.c > here in this patch) get protected by an ACCESS_ONCE(). > > Does that make sense? I was trying to address the suggestion of Stefano to drop the rank lock in some place and also make the solution generic. I failed to address properly the lock drop. And have some idea how to do it. Cheers,
Hi, On 06/06/17 18:11, Julien Grall wrote: > > > On 06/06/17 18:06, Andre Przywara wrote: >> On 30/05/17 11:47, Julien Grall wrote: >>> Hi Andre, >>> >>> On 26/05/17 18:35, 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 looks safe to just use read_atomic() instead. >>>> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com> >>>> --- >>>> xen/arch/arm/vgic.c | 8 +------- >>>> 1 file changed, 1 insertion(+), 7 deletions(-) >>>> >>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >>>> index 83569b0..54b2aad 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 read_atomic(&rank->priority[virq & INTERRUPT_RANK_MASK]); >>> >>> The write in rank->priority will not be atomic (see vgic_reg_update >>> implementation): the register is first masked, the the priority set. >>> >>> So you may end up to read 0 (which is the higher priority) by mistake. >>> >>> We should probably think to make vgic_reg_* helper atomic. >> >> The patch you sent yesterday may be a bit over the top for this. > > Patch which I only sent internally to get feedback. ;) Ah, indeed, sorry for dragging you into the light ;-) For the records: I am not saying that your patch was wrong, I am just a bit reluctant to change such a central function (vgic_reg_update) when we only care about the priority. >> >> What about this one (in xen/arch/arm/vgic-v[23].c): >> >> static int vgic_v2_distr_mmio_write(struct vcpu *v, ... >> { >> - uint32_t *ipriorityr; >> + uint32_t *ipriorityr, priority; >> .... >> vgic_lock_rank(v, rank, flags); >> 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); >> .... > > What you do is very similar to my patch but open-code it. It might be > possible to have other place which could take advantage of the a generic > solution, so I am not convinced this would be the right way. My idea was to confine this fix to what we care for right now. I guess eventually it doesn't matter as we will probably get rid of the rank and its lock anyway. >> >> Concurrent writes are protected by the rank lock, and reads are >> guaranteed to be atomic due to ACCESS_ONCE and the architectural >> guarantee of an aligned uint32_t access being atomic. >> We can't do anything about the benign race when the priority gets >> changed while we read it, but the above should make sure that we either >> read the old or the new value and nothing in between. >> The other read accesses (in vgic_v[23]_distr_mmio_read() and in vgic.c >> here in this patch) get protected by an ACCESS_ONCE(). >> >> Does that make sense? > > I was trying to address the suggestion of Stefano to drop the rank lock > in some place and also make the solution generic. > > I failed to address properly the lock drop. And have some idea how to do > it. Looking forward to it .... Cheers, Andre.
Hi Andre, On 06/06/17 18:20, Andre Przywara wrote: >>> >>> What about this one (in xen/arch/arm/vgic-v[23].c): >>> >>> static int vgic_v2_distr_mmio_write(struct vcpu *v, ... >>> { >>> - uint32_t *ipriorityr; >>> + uint32_t *ipriorityr, priority; >>> .... >>> vgic_lock_rank(v, rank, flags); >>> 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); >>> .... >> >> What you do is very similar to my patch but open-code it. It might be >> possible to have other place which could take advantage of the a generic >> solution, so I am not convinced this would be the right way. > > My idea was to confine this fix to what we care for right now. > I guess eventually it doesn't matter as we will probably get rid of the > rank and its lock anyway. That's a good point. So we may want to wait until the vGIC rework to see what we can do here. Stefano, any opinion? Cheers,
On Tue, 6 Jun 2017, Julien Grall wrote: > Hi Andre, > > On 06/06/17 18:20, Andre Przywara wrote: > > > > > > > > What about this one (in xen/arch/arm/vgic-v[23].c): > > > > > > > > static int vgic_v2_distr_mmio_write(struct vcpu *v, ... > > > > { > > > > - uint32_t *ipriorityr; > > > > + uint32_t *ipriorityr, priority; > > > > .... > > > > vgic_lock_rank(v, rank, flags); > > > > 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); > > > > .... > > > > > > What you do is very similar to my patch but open-code it. It might be > > > possible to have other place which could take advantage of the a generic > > > solution, so I am not convinced this would be the right way. > > > > My idea was to confine this fix to what we care for right now. > > I guess eventually it doesn't matter as we will probably get rid of the > > rank and its lock anyway. > > That's a good point. So we may want to wait until the vGIC rework to see what > we can do here. Stefano, any opinion? Either would work. Since we are at it, I would have done what Julien has done, but also doing the minimum required now is also OK. I don't think the order of changes is important in this case given that the end result will be the same.
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index 83569b0..54b2aad 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 read_atomic(&rank->priority[virq & INTERRUPT_RANK_MASK]); } bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
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 looks safe to just use read_atomic() instead. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/vgic.c | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-)