On Wed, Feb 10, 2021 at 10:00:21AM -0800, Axel Rasmussen wrote:

quoted

quoted

 static __always_inline ssize_t mfill_atomic_pte(struct mm_struct *dst_mm,

@@ -417,10 +416,14 @@ static __always_inline ssize_t mfill_atomic_pte(struct mm_struct *dst_mm,
                                              unsigned long dst_addr,
                                              unsigned long src_addr,
                                              struct page **page,
-                                             bool zeropage,
+                                             enum mcopy_atomic_mode mode,
                                              bool wp_copy)
 {
      ssize_t err;
+     bool zeropage = (mode == MCOPY_ATOMIC_ZEROPAGE);
+
+     if (mode == MCOPY_ATOMIC_CONTINUE)
+             return -EINVAL;

So you still passed in the mode into mfill_atomic_pte() just to make sure
CONTINUE is not called there.  It's okay, but again I think it's not extremely
necessary: we should make sure to fail early at the entry of uffdio_continue()
by checking against the vma type to be hugetlb, rather than reaching here.

Hmm, it's not quite as simple as that. We don't have the dst_vma yet
in uffdio_continue(), __mcopy_atomic looks it up.

I'd prefer not to look it up in uffdio_continue(), because I think
that means changing the API so all the ioctls look up the vma, and
then plumb it into __mcopy_atomic. (We don't want to look it up twice,
since each lookup has to traverse the rbtree.) This is complicated too
by the fact that the ioctl handlers would need to perform various
validation / checks - e.g., acquiring mmap_lock, dealing with
*mmap_changing, validating the range, ....

Sure.

We can move the enforcement up one more layer, into __mcopy_atomic,
easily enough, though.

Right, that sounds good to me.  It should be right after the "if
(!vma_is_anonymous(dst_vma) && !vma_is_shmem(dst_vma))" check.

Thanks,

-- 
Peter Xu