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

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



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?

 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?


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.

Cheers,

--
Julien Grall

_______________________________________________
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®.