Re: [PATCH v12.2 01/15] vpci: use per-domain PCI lock to protect vpci structure

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.01.2024 06:07, Stewart Hildebrand wrote:
> On 1/23/24 09:29, Jan Beulich wrote:
>> On 15.01.2024 20:43, Stewart Hildebrand wrote:
>>> @@ -1043,11 +1043,11 @@ static int __pci_enable_msix(struct pci_dev *pdev, 
>>> struct msi_info *msi,
>>>  {
>>>      struct msi_desc *old_desc;
>>>  
>>> -    ASSERT(pcidevs_locked());
>>> -
>>>      if ( !pdev || !pdev->msix )
>>>          return -ENODEV;
>>>  
>>> +    ASSERT(pcidevs_locked() || rw_is_locked(&pdev->domain->pci_lock));
>>> +
>>>      if ( msi->entry_nr >= pdev->msix->nr_entries )
>>>          return -EINVAL;
>>
>> Further looking at this - is dereferencing pdev actually safe without holding
>> the global lock?
> 
> Are you referring to the new placement of the ASSERT, which opens up the 
> possibility that pdev could be dereferenced and the function return before 
> the ASSERT? If that is what you mean, I see your point. The ASSERT was placed 
> there simply because we wanted to check that pdev != NULL first. See prior 
> discussion at [1]. Hmm.. How about splitting the pdev-checking condition? 
> E.g.:
> 
>     if ( !pdev )
>         return -ENODEV;
> 
>     ASSERT(pcidevs_locked() || rw_is_locked(&pdev->domain->pci_lock));
> 
>     if ( !pdev->msix )
>         return -ENODEV;

Yes, this is the particular arrangement I would have expected (at least
partly based on the guessing of the purpose of holding those locks).

Jan

> [1] 
> https://lore.kernel.org/xen-devel/85a52f8d-d6db-4478-92b1-2b6305769c96@xxxxxxx/