Re: [PATCH v4 02/11] vpci: cancel pending map/unmap on vpci removal

[Jan Beulich <jbeulich@xxxxxxxx>]

On 16.11.2021 14:41, Oleksandr Andrushchenko wrote:
> 
> 
> On 16.11.21 10:23, Oleksandr Andrushchenko wrote:
>>
>> On 16.11.21 10:01, Jan Beulich wrote:
>>> On 16.11.2021 08:32, Oleksandr Andrushchenko wrote:
>>>> On 15.11.21 18:56, Jan Beulich wrote:
>>>>> On 05.11.2021 07:56, Oleksandr Andrushchenko wrote:
>>>>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@xxxxxxxx>
>>>>>>
>>>>>> When a vPCI is removed for a PCI device it is possible that we have
>>>>>> scheduled a delayed work for map/unmap operations for that device.
>>>>>> For example, the following scenario can illustrate the problem:
>>>>>>
>>>>>> pci_physdev_op
>>>>>>       pci_add_device
>>>>>>           init_bars -> modify_bars -> defer_map -> 
>>>>>> raise_softirq(SCHEDULE_SOFTIRQ)
>>>>>>       iommu_add_device <- FAILS
>>>>>>       vpci_remove_device -> xfree(pdev->vpci)
>>>>>>
>>>>>> leave_hypervisor_to_guest
>>>>>>       vpci_process_pending: v->vpci.mem != NULL; v->vpci.pdev->vpci == 
>>>>>> NULL
>>>>>>
>>>>>> For the hardware domain we continue execution as the worse that
>>>>>> could happen is that MMIO mappings are left in place when the
>>>>>> device has been deassigned
>>>>> Is continuing safe in this case? I.e. isn't there the risk of a NULL
>>>>> deref?
>>>> I think it is safe to continue
>>> And why do you think so? I.e. why is there no race for Dom0 when there
>>> is one for DomU?
>> Well, then we need to use a lock to synchronize the two.
>> I guess this needs to be pci devs lock unfortunately
> The parties involved in deferred work and its cancellation:
> 
> MMIO trap -> vpci_write -> vpci_cmd_write -> modify_bars -> defer_map
> 
> Arm: leave_hypervisor_to_guest -> check_for_vcpu_work -> vpci_process_pending
> 
> x86: two places -> hvm_do_resume -> vpci_process_pending
> 
> So, both defer_map and vpci_process_pending need to be synchronized with
> pcidevs_{lock|unlock).

If I was an Arm maintainer, I'm afraid I would object to the pcidevs lock
getting used in leave_hypervisor_to_guest.

Jan