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;
[1]
https://lore.kernel.org/xen-devel/[email protected]/
