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;
+}
--
Cheers,
David / dhildenb
