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, 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.

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