Re: [PATCH v4 13/15] KVM: guest_memfd: implement userfaultfd operations

[Ackerley Tng]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

>
> [...snip...]
>
>> +#ifdef CONFIG_USERFAULTFD
>> +static bool kvm_gmem_can_userfault(struct vm_area_struct *vma, vm_flags_t vm_flags)
>> +{
>> +	struct inode *inode = file_inode(vma->vm_file);
>> +
>> +	/*
>> +	 * Only support userfaultfd for guest_memfd with INIT_SHARED flag.
>> +	 * This ensures the memory can be mapped to userspace.
>> +	 */

Is the principle here that any memory that is allowed to be mapped to
userspace can be userfault-ed?

>> +	if (!(GMEM_I(inode)->flags & GUEST_MEMFD_FLAG_INIT_SHARED))
>> +		return false;
>
> I'm not comfortable with this change.  It works for now, but it's going to be
> wildly wrong when in-place conversion comes along.  While I agree with the "Let's
> solve each problem in it's time :)"[*], the time for in-place conversion is now.
> In-place conversion isn't landing this cycle or next, but it's been in development
> for longer than UFFD support, and I'm not willing to punt solvable problems to
> that series, because it's plenty fat as is.
>

Thanks :)

> Happily, IIUC, this is an easy problem to solve, and will have a nice side effect
> for the common UFFD code.
>
> My objection to an early, global "can_userfault()" check is that it's guaranteed
> to cause TOCTOU issues.  E.g. for VM_UFFD_MISSING and VM_UFFD_MINOR, the check on
> whether or not a given address can be faulted in needs to happen in __do_userfault(),
> not broadly when VM_UFFD_MINOR is added to a VMA.  Conceptually, that also better
> aligns the code with the "normal" user fault path in kvm_gmem_fault_user_mapping().
>
> I'm definitely not asking to fully prep for in-place conversion, I just want to
> set us up for success and also to not have to churn a pile of code.  Concretely,
> again IIUC, I think we just need to move the INIT_SHARED check to ->alloc_folio()
> and ->get_folio_noalloc().  And if we extract kvm_gmem_is_shared_mem() now instead
> of waiting for in-place conversion, then we'll avoid a small amount of churn when
> in-place conversion comes along.
>

I think it would help if we do some refactoring of existing allocation
functions to provide guest_memfd's uffd ->alloc_folio and
->get_folio_noalloc callbacks instead of having new functions.

Perhaps uffd ->alloc_folio and ->get_folio_noalloc can call an alloc
function refactored out of kvm_gmem_get_folio(), and also do the
kvm_gmem_is_shared_mem() check.

+tarunsahu@xxxxxxxxxx, who is starting work on guest_memfd preservation
(as in LUO/KHO).

>
> [...snip...]
>