On Mon, Oct 23, 2023 at 5:29 AM David Hildenbrand david@redhat.com wrote:
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.
I don't see anything special done for mremap. Do you have something in mind?
/* 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).
As you noticed later, both should have VM_WRITE.
/** 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.
Ok, I'll look into shortening it.
** 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.
I think that makes sense. Not sure why the original implementation needed the check for source too. Seems unnecessary.
*/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".
Will do. I guess Andrea had plans to cover tmpfs as well.
/** 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;+}
Thanks, Suren.
-- Cheers,
David / dhildenb
-- To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com.