[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH] xen/arm: fix rank/vgic locks inversion bug



On Thu, 15 Dec 2016, Julien Grall wrote:
> Hi Stefano,
> 
> On 15/12/2016 01:04, Stefano Stabellini wrote:
> > The locking order is: first rank lock, then vgic lock. The order is
> > respected everywhere, except for gic_update_one_lr.
> > 
> > gic_update_one_lr is called with the vgic lock held, but it calls
> > vgic_get_target_vcpu, which tries to obtain the rank lock. This can
> > cause deadlocks.
> > 
> > We already have a version of vgic_get_target_vcpu that doesn't take the
> > rank lock: __vgic_get_target_vcpu. Also the only routine that modify
> > the target vcpu are vgic_store_itargetsr and vgic_store_irouter. They
> > call vgic_migrate_irq, which already take the vgic lock.
> > 
> > Solve the lock inversion problem, by not taking the rank lock in
> > gic_update_one_lr (calling __vgic_get_target_vcpu instead of
> > vgic_get_target_vcpu).
> 
> If I look at the callers of gic_update_one_lr, the function gic_clear_lrs will
> not take the rank lock. So from my understanding nobody will take the rank
> here.
>
> However __vgic_get_target_vcpu has an ASSERT to check whether the rank lock
> has been taken. So who is taking lock for gic_update_one_lr now?

I should have removed the ASSERT - nobody is taking the rank lock now on
the gic_update_one_lr code path.


>  We make this safe, by placing modifications to
> > rank->vcpu within regions protected by the vgic lock.
> 
> This look unsafe to me. The vgic lock is per-vcpu, but rank->vcpu could be
> written/read by any vCPU. So you will never protect rank->vcpu with this lock.
> Did I miss anything?

The vgic lock is per-vcpu, but it is always the vgic lock of the old (old
in the sense, before the interrupt is migrated) vcpu that is taken.

On one hand any vcpu can read/write to itargetsr. Those operations are
protected by the rank lock. However vgic_migrate_irq and writes to
rank->vcpu are protected also by the vgic lock of the old vcpu (first
the rank lock is taken, then the vgic lock of the old vcpu).

gic_update_one_lr doesn't take the rank lock, but it takes the vgic lock
of the old vcpu. We know that it is the old vcpu because it is the one
with the interrupt in question in an LR register. gic_update_one_lr
accesses to rank->vcpu are safe because they are protected by the same
vgic lock.

Does it make sense?



> > 
> > Signed-off-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> > 
> > ---
> > There are supposed to be 2 Coverity IDs associated with this, but the
> > system the currently down.
> 
> I think the 2 Coverity IDs are 1381855 and 1381853.
 


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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.