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/[email protected]/
