Re: [Xen-devel] [PATCH v6 10/36] ARM: GIC: Add checks for NULL pointer pending_irq's

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Fri, 7 Apr 2017, Stefano Stabellini wrote:
> On Fri, 7 Apr 2017, Julien Grall wrote:
> > Hi Andre,
> > 
> > On 04/07/2017 09:46 PM, André Przywara wrote:
> > > On 07/04/17 20:07, Stefano Stabellini wrote:
> > > > On Fri, 7 Apr 2017, Andre Przywara wrote:
> > > > > For LPIs the struct pending_irq's are somewhat dynamically allocated 
> > > > > and
> > > > > the pointers are stored in a radix tree. While I convinced myself that
> > > > > an invalid LPI number can't make it into the core code, people might 
> > > > > be
> > > > > concerned about NULL pointer dereferences.
> > > > > So add checks in some places just to be on the safe side.
> > > > > 
> > > > > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > > > 
> > > > This approach looks fragile: what if we miss one irq_to_pending call? We
> > > > need a way to avoid pending_irq structs being freed when an irq is still
> > > > inflight. Only after an irq is not inflight anymore, a struct
> > > > pending_irq could be freed.
> > > 
> > > Indeed. I was wondering if a dummy pend_irq could help on the first
> > > step: Upon unmapping, we replace the radix-tree member(s) with this one
> > > reserved instance (per domain), that would avoid the NULL pointer
> > > dereference.
> > 
> > The problem with that is the code will continue to handle it, but as it is a
> > dummy pending_irq (I understand there will be only one existing) you will
> > corrupt the list.
> 
> > > Does that sound like a possible route?
> > 
> > How about using a reference (either on the device or the pending_irq)? A
> > reference would be taken until the vLPI is correctly handled (i.e clear from
> > the LRs).
> > 
> > This would prevent complex locking.
> 
> We need to mark the pending_irq "to be freed", like we do with migration
> (GIC_IRQ_GUEST_MIGRATING marks an irq to be migrated). Afterwards, when
> the irq is ready, it will removed from the tree and freed.

I'll clarify. Andre, give a look at how vgic_migrate_irq is implemented:
first it tries to migrate the irq immediately, but if it is not
possible, it will mark the irq "to be migrated". Then, in
gic_update_one_lr, we check for the "to be migrated" flag and do the
actual migration.

We need to introduce a similar workflow here. If we can remove
pending_irq from the tree, we do it immediately. Otherwise we mark it
"to be removed" and do it later as soon as we can. We also add check for
the "to be removed" flag in other places, where appropriate, to avoid
trying to inject another LPI that is already supposed to be removed. I
hope that's clearer.

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