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

[Sean Christopherson]

On Mon, Sep 07, 2026, David Hildenbrand (Arm) wrote:
> On 9/4/26 02:43, Sean Christopherson wrote:
> 
> > +		if (WARN_ON_ONCE(change != KVM_MR_CREATE))
> > +			goto err_bind;

This is buggy, it fails to set 'r', i.e. will signal success but not actually do
anything (or worse, half-do something?).  I'm just going to delete this sanity
check, as there are already existing sanity checks that save KVM from the worst
case scenario.  More below.

> > +
> > +		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?

Correct.  I added the check on "new" partly because it felt so wrong to not have
such a check, but also to guard against any future usage of the unwinding.

Oof, but calling kvm_arch_free_memslot() is safe only for CREATE operations.  For
FLAGS_ONLY operations, x86 and PPC reuse arch metadata, i.e. trying to unwind
prepartion for FLAGS_ONLY would do more harm than good.

So rather than try to provide a goto sequence, I'll add a prep patch to restrict
the kvm_gmem_bind() call to CREATE (which is a nop because it's dead code for
MOVE and FLAGS_ONLY), and then this patch can do:

	if (change == KVM_MR_CREATE && (new->flags & KVM_MEM_GUEST_MEMFD)) {
		r = kvm_gmem_bind(kvm, new, gmem_fd, gmem_offset);
		if (r) {
			kvm_arch_free_memslot(kvm, new);
			kvm_destroy_dirty_bitmap(new);
			goto err;
		}
	}

That addresses the new-can't-be-NULL concern as well as the duplicate code concern,
and can also address the bad sanity check above by adjusting the TODO comment in
kvm_commit_memory_region() about what needs to happen if/when dirty logging is
supported (KVM needs to rebind() here, not do separate bind()+unbind() calls).

And of course calling kvm_destroy_dirty_bitmap() is dead code until dirty logging
of guest_memfd memslots is supported, but it's harmless and IMO far less risky than
hoping future us remembers to add the call when dirty logging support comes along.

> > +		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.