Messages in this thread | | | Date | Mon, 23 Oct 2023 14:29:34 +0200 | Subject | Re: [PATCH v3 2/3] userfaultfd: UFFDIO_MOVE uABI | From | David Hildenbrand <> |
| |
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
| |