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

[Sean Christopherson]

On Thu, Sep 24, 2026, Sean Christopherson wrote:
> On Thu, Sep 24, 2026, Ackerley Tng wrote:
> But after fiddling with this for an hour or so, I realized that if we fully commit
> to configuring new.gmem in kvm_set_memory_region(), then we can move the prepare()
> call into kvm_prepare_memory_region() without needing to plumb extra parameters,
> and make the gmem stuff look a lot more like the rest of the memslot code.  E.g.
> 
> 	if (mem->flags & KVM_MEM_GUEST_MEMFD) {
> #ifdef CONFIG_KVM_GUEST_MEMFD
> 		new->gmem.file = fget(mem->guest_memfd);
> 		if (!new->gmem.file) {
> 			r = -EBADF;
> 			goto out;
> 		}
> 
> 		new->gmem.pgoff = mem->guest_memfd_offset >> PAGE_SHIFT;
> #endif
> 	}
> 
> 	r = kvm_set_memslot(kvm, old, new, change);
> 
> 	/*
> 	 * Drop the reference to the gmem 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.
> 	 */
> #ifdef CONFIG_KVM_GUEST_MEMFD
> 	if (new->gmem.file)
> 		fput(new->gmem.file);
> #endif
> 
> Not being able to get rid of the #ifdefs is a shame, but from a control flow
> perspective, this feels more right than anything else.  Full diff (not fully
> tested, and would need to be split into 3+ patches):

Actually, plumbing in @old and @change can wait.  As much as I want to make the
calls match the other prepare()+commit() hooks, @old and @change aren't needed
until flags-only updates come along, and adding them at that time provide a better
git history as the additional plumbing will directly precede their usage (or maybe
even be in the same patch).

> -	if (!PAGE_ALIGNED(offset) || offset + size > i_size_read(inode))
> -		goto err;
> +	if (new->gmem.pgoff + new->npages  > i_size_read(inode) >> PAGE_SHIFT)
> +		return -EINVAL;

...

> +	if (mem->flags & KVM_MEM_GUEST_MEMFD) {
> +#ifdef CONFIG_KVM_GUEST_MEMFD
> +		new->gmem.file = fget(mem->guest_memfd);
> +		if (!new->gmem.file) {
> +			r = -EBADF;
>  			goto out;
> +		}

Note, I didn't lose the offset check, the existing one in
kvm_gmem_prepare_memory_region() (nee bind()) is redundant with this one in
kvm_set_memory_region():

	if (mem->flags & KVM_MEM_GUEST_MEMFD &&
	    (mem->guest_memfd_offset & (PAGE_SIZE - 1) ||  <========
	     mem->guest_memfd_offset + mem->memory_size < mem->guest_memfd_offset))
		return -EINVAL;
> +
> +		new->gmem.pgoff = mem->guest_memfd_offset >> PAGE_SHIFT;
> +#endif
>  	}
>  
>  	r = kvm_set_memslot(kvm, old, new, change);
> @@ -2148,7 +2159,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
>  	 * the file is closed before memslots are destroyed.
>  	 */
>  #ifdef CONFIG_KVM_GUEST_MEMFD
> -	if (change == KVM_MR_CREATE && (mem->flags & KVM_MEM_GUEST_MEMFD))
> +	if (new->gmem.file)
>  		fput(new->gmem.file);
>  #endif
>