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

[Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>]

Hi Stewart,

Stewart Hildebrand <stewart.hildebrand@amd.com> writes:

> On 6/13/23 06:32, Volodymyr Babchuk wrote:
>
> ...
>
>> diff --git a/xen/drivers/vpci/vpci.c b/xen/drivers/vpci/vpci.c
>> index 652807a4a4..1270174e78 100644
>> --- a/xen/drivers/vpci/vpci.c
>> +++ b/xen/drivers/vpci/vpci.c
>> @@ -38,20 +38,32 @@ extern vpci_register_init_t *const __end_vpci_array[];
>> 
>>  void vpci_remove_device(struct pci_dev *pdev)
>>  {
>> -    if ( !has_vpci(pdev->domain) || !pdev->vpci )
>> +    struct vpci *vpci;
>> +
>> +    if ( !has_vpci(pdev->domain) )
>>          return;
>> 
>> -    spin_lock(&pdev->vpci->lock);
>> +    write_lock(&pdev->domain->vpci_rwlock);
>> +    if ( !pdev->vpci )
>> +    {
>> +        write_unlock(&pdev->domain->vpci_rwlock);
>> +        return;
>> +    }
>> +
>> +    vpci = pdev->vpci;
>> +    pdev->vpci = NULL;
>
> Here we set pdev->vpci to NULL, yet there are still a few uses
> remaining after this in vpci_remove_device. I suspect they were missed
> during a s/pdev->vpci/vpci/ search and replace.
>

Yes, you are right. Thank you, I'll fix this in the next version.

>> +    write_unlock(&pdev->domain->vpci_rwlock);
>> +
>>      while ( !list_empty(&pdev->vpci->handlers) )
>
> pdev->vpci dereferenced here
>
>>      {
>> -        struct vpci_register *r = list_first_entry(&pdev->vpci->handlers,
>> +        struct vpci_register *r = list_first_entry(&vpci->handlers,
>>                                                     struct vpci_register,
>>                                                     node);
>> 
>>          list_del(&r->node);
>>          xfree(r);
>>      }
>> -    spin_unlock(&pdev->vpci->lock);
>> +
>>      if ( pdev->vpci->msix )
>
> pdev->vpci dereferenced here
>
>>      {
>>          unsigned int i;
>> @@ -61,29 +73,33 @@ void vpci_remove_device(struct pci_dev *pdev)
>>              if ( pdev->vpci->msix->table[i] )
>
> pdev->vpci dereferenced here, and two more above not shown in the diff context
>
>>                  iounmap(pdev->vpci->msix->table[i]);
>
> pdev->vpci dereferenced here
>
>>      }
>> -    xfree(pdev->vpci->msix);
>> -    xfree(pdev->vpci->msi);
>> -    xfree(pdev->vpci);
>> -    pdev->vpci = NULL;
>> +    xfree(vpci->msix);
>> +    xfree(vpci->msi);
>> +    xfree(vpci);
>>  }


-- 
WBR, Volodymyr