| Message ID | 159017449210.18853.15037950701494323009.stgit@gimli.home (mailing list archive) |
|---|---|
| Headers | show |
| Series | vfio-pci: Block user access to disabled device MMIO | expand |
On Fri, May 22, 2020 at 01:17:09PM -0600, Alex Williamson wrote: > v3: > > The memory_lock semaphore is only held in the MSI-X path for callouts > to functions that may access MSI-X MMIO space of the device, this > should resolve the circular locking dependency reported by Qian > (re-testing very much appreciated). I've also incorporated the > pci_map_rom() and pci_unmap_rom() calls under the memory_lock. Commit > 0cfd027be1d6 ("vfio_pci: Enable memory accesses before calling > pci_map_rom") made sure memory was enabled on the info path, but did > not provide locking to protect that state. The r/w path of the BAR > access is expanded to include ROM mapping/unmapping. Unless there > are objections, I'll plan to drop v2 from my next branch and replace > it with this. Thanks, FYI, the lockdep warning is gone.
On Fri, 22 May 2020 18:08:58 -0400 Qian Cai <cai@lca.pw> wrote: > On Fri, May 22, 2020 at 01:17:09PM -0600, Alex Williamson wrote: > > v3: > > > > The memory_lock semaphore is only held in the MSI-X path for callouts > > to functions that may access MSI-X MMIO space of the device, this > > should resolve the circular locking dependency reported by Qian > > (re-testing very much appreciated). I've also incorporated the > > pci_map_rom() and pci_unmap_rom() calls under the memory_lock. Commit > > 0cfd027be1d6 ("vfio_pci: Enable memory accesses before calling > > pci_map_rom") made sure memory was enabled on the info path, but did > > not provide locking to protect that state. The r/w path of the BAR > > access is expanded to include ROM mapping/unmapping. Unless there > > are objections, I'll plan to drop v2 from my next branch and replace > > it with this. Thanks, > > FYI, the lockdep warning is gone. > Thank you for testing! Alex
v3: The memory_lock semaphore is only held in the MSI-X path for callouts to functions that may access MSI-X MMIO space of the device, this should resolve the circular locking dependency reported by Qian (re-testing very much appreciated). I've also incorporated the pci_map_rom() and pci_unmap_rom() calls under the memory_lock. Commit 0cfd027be1d6 ("vfio_pci: Enable memory accesses before calling pci_map_rom") made sure memory was enabled on the info path, but did not provide locking to protect that state. The r/w path of the BAR access is expanded to include ROM mapping/unmapping. Unless there are objections, I'll plan to drop v2 from my next branch and replace it with this. Thanks, Alex v2: Locking in 3/ is substantially changed to avoid the retry scenario within the fault handler, therefore a caller who does not allow retry will no longer receive a SIGBUS on contention. IOMMU invalidations are still not included here, I expect that will be a future follow-on change as we're not fundamentally changing that issue in this series. The 'add to vma list only on fault' behavior is also still included here, per the discussion I think it's still a valid approach and has some advantages, particularly in a VM scenario where we potentially defer the mapping until the MMIO BAR is actually DMA mapped into the VM address space (or the guest driver actually accesses the device if that DMA mapping is eliminated at some point). Further discussion and review appreciated. Thanks, Alex v1: Add tracking of the device memory enable bit and block/fault accesses to device MMIO space while disabled. This provides synchronous fault handling for CPU accesses to the device and prevents the user from triggering platform level error handling present on some systems. Device reset and MSI-X vector table accesses are also included such that access is blocked across reset and vector table accesses do not depend on the user configuration of the device. This is based on the vfio for-linus branch currently in next, making use of follow_pfn() in vaddr_get_pfn() and therefore requiring patch 1/ to force the user fault in the case that a PFNMAP vma might be DMA mapped before user access. Further PFNMAP iommu invalidation tracking is not yet included here. As noted in the comments, I'm copying quite a bit of the logic from rdma code for performing the zap_vma_ptes() calls and I'm also attempting to resolve lock ordering issues in the fault handler to lockdep's satisfaction. I appreciate extra eyes on these sections in particular. I expect this to be functionally equivalent for any well behaved userspace driver, but obviously there is a potential for the user to get -EIO or SIGBUS on device access. The device is provided to the user enabled and device resets will restore the command register, so by my evaluation a user would need to explicitly disable the memory enable bit to trigger these faults. We could potentially remap vmas to a zero page rather than SIGBUS if we experience regressions, but without known code requiring that, SIGBUS seems the appropriate response to this condition. Thanks, Alex --- Alex Williamson (3): vfio/type1: Support faulting PFNMAP vmas vfio-pci: Fault mmaps to enable vma tracking vfio-pci: Invalidate mmaps and block MMIO access on disabled memory drivers/vfio/pci/vfio_pci.c | 349 ++++++++++++++++++++++++++++++++--- drivers/vfio/pci/vfio_pci_config.c | 36 +++- drivers/vfio/pci/vfio_pci_intrs.c | 14 + drivers/vfio/pci/vfio_pci_private.h | 15 ++ drivers/vfio/pci/vfio_pci_rdwr.c | 24 ++ drivers/vfio/vfio_iommu_type1.c | 36 +++- 6 files changed, 435 insertions(+), 39 deletions(-)