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

[Julien Grall <julien.grall@xxxxxxx>]

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.
> Then we keep the existing pend_irq (array) around until we are sure that
> no-one is holding a reference anymore - either by using RCU (although I
> think this is problematic because of the rcu_read_lock) or by finding a
> definite point in time when no-one can possibly use that pointer
> anymore. The pointer usage seems to be very confined, so I am hopeful we
> can find such a limit (say: once every VCPU has exited and entered once
> or the like).
I think the RCU case is fragile because the pending_irq could be added 
in the list which will be carried even after re-entering to the guest.
> 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.

Cheers,

--
Julien Grall

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