[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v12.2 01/15] vpci: use per-domain PCI lock to protect vpci structure
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.
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |