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

[Stewart Hildebrand <stewart.hildebrand@xxxxxxx>]

On 1/24/24 03:21, Roger Pau Monné wrote:
> On Wed, Jan 24, 2024 at 12:07:28AM -0500, 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?
> 
> It is safe because either the global pcidevs lock or the per-domain
> pci_lock will be held, which should prevent the device from being
> removed.
> 
>> 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;
> 
> I'm not specially worried about the position of the assert, those are
> just debug messages at the end.
> 
> One worry I have after further looking at the code, when called from
> ns16550_init_postirq(), does the device have pdev->domain set?
> 
> That case would satisfy the first condition of the assert, so won't
> attempt to dereference pdev->domain, but still would be good to ensure
> consistency here wrt the state of pdev->domain.

Indeed. How about this?

    if ( !pdev )
        return -ENODEV;

    ASSERT(pcidevs_locked() ||
           (pdev->domain && rw_is_locked(&pdev->domain->pci_lock)));

    if ( !pdev->msix )
        return -ENODEV;

And similarly in __pci_enable_msi(), without the !pdev->msix check. And 
similarly in pci_enable_msi(), which then should also gain its own if ( !pdev ) 
return -ENODEV; check.