Re: [PATCH v7 01/12] vpci: introduce per-domain lock to protect vpci structure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.07.2023 10:59, Roger Pau Monné wrote:
> On Wed, Jul 05, 2023 at 09:11:10AM +0200, Jan Beulich wrote:
>> On 04.07.2023 23:03, Volodymyr Babchuk wrote:
>>> I am currently implementing your proposal (along with Jan's
>>> suggestions), but I am facing ABBA deadlock with IOMMU's
>>> reassign_device() call, which has this piece of code:
>>>
>>>         list_move(&pdev->domain_list, &target->pdev_list);
>>>
>>> My immediate change was:
>>>
>>>         write_lock(&pdev->domain->pci_lock);
>>>         list_del(&pdev->domain_list);
>>>         write_unlock(&pdev->domain->pci_lock);
>>>
>>>         write_lock(&target->pci_lock);
>>>         list_add(&pdev->domain_list, &target->pdev_list);
>>>         write_unlock(&target->pci_lock);
>>>
>>> But this will not work because reassign_device is called from
>>> pci_release_devices() which iterates over d->pdev_list, so we need to
>>> take a d->pci_lock early.
>>>
>>> Any suggestions on how to fix this? My idea is to remove a device from a
>>> list one at time:
>>>
>>> int pci_release_devices(struct domain *d)
>>> {
>>>     struct pci_dev *pdev;
>>>     u8 bus, devfn;
>>>     int ret;
>>>
>>>     pcidevs_lock();
>>>     write_lock(&d->pci_lock);
>>>     ret = arch_pci_clean_pirqs(d);
>>>     if ( ret )
>>>     {
>>>         pcidevs_unlock();
>>>         write_unlock(&d->pci_lock);
>>>         return ret;
>>>     }
>>>
>>>     while ( !list_empty(&d->pdev_list) )
>>>     {
>>>         pdev = list_entry(&d->pdev_list, struct pci_dev, domain_list);
>>>         bus = pdev->bus;
>>>         devfn = pdev->devfn;
>>>         list_del(&pdev->domain_list);
>>>         write_unlock(&d->pci_lock);
>>>         ret = deassign_device(d, pdev->seg, bus, devfn) ?: ret;
>>>         write_lock(&d->pci_lock);
>>
>> I think it needs doing almost like this, but with two more tweaks and
>> no list_del() right here (first and foremost to avoid needing to
>> figure whether removing early isn't going to subtly break anything;
>> see below for an error case that would end up with changed behavior):
>>
>>     while ( !list_empty(&d->pdev_list) )
>>     {
>>         const struct pci_dev *pdev = list_first_entry(&d->pdev_list, struct 
>> pci_dev, domain_list);
>>         uint16_t seg = pdev->seg;
>>         uint8_t bus = pdev->bus;
>>         uint8_t devfn = pdev->devfn;
>>
>>         write_unlock(&d->pci_lock);
> 
> I think you need to remove the device from the pdev_list before
> dropping the lock, or else release could race with other operations.
> 
> That's unlikely, but still if the lock is dropped and the routine
> needs to operate on the device it is better remove such device from
> the domain so other operations cannot get a reference to it.
> 
> Otherwise you could modify reassign_device() implementations so they
> require the caller to hold the source->pci_lock when calling the
> routine, but that's ugly because the lock would need to be dropped in
> order to reassign the device from source to target domains.
> 
> Another option would be to move the whole d->pdev_list to a local
> variable (so that d->pdev_list would be empty) and then iterate over
> it without the d->pci_lock.  On failure you would take the lock and
> add the failing device back into d->pdev_list.

Conceptually I like this last variant, but like the individual list_del()
it requires auditing code for no dependency on the device still being on
that list. In fact deassign_device()'s use of pci_get_pdev() does. The
function would then need changing to have struct pci_dev * passed in.
Yet who knows where else there are uses of pci_get_pdev() lurking.

Jan