Re: [Xen-devel] [PATCH v4 1/2] arm: read/write rank->vcpu atomically

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Thu, 16 Feb 2017, Julien Grall wrote:
> Hi Stefano,
> 
> On 11/02/17 02:05, Stefano Stabellini wrote:
> > We don't need a lock in vgic_get_target_vcpu anymore, solving the
> > following lock inversion bug: the rank lock should be taken first, then
> > the vgic lock. However, gic_update_one_lr is called with the vgic lock
> > held, and it calls vgic_get_target_vcpu, which tries to obtain the rank
> > lock.
> > 
> > Coverity-ID: 1381855
> > Coverity-ID: 1381853
> > 
> > Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > ---
> >  xen/arch/arm/vgic-v2.c |  6 +++---
> >  xen/arch/arm/vgic-v3.c |  6 +++---
> >  xen/arch/arm/vgic.c    | 27 +++++----------------------
> >  3 files changed, 11 insertions(+), 28 deletions(-)
> > 
> > diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
> > index 3dbcfe8..b30379e 100644
> > --- a/xen/arch/arm/vgic-v2.c
> > +++ b/xen/arch/arm/vgic-v2.c
> > @@ -79,7 +79,7 @@ static uint32_t vgic_fetch_itargetsr(struct vgic_irq_rank
> > *rank,
> >      offset &= ~(NR_TARGETS_PER_ITARGETSR - 1);
> > 
> >      for ( i = 0; i < NR_TARGETS_PER_ITARGETSR; i++, offset++ )
> > -        reg |= (1 << rank->vcpu[offset]) << (i * NR_BITS_PER_TARGET);
> > +        reg |= (1 << read_atomic(&rank->vcpu[offset])) << (i *
> > NR_BITS_PER_TARGET);
> 
> I was about to suggested to turn vcpu into an atomic_t to catch potential
> misuse. But unfortunately atomic_t is int.

Indeed


> So I would probably add a comment on top of the field vcpu in vgic_irq_rank
> explaining that vcpu should be read using atomic.
>
> With that:
> 
> Reviewed-by: Julien Grall <julien.grall@xxxxxxx>

Thank you, I added:

+     * Use atomic operations to read/write the vcpu fields to avoid
+     * taking the rank lock.

and committed.

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