Re: [PATCH v5 4/6] KVM: guest_memfd: Split bind() into prepare()+commit() phases

[Ackerley Tng]

Sean Christopherson <seanjc@xxxxxxxxxx> writes:

> Split binding a memslot to a guest_memfd instance into prepare() and
> commit() phases so that KVM can separate preparing the memslot from binding
> the memslot to the gmem instance, i.e. from committing the memslot.  This
> will allow waiting to commit the memslot+gmem binding until the memslot is
> fully prepared, which is necessary as the memslot becomes reachable when
> the binding is created.
>
> As a bonus, drop the unwind-on-failure from the commit phase (other than
> nullifying the bindings), as the only reason bind() did the full unwind is
> because it technically didn't own the memslot, i.e. "needed" to leave
> memslot in the same state it started in.
>
> No functional change intended (the unwinding down on bind() failure was
> effectively dead code since KVM simply deletes the memslot on failure,
> i.e. there was nothing that could actually observe the unwind).
>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  virt/kvm/guest_memfd.c | 68 ++++++++++++++++++++++++------------------
>  virt/kvm/guest_memfd.h | 19 ++++++++----
>  virt/kvm/kvm_main.c    | 18 ++++++++++-
>  3 files changed, 70 insertions(+), 35 deletions(-)
>
> diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> index c094611f7c7a..80932f4ec4a3 100644
> --- a/virt/kvm/guest_memfd.c
> +++ b/virt/kvm/guest_memfd.c
> @@ -641,15 +641,14 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
>  	return __kvm_gmem_create(kvm, size, flags);
>  }
>
> -int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> -		  unsigned int fd, uoff_t offset)
> +int kvm_gmem_prepare_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot,
> +				   unsigned int fd, uoff_t offset)
>  {
>  	uoff_t size = slot->npages << PAGE_SHIFT;
> -	unsigned long start, end;
>  	struct gmem_file *f;
>  	struct inode *inode;
>  	struct file *file;
> -	int r = -EINVAL;
> +

An extra empty line was added here.

>
>  	BUILD_BUG_ON(sizeof(gpa_t) != sizeof(offset));
>  	BUILD_BUG_ON(sizeof(gfn_t) != sizeof(slot->gmem.pgoff));
> @@ -673,44 +672,55 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
>  	if (!PAGE_ALIGNED(offset) || offset + size > i_size_read(inode))
>  		goto err;
>
> -	filemap_invalidate_lock(inode->i_mapping);
> -
> -	start = offset >> PAGE_SHIFT;
> -	end = start + slot->npages;
> -
> -	if (!xa_empty(&f->bindings) &&
> -	    xa_find(&f->bindings, &start, end - 1, XA_PRESENT)) {
> -		r = -EEXIST;
> -		filemap_invalidate_unlock(inode->i_mapping);
> -		goto err;
> -	}
> -
>  	/*
>  	 * memslots of flag KVM_MEM_GUEST_MEMFD are immutable to change, so
>  	 * kvm_gmem_bind() must occur on a new memslot.  Because the memslot
>  	 * is not visible yet, kvm_gmem_get_pfn() is guaranteed to see the file.
>  	 */
>  	WRITE_ONCE(slot->gmem.file, file);
> -	slot->gmem.pgoff = start;
> +	slot->gmem.pgoff = offset >> PAGE_SHIFT;
>  	if (kvm_gmem_supports_mmap(inode))
>  		slot->flags |= KVM_MEMSLOT_GMEM_ONLY;
>
> -	r = xa_err(xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL));
> -	if (r) {
> -		xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
> -		slot->gmem.file = NULL;
> -		slot->gmem.pgoff = 0;
> -		slot->flags &= ~KVM_MEMSLOT_GMEM_ONLY;
> -	}
> -	filemap_invalidate_unlock(inode->i_mapping);
> -
>  	/*
> -	 * Drop the reference to the file, even on success.  The file pins KVM,
> -	 * not the other way 'round.  Active bindings are invalidated if the
> -	 * file is closed before memslots are destroyed.
> +	 * Gift the caller a reference to the file.  The reference will be
> +	 * dropped after bindings are established, or if installing the new
> +	 * memslot ultimately fails.
>  	 */

prepare+commit is nice :)

Reviewed-by: Ackerley Tng <ackerleytng@xxxxxxxxxx>


How about this instead:

1. Move the fget() in kvm_gmem_prepare_memory_region() into
   kvm_set_memory_region()
2. Move the comment about not taking a refcount into either prepare or
   commit (I think commit is better). Both prepare and commit are
   supposed to be called with a stable file. I think commit is the stage
   where people usually expect a reference transfer, but in this case
   there isn't a reference transfer. Perhaps like:

	/*
	 * No reference is taken on the file, because when the gmem file was
	 * created, it already pins KVM. Active bindings are invalidated if the
	 * file is closed before memslots are destroyed.
	 */

3. Always fput(file) if fget() happened in kvm_set_memory_region().

The gifting seems a bit asymmetric to me. For gmem bindings, always
getting a ref on the file and always dropping would be more
symmetric.

Now that there is a commit stage, explicitly not taking a ref
on the file in the committing stage is clearer imo.

If you'd like to stack the above refactoring as a patch after [5/6],
here's my suggestion: