Re: [PATCH 15/33] userfaultfd: hugetlbfs: add __mcopy_atomic_hugetlb for huge page UFFDIO_COPY

[Andrea Arcangeli <aarcange@redhat.com>]

Hello Mike,

On Tue, Nov 08, 2016 at 01:06:06PM -0800, Mike Kravetz wrote:
> -- 
> Mike Kravetz
> 
> From: Mike Kravetz <mike.kravetz@oracle.com>
> 
> userfaultfd: hugetlbfs: fix __mcopy_atomic_hugetlb retry/error processing
> 
> The new routine copy_huge_page_from_user() uses kmap_atomic() to map
> PAGE_SIZE pages.  However, this prevents page faults in the subsequent
> call to copy_from_user().  This is OK in the case where the routine
> is copied with mmap_sema held.  However, in another case we want to
> allow page faults.  So, add a new argument allow_pagefault to indicate
> if the routine should allow page faults.
> 
> A patch (mm/hugetlb: fix huge page reservation leak in private mapping
> error paths) was recently submitted and is being added to -mm tree.  It
> addresses the issue huge page reservations when a huge page is allocated,
> and free'ed before being instantiated in an address space.  This would
> typically happen in error paths.  The routine __mcopy_atomic_hugetlb has
> such an error path, so it will need to call restore_reserve_on_error()
> before free'ing the huge page.  restore_reserve_on_error is currently
> only visible in mm/hugetlb.c.  So, add it to a header file so that it
> can be used in mm/userfaultfd.c.  Another option would be to move
> __mcopy_atomic_hugetlb into mm/hugetlb.c

It would have been better to split this in two patches.

> @@ -302,8 +302,10 @@ static __always_inline ssize_t
> __mcopy_atomic_hugetlb(struct mm_struct *dst_mm,
>  out_unlock:
>  	up_read(&dst_mm->mmap_sem);
>  out:
> -	if (page)
> +	if (page) {
> +		restore_reserve_on_error(h, dst_vma, dst_addr, page);
>  		put_page(page);
> +	}
>  	BUG_ON(copied < 0);

If the revalidation fails dst_vma could even be NULL.

We get there with page not NULL only if something in the revalidation
fails effectively... I'll have to drop the above change as the fix
will hurt more than the vma reservation not being restored. Didn't
think too much about it, but there was no obvious way to restore the
reservation of a vma, after we drop the mmap_sem. However if we don't
drop the mmap_sem, we'd recurse into it, and it'll deadlock in current
implementation if a down_write is already pending somewhere else. In
this specific case fairness is not an issue, but it's not checking
it's the same thread taking it again, so it's doesn't allow to recurse
(checking it's the same thread would make it slower).

I also fixed the gup support for userfaultfd, could you review it?
Beware, untested... will test it shortly with qemu postcopy live
migration with hugetlbfs instead of THP (that currently gracefully
complains about FAULT_FLAG_ALLOW_RETRY missing, KVM ioctl returns
badaddr and DEBUG_VM=y clearly showed the stack trace of where
FAULT_FLAG_ALLOW_RETRY was missing).

I think this enhancement is needed by Oracle too, so that you don't
get an error from I/O syscalls, and you instead get an userfault.

We need to update the selftest to trigger userfaults not only with the
CPU but with O_DIRECT too.

Note, the FOLL_NOWAIT is needed to offload the userfaults to async
page faults. KVM tries an async fault first (FOLL_NOWAIT, nonblocking
= NULL), if that fails it offload a blocking (*nonblocking = 1) fault
through async page fault kernel thread while guest scheduler schedule
away the blocked process. So the userfaults behave like SSD swapins
from disk hitting on a single guest thread and not the whole host vcpu
thread. Clearly hugetlbfs cannot ever block for I/O, FOLL_NOWAIT is
only useful to avoid blocking in the vcpu thread in
handle_userfault().