Re: [PATCH v3 2/3] userfaultfd: UFFDIO_MOVE uABI

Focusing on validate_remap_areas():

+static int validate_remap_areas(struct vm_area_struct *src_vma,

• 		struct vm_area_struct *dst_vma)
  

+{

• /* Only allow remapping if both have the same access and protection */
• if ((src_vma->vm_flags & VM_ACCESS_FLAGS) != (dst_vma->vm_flags & VM_ACCESS_FLAGS) ||

•    pgprot_val(src_vma->vm_page_prot) != pgprot_val(dst_vma->vm_page_prot))
  

• return -EINVAL;
  

Makes sense. I do wonder about pkey and friends and if we even have to so anything special.

• /* Only allow remapping if both are mlocked or both aren't */
• if ((src_vma->vm_flags & VM_LOCKED) != (dst_vma->vm_flags & VM_LOCKED))

• return -EINVAL;
  

• if (!(src_vma->vm_flags & VM_WRITE) || !(dst_vma->vm_flags & VM_WRITE))

• return -EINVAL;
  

Why does one of both need VM_WRITE? If one really needs it, then the destination (where we're moving stuff to).

• /*

• * Be strict and only allow remap_pages if either the src or
  

• * dst range is registered in the userfaultfd to prevent
  

• * userland errors going unnoticed. As far as the VM
  

• * consistency is concerned, it would be perfectly safe to
  

• * remove this check, but there's no useful usage for
  

• * remap_pages ouside of userfaultfd registered ranges. This
  

• * is after all why it is an ioctl belonging to the
  

• * userfaultfd and not a syscall.
  

I think the last sentence is the important bit and the comment can likely be reduced.

• *
  

• * Allow both vmas to be registered in the userfaultfd, just
  

• * in case somebody finds a way to make such a case useful.
  

• * Normally only one of the two vmas would be registered in
  

• * the userfaultfd.
  

Should we just check the destination? That makes most sense to me, because with uffd we are resolving uffd-events. And just like copy/zeropage we want to resolve a page fault ("userfault") of a non-present page on the destination.

• */
  

• if (!dst_vma->vm_userfaultfd_ctx.ctx &&

•    !src_vma->vm_userfaultfd_ctx.ctx)
  

• return -EINVAL;
  

• /*

• * FIXME: only allow remapping across anonymous vmas,
  

• * tmpfs should be added.
  

• */
  

• if (!vma_is_anonymous(src_vma) || !vma_is_anonymous(dst_vma))

• return -EINVAL;
  

Why a FIXME here? Just drop the comment completely or replace it with "We only allow to remap anonymous folios accross anonymous VMAs".

• /*

• * Ensure the dst_vma has a anon_vma or this page
  

• * would get a NULL anon_vma when moved in the
  

• * dst_vma.
  

• */
  

• if (unlikely(anon_vma_prepare(dst_vma)))

• return -ENOMEM;
  

Makes sense.

• return 0;

+}