[Xen-devel] ITS emulation: case for checking pending_irq against NULL

[Andre Przywara <andre.przywara@xxxxxxx>]

Hello,

this is trying to make the point that having NULL pointer entries in the
radix tree and letting Xen handle that is a valid case.
To some degree this serves as a tag (LPI has been unmapped), so should
semantically come close to what we are discussing.
It has the big advantage of being able to immediately clean up the
pending_irq and free it right away.

So I checked all call sites of irq_to_pending (see below). A number of
them is *not* used for LPIs (but only for SPIs, for instance). An LPI
number will never make it to those places.
A number of calls is already protected in patch 10/34 in the v7, and I
just added one case.
Interestingly all those calls use the retrieved pending_irq pointer
either already under the VCPU VGIC lock or can be easily moved under it
without changing any semantics. Since irq_to_pending right now was just
a wrapper around some static array lookup, so far we just didn't bother
with taking the lock before or after the irq_to_pending call. Changing
this now doesn't change anything for the existing code.

So this means that the pending_irq pointer is only retrieved and used
under the lock - which gives us the guarantee that it's always valid to
use. So as long as we hold the VGIC VCPU lock while we replace the radix
tree entry we can't never have a use-after-free race.

So this is the summary based on "git grep irq_to_pending":
xen/arch/arm/gic.c:
gic_route_irq_to_guest(): only called for SPIs
gic_remove_irq_from_guest(): only called for SPIs
gic_remove_from_queues(): PROTECTED, VCPU VGIC lock
gic_raise_inflight_irq(): PROTECTED, VCPU VGIC lock
gic_raise_guest_irq(): TO BE PROTECTED, VCPU VGIC lock
gic_update_one_lr(): PROTECTED, VCPU VGIC lock

xen/arch/arm/vgic.c:
vgic_migrate_irq(): not called for LPIs (not for virtual IRQs)
arch_move_irqs(): not iterating over LPIs
vgic_disable_irqs(): not called for LPIs
vgic_enable_irqs(): not called for LPIs
vgic_vcpu_inject_irq(): PROTECTED, moved under VCPU VGIC lock

xen/include/asm-arm/event.h:
local_events_need_delivery_nomask(): only called for a PPI

xen/include/asm-arm/vgic.h:
(prototype)

So here is the suggestion (yet another one, I know):
- DISCARD translates the DevID/EvID pair to a vCPU/vLPI pair. This is
done under the vcmd lock and ITS lock. We then lock the respective vCPU
VGIC lock, take the radix tree lock and remove the pending_irq from the
radix tree (making it read NULL), drop the radix tree lock and vCPU VGIC
lock again. We clean up the pending_irq (removing from lists) under the
lock still.
Any VGIC code either sees a valid pending_irq pointer (and uses it only
while holding the vCPU lock!) or a NULL pointer, and due to patch 10/34
can deal with this.
A VCPU returning will just clear the LR when it reads NULL (because
there is nothing to handle).

- MAPD(V=0) just does the above in a loop with every mapped event. At
the end it can safely free the pend_irq array.

I implemented that along with addressing the other comments and pushed
this to
http://www.linux-arm.org/git?p=xen-ap.git;a=shortlog;h=refs/heads/its/v8-pre

Maybe we can use this as a discussion base?

Cheers,
Andre.

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