lkml.org 
[lkml]   [2023]   [Oct]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH v3 2/3] userfaultfd: UFFDIO_MOVE uABI
    From
    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

    \
     
     \ /
      Last update: 2023-10-23 14:31    [W:2.207 / U:0.048 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site