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

From: Jan Beulich <jbeulich@suse.com>
To: Stewart Hildebrand <stewart.hildebrand@amd.com>
Cc: "Oleksandr Andrushchenko" <oleksandr_andrushchenko@epam.com>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	"Roger Pau Monné" <roger.pau@citrix.com>, "Wei Liu" <wl@xen.org>,
	"George Dunlap" <george.dunlap@citrix.com>,
	"Julien Grall" <julien@xen.org>,
	"Stefano Stabellini" <sstabellini@kernel.org>,
	"Jun Nakajima" <jun.nakajima@intel.com>,
	"Kevin Tian" <kevin.tian@intel.com>,
	"Paul Durrant" <paul@xen.org>,
	"Volodymyr Babchuk" <volodymyr_babchuk@epam.com>,
	xen-devel@lists.xenproject.org
Subject: Re: [PATCH v12.2 01/15] vpci: use per-domain PCI lock to protect vpci structure
Date: Wed, 24 Jan 2024 09:50:55 +0100	[thread overview]
Message-ID: <16bdb126-0cee-4014-9a88-d916455e35c4@suse.com> (raw)
In-Reply-To: <e05e63ab-a86f-45c4-bd80-2b7d6b99aa3d@amd.com>

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@amd.com/