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

[André Przywara <andre.przywara@xxxxxxx>]

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