Re: [PATCH v3 3/4] KVM: guest_memfd: Establish memslot<=>guest_memfd bindings *after* memslot is ready

[David Hildenbrand (Arm)]

On 9/4/26 02:43, Sean Christopherson wrote:
> Wait to bind a memslot to a guest_memfd instance until *after* the memslot
> is fully prepared, as creating the binding in guest_memfd will effectively
> expose the memslot to readers.  As pointed out by Sashiko, binding the
> memslot before it's ready to be exposed to the rest of the world can break
> various memslot assumption and rules.  E.g. x86 could observe a NULL rmap
> pointer if a PUNCH_HOLE hit the guest_memfd after the binding was created,
> but before KVM made it through kvm_prepare_memory_region().
> 
> Begrudgingly resort to passing in the guest_memfd fd+offset pair to
> kvm_set_memslot(), as creating the binding really does need to happen in
> the middle of setting the new memslot.  Alternatively, to preserve the
> aesthetically pleasing function prototype, "struct kvm_memory_slot" could
> be expanded to track the fd and the file, but that would create the
> possibility for TOCTOU bugs on the fd vs. file, and would add zero value
> beyond making kvm_set_memslot() look pretty.
> 
> Fixes:a7800aa80ea4 ("KVM: Add KVM_CREATE_GUEST_MEMFD ioctl() for guest-specific backing memory")
> Cc: stable@xxxxxxxxxxxxxxx
> Reported-by: Sashiko Bot <sashiko-bot@xxxxxxxxxx>
> Closes: https://lore.kernel.org/all/20260826170551.BEF801F000E9@xxxxxxxxxxxxxxx
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>  virt/kvm/kvm_main.c | 34 ++++++++++++++++++++++------------
>  1 file changed, 22 insertions(+), 12 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 3c0dbe60a5b4..21c10cbbac66 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -1887,7 +1887,8 @@ static void kvm_update_flags_memslot(struct kvm *kvm,
>  static int kvm_set_memslot(struct kvm *kvm,
>  			   struct kvm_memory_slot *old,
>  			   struct kvm_memory_slot *new,
> -			   enum kvm_mr_change change)
> +			   enum kvm_mr_change change,
> +			   unsigned int gmem_fd, uoff_t gmem_offset)
>  {
>  	struct kvm_memory_slot *invalid_slot;
>  	int r;
> @@ -1934,6 +1935,15 @@ static int kvm_set_memslot(struct kvm *kvm,
>  	if (r)
>  		goto err;
>  
> +	if (new && new->flags & KVM_MEM_GUEST_MEMFD) {

For readability I'd throw in an extra pair of (). But KVM seems to use both
styles, so there is no clear preference when staring at the existing code :)

> +		if (WARN_ON_ONCE(change != KVM_MR_CREATE))
> +			goto err_bind;
> +
> +		r = kvm_gmem_bind(kvm, new, gmem_fd, gmem_offset);
> +		if (r)
> +			goto err_bind;
> +	}
> +
>  	/*
>  	 * For DELETE and MOVE, the working slot is now active as the INVALID
>  	 * version of the old slot.  MOVE is particularly special as it reuses
> @@ -1965,6 +1975,13 @@ static int kvm_set_memslot(struct kvm *kvm,
>  
>  	return 0;
>  
> +err_bind:
> +	if (new) {

We'd never end up here with !new, right?

> +		kvm_arch_free_memslot(kvm, new);
> +
> +		if (new->dirty_bitmap && (!old || !old->dirty_bitmap))
> +			kvm_destroy_dirty_bitmap(new);

That's essentially the cleanup path in kvm_prepare_memory_region().

I guess with some more reshuffling we could have a single dirty bitmap cleanup
path in this code.

> +	}
>  err:
>  	/*
>  	 * For DELETE/MOVE, revert the above INVALID change.  No modifications
> @@ -2059,7 +2076,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
>  		if (WARN_ON_ONCE(kvm->nr_memslot_pages < old->npages))
>  			return -EIO;
>  
> -		return kvm_set_memslot(kvm, old, NULL, KVM_MR_DELETE);
> +		return kvm_set_memslot(kvm, old, NULL, KVM_MR_DELETE, -1, 0);
>  	}
>  
>  	base_gfn = (mem->guest_phys_addr >> PAGE_SHIFT);
> @@ -2106,21 +2123,14 @@ static int kvm_set_memory_region(struct kvm *kvm,
>  	new->npages = npages;
>  	new->flags = mem->flags;
>  	new->userspace_addr = mem->userspace_addr;
> -	if (mem->flags & KVM_MEM_GUEST_MEMFD) {
> -		r = kvm_gmem_bind(kvm, new, mem->guest_memfd, mem->guest_memfd_offset);
> -		if (r)
> -			goto out;
> -	}
>  
> -	r = kvm_set_memslot(kvm, old, new, change);
> +	r = kvm_set_memslot(kvm, old, new, change,
> +			    mem->guest_memfd, mem->guest_memfd_offset);
>  	if (r)
> -		goto out_unbind;
> +		goto out;
>  
>  	return 0;
>  
> -out_unbind:
> -	if (mem->flags & KVM_MEM_GUEST_MEMFD)
> -		kvm_gmem_unbind(new);
>  out:
>  	kfree(new);
>  	return r;

In general LGTM.

-- 
Cheers,

David