[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v6 10/36] ARM: GIC: Add checks for NULL pointer pending_irq's
On 07/04/17 23:09, Stefano Stabellini wrote: > 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. Indeed, that sounds like a good idea. Thanks for that! Now we just need to find a neat way of dealing with the array nature of our pend_irq allocation, so we can only free it when *all* LPIs from that device have been removed. But I am confident to find something. Cheers, Andre. _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx https://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |