On 1/15/24 03:58, Jan Beulich wrote: > On 12.01.2024 19:14, Stewart Hildebrand wrote: >> From: Oleksandr Andrushchenko <[email protected]> >> >> Use the per-domain PCI read/write lock to protect the presence of the >> pci device vpci field. This lock can be used (and in a few cases is used >> right away) so that vpci removal can be performed while holding the lock >> in write mode. Previously such removal could race with vpci_read for >> example. >> >> When taking both d->pci_lock and pdev->vpci->lock, they should be >> taken in this exact order: d->pci_lock then pdev->vpci->lock to avoid >> possible deadlock situations. >> >> 1. Per-domain's pci_lock is used to protect pdev->vpci structure >> from being removed. >> >> 2. Writing the command register and ROM BAR register may trigger >> modify_bars to run, which in turn may access multiple pdevs while >> checking for the existing BAR's overlap. The overlapping check, if >> done under the read lock, requires vpci->lock to be acquired on both >> devices being compared, which may produce a deadlock. It is not >> possible to upgrade read lock to write lock in such a case. So, in >> order to prevent the deadlock, use d->pci_lock in write mode instead. >> >> All other code, which doesn't lead to pdev->vpci destruction and does >> not access multiple pdevs at the same time, can still use a >> combination of the read lock and pdev->vpci->lock. >> >> 3. Drop const qualifier where the new rwlock is used and this is >> appropriate. >> >> 4. Do not call process_pending_softirqs with any locks held. For that >> unlock prior the call and re-acquire the locks after. After >> re-acquiring the lock there is no need to check if pdev->vpci exists: >> - in apply_map because of the context it is called (no race condition >> possible) >> - for MSI/MSI-X debug code because it is called at the end of >> pdev->vpci access and no further access to pdev->vpci is made >> >> 5. Use d->pci_lock around for_each_pdev and pci_get_pdev_by_domain >> while accessing pdevs in vpci code. >> >> Suggested-by: Roger Pau Monné <[email protected]> >> Suggested-by: Jan Beulich <[email protected]> >> Signed-off-by: Oleksandr Andrushchenko <[email protected]> >> Signed-off-by: Volodymyr Babchuk <[email protected]> >> Signed-off-by: Stewart Hildebrand <[email protected]> >> Reviewed-by: Roger Pau Monné <[email protected]> > > While I know Roger did offer the tag with certain adjustments, ... > >> @@ -913,7 +911,12 @@ int vpci_msix_arch_print(const struct vpci_msix *msix) >> struct pci_dev *pdev = msix->pdev; >> >> spin_unlock(&msix->pdev->vpci->lock); >> + read_unlock(&pdev->domain->pci_lock); >> process_pending_softirqs(); >> + >> + if ( !read_trylock(&pdev->domain->pci_lock) ) >> + return -EBUSY; >> + >> /* NB: we assume that pdev cannot go away for an alive domain. >> */ >> if ( !pdev->vpci || !spin_trylock(&pdev->vpci->lock) ) >> return -EBUSY; > > ... I'm sure he was assuming you would get this right, in also > dropping the 1st-try-acquired lock when this 2nd try-lock fails.
Thanks for catching this, and I appreciate the suggestion. I'll make sure both locks are dropped if needed on all error paths in vpci_msix_arch_print(), and adjust vpci_dump_msi() accordingly. > Personally I feel this is the kind of change one would better not > offer (or take) R-b ahead of time. I'll drop Roger's R-b for v12.2. > > I further think the respective comment in vpci_dump_msi() also wants > adjusting from singular to plural. I'll fix for v12.2, thanks for suggesting this.
