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

From: "Roger Pau Monné" <roger.pau@citrix.com>
To: Stewart Hildebrand <stewart.hildebrand@amd.com>
Cc: Jan Beulich <jbeulich@suse.com>,
	Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>,
	Andrew Cooper <andrew.cooper3@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:21:54 +0100	[thread overview]
Message-ID: <ZbDIoobsFmx-U5Xt@macbook> (raw)
In-Reply-To: <e05e63ab-a86f-45c4-bd80-2b7d6b99aa3d@amd.com>

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.

Regards, Roger.