Re: [PATCH 3/3] vfio-pci: Invalidate mmaps and block MMIO access on disabled memory

[Jason Gunthorpe]

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