On Fri, May 01, 2020 at 03:39:30PM -0600, Alex Williamson wrote:
> static int vfio_pci_add_vma(struct vfio_pci_device *vdev,
> struct vm_area_struct *vma)
> {
> @@ -1346,15 +1450,49 @@ static vm_fault_t vfio_pci_mmap_fault(struct vm_fault *vmf)
> {
> struct vm_area_struct *vma = vmf->vma;
> struct vfio_pci_device *vdev = vma->vm_private_data;
> + vm_fault_t ret = VM_FAULT_NOPAGE;
>
> - if (vfio_pci_add_vma(vdev, vma))
> - return VM_FAULT_OOM;
> + /*
> + * Zap callers hold memory_lock and acquire mmap_sem, we hold
> + * mmap_sem and need to acquire memory_lock to avoid races with
> + * memory bit settings. Release mmap_sem, wait, and retry, or fail.
> + */
> + if (unlikely(!down_read_trylock(&vdev->memory_lock))) {
> + if (vmf->flags & FAULT_FLAG_ALLOW_RETRY) {
> + if (vmf->flags & FAULT_FLAG_RETRY_NOWAIT)
> + return VM_FAULT_RETRY;
> +
> + up_read(&vma->vm_mm->mmap_sem);
> +
> + if (vmf->flags & FAULT_FLAG_KILLABLE) {
> + if (!down_read_killable(&vdev->memory_lock))
> + up_read(&vdev->memory_lock);
> + } else {
> + down_read(&vdev->memory_lock);
> + up_read(&vdev->memory_lock);
> + }
> + return VM_FAULT_RETRY;
> + }
> + return VM_FAULT_SIGBUS;
> + }
So, why have the wait? It isn't reliable - if this gets faulted from a
call site that can't handle retry then it will SIGBUS anyhow?
The weird use of a rwsem as a completion suggest that perhaps using
wait_event might improve things:
disable:
// Clean out the vma list with zap, then:
down_read(mm->mmap_sem)
mutex_lock(vma_lock);
list_for_each_entry_safe()
// zap and remove all vmas
pause_faults = true;
mutex_write(vma_lock);
fault:
// Already have down_read(mmap_sem)
mutex_lock(vma_lock);
while (pause_faults) {
mutex_unlock(vma_lock)
wait_event(..., !pause_faults)
mutex_lock(vma_lock)
}
list_add()
remap_pfn()
mutex_unlock(vma_lock)
enable:
pause_faults = false
wake_event()
The only requirement here is that while inside the write side of
memory_lock you cannot touch user pages (ie no copy_from_user/etc)
Jason
