[RFC PATCH 2/3] KVM: Implement dirty page logging for guest_memfd-only memslots
Alexandru Elisei
alexandru.elisei at arm.com
Tue Jul 7 10:12:20 PDT 2026
Hi Sean,
On Mon, Jul 06, 2026 at 06:29:11PM -0700, Sean Christopherson wrote:
> On Thu, Jul 02, 2026, Alexandru Elisei wrote:
> > The entire memory represented by guest_memfd-only memslot is shared and
> > accessible by userspace.
>
> ...
>
> > +8.48 KVM_CAP_GUEST_MEMFD_MMAP_LOG_DIRTY_PAGES
> > +---------------------------------------------
> > +
> > +:Architectures: all
> > +
> > +The presence of this capability indicates that memslots backed by a guest_memfd
> > +file descriptor created with the GUEST_MEMFD_FLAG_MMAP flag can have dirty
> > +page logging enabled.
>
> What does mmap() have to do with anything? Supporting mmap() doesn't guarantee
> the memory is shared, and I can't think of any dependency on memory actually
> being mapped into userspace.
My bad, it should have been GUEST_MEMFD_FLAG_MMAP +
GUEST_MEMFD_FLAG_INIT_SHARED. I'm not sure what you mean by "dependency on
memory actually being mapped into userspace".
>From my point of view, it only makes sense to enable dirty page logging if
the contents of the memory is accessible to userspace, hence I made dirty
page logging depend on userspace having the option to access the memory.
This can only happen if the guest_memfd file is mmap'able and accessible by
userspace. But it doesn't force userspace to actually have the memory
mapped to allow the log dirty pages flag to be set for a guest_memfd backed
memslot. Hm.. now that I think about it, maybe I should have made depend on
guest_memfd also having been created as shared? Though I think that can be
changed with KVM_SET_MEMORY_ATTRIBUTES on x86.
Does that answer your question?
>
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index 43ef8e908aaf..210bdd76f0aa 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -622,6 +622,11 @@ bool __weak kvm_arch_supports_gmem_init_shared(struct kvm *kvm)
> > return true;
> > }
> >
> > +bool __weak kvm_arch_supports_gmem_mmap_dirty_logging(struct kvm *kvm)
> > +{
> > + return false;
> > +}
> > +
> > static int __kvm_gmem_create(struct kvm *kvm, loff_t size, u64 flags)
> > {
> > static const char *name = "[kvm-gmem]";
> > @@ -705,6 +710,66 @@ int kvm_gmem_create(struct kvm *kvm, struct kvm_create_guest_memfd *args)
> > return __kvm_gmem_create(kvm, size, flags);
> > }
> >
> > +static int __kvm_gmem_check_no_change(struct kvm *kvm, struct kvm_memory_slot *old,
> > + struct file *old_file, unsigned int fd,
> > + loff_t offset)
> > +{
> > + struct file *new_file;
> > +
> > + new_file = fget(fd);
> > + if (!new_file)
> > + return -EBADF;
> > + if (new_file != old_file) {
>
> There's a TOCTOU issue here, no? Nothing prevents userspace from deleting and
> replacing the old guest_memfd instance between now and when the re-binding
> happens.
When userspace closes a guest_memfd file, for all memslots that use that
file, slot->gmem.file is set to NULL in kvm_gmem_release().
kvm_gmem_release() takes the kvm->slots_lock(), so I don't think changes to
memslots can run concurrently with a guest_memfd instance being closed -
i.e, if a guest_memfd file has been closed then the subsequent memslot
update will see slot->gmem.file = NULL. Or if a memslot update is under
way, slot->gmem.file won't be made NULL until the memslot update completes.
Unless I misunderstood what you were saying.
> Ah, no, because the check in kvm_set_memory_region() is only to check
> for a "nop" update. I think we should continue to disallow such "updates", I
Sure, I added this here because calling the legacy
KVM_SET_USER_MEMORY_REGION with the same values for the struct
kvm_user_memory_region fields does not return an error.
> can't think of any reasonable use case, and then we can fold this helper into
> its sole remaining caller.
>
> > + fput(new_file);
> > + return -EBADF;
> > + }
> > + fput(new_file);
> > +
> > + if (old->gmem.pgoff != offset >> PAGE_SHIFT)
>
> This can and should be handled in common KVM, not in guest_memfd. It's a
> property of the memslot, not of the gmem instance.
Sure.
>
> > + return -EINVAL;
> > +
> > + return 0;
> > +}
> > +int kvm_gmem_change_flags(struct kvm *kvm, struct kvm_memory_slot *old,
>
> Hmm, if we use a separate helper, I think this should be phrased in terms of
> commands to guest_memfd, not in terms of why common KVM is making changes.
> guest_memfd shouldn't have to care *why* it's being asked to re-bind to a
> different memslot.
>
> Alternatively, provide kvm_gmem_commit_memory_region() and pass in a
> kvm_mr_change param, but that gets weird since the unbind() case needs to be
> handled even without an explicit DELETE.
The way I see it, guest_memfd is not rebinding to a new memslot, since
f->bindings are not changed at any point in the handling of the
KVM_SET_USER_MEMORY_REGION2 call if only the flags are changed.
The function was initially supposed to validate that this indeed is a flag
only change, which means that the guest_memfd fd and file offset are
unchanged. But then I added the update to new->flags to set the
KVM_MEMSLOT_GMEM_ONLY flag. I guess I could split it into
kvm_gmem_check_flags_update() and kvm_gmem_copy_flags()?
Also, I think the best place for both functions would be in
kvm_set_memory_region():
* That's where the validity of a change to a memslot is checked.
* That's where new->flags is copied from kvm_userspace_memory_region2->flags.
What do you think? Does that make sense?
>
> > @@ -734,6 +799,11 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
> > if (!PAGE_ALIGNED(offset) || offset + size > i_size_read(inode))
> > goto err;
> >
> > + if (slot->flags & KVM_MEM_LOG_DIRTY_PAGES &&
> > + (!kvm_gmem_supports_mmap(inode) ||
> > + !kvm_arch_supports_gmem_mmap_dirty_logging(kvm)))
>
> I think I would rather handle this in kvm_arch_prepare_memory_region(). AFAIK,
> arm64 and x86 are the only architectures that support using gmem for private
> memory, and so are the only architectures that would need to restrict dirty
> logging (TDX needs additional plumbiong). It'd mean updating x86 at the same
> time, but that should be relatively straighforward.
Sure, I'll give it a go.
>
> That would mean we couldn't handle the check in check_memory_region_flags(), but
> that should be a non-issue, e.g. arm64 and RISC-V already put additional
> restrictions on what can regions be dirty-logged.
>
> > @@ -1739,16 +1739,6 @@ static void kvm_commit_memory_region(struct kvm *kvm,
> > */
> > if (old->dirty_bitmap && !new->dirty_bitmap)
> > kvm_destroy_dirty_bitmap(old);
> > -
> > - /*
> > - * Unbind the guest_memfd instance as needed; the @new slot has
> > - * already created its own binding. TODO: Drop the WARN when
> > - * dirty logging guest_memfd memslots is supported. Until then,
> > - * flags-only changes on guest_memfd slots should be impossible.
> > - */
> > - if (WARN_ON_ONCE(old->flags & KVM_MEM_GUEST_MEMFD))
> > - kvm_gmem_unbind(old);
> > -
> > /*
> > * The final quirk. Free the detached, old slot, but only its
> > * memory, not any metadata. Metadata, including arch specific
> > @@ -2073,22 +2063,27 @@ static int kvm_set_memory_region(struct kvm *kvm,
> > if ((kvm->nr_memslot_pages + npages) < kvm->nr_memslot_pages)
> > return -EINVAL;
> > } else { /* Modify an existing slot. */
> > - /* Private memslots are immutable, they can only be deleted. */
> > - if (mem->flags & KVM_MEM_GUEST_MEMFD)
> > - return -EINVAL;
> > if ((mem->userspace_addr != old->userspace_addr) ||
> > (npages != old->npages) ||
> > ((mem->flags ^ old->flags) & (KVM_MEM_READONLY | KVM_MEM_GUEST_MEMFD)))
> > return -EINVAL;
> >
> > - if (base_gfn != old->base_gfn)
> > + if (base_gfn != old->base_gfn) {
> > change = KVM_MR_MOVE;
> > - else if (mem->flags != old->flags)
> > + } else if (mem->flags != (old->flags & MEMSLOT_USER_FLAGS_MASK)) {
> > change = KVM_MR_FLAGS_ONLY;
> > - else /* Nothing to change. */
> > + } else if (mem->flags & KVM_MEM_GUEST_MEMFD) {
> > + return kvm_gmem_check_no_change(kvm, old, mem->guest_memfd,
> > + mem->guest_memfd_offset);
>
> As above, just return -EINVAL.
Sure.
>
> > + } else {
> > return 0;
> > + }
> > }
> >
> > + if (mem->flags & KVM_MEM_GUEST_MEMFD &&
> > + change != KVM_MR_CREATE && change != KVM_MR_FLAGS_ONLY)
>
> This is a *very* convoluted way of disallowing MOVE. Handle this above. E.g.o
>
> if (base_gfn != old->base_gfn) {
> /* KVM doesn't support moving guest_memfd bindings. */
> if (mem->flags & KVM_MEM_GUEST_MEMFD)
> return -EINVAL;
>
> change = KVM_MR_MOVE;
> } else if (mem->flags != (old->flags & MEMSLOT_USER_FLAGS_MASK)) {
> if (mem->flags & KVM_MEM_GUEST_MEMFD &&
> old->gmem.pgoff != mem->guest_memfd_offset)
> change = KVM_MR_FLAGS_ONLY;
> } else if (mem->flags & KVM_MEM_GUEST_MEMFD) {
> return -EINVAL;
> } else {
> return 0;
> }
Sure, looks better.
>
> > + return -EINVAL;
> > +
> > if ((change == KVM_MR_CREATE || change == KVM_MR_MOVE) &&
> > kvm_check_memslot_overlap(slots, id, base_gfn, base_gfn + npages))
> > return -EEXIST;
> > @@ -2105,7 +2100,12 @@ static int kvm_set_memory_region(struct kvm *kvm,
> > 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 (change == KVM_MR_CREATE) {
>
> Curly braces aren't needed.
Sure, I'll remove them, I put them there because the block for
KVM_MR_FLAGS_ONLY, which, even though has only an instruction, spans two
lines.
>
> > + r = kvm_gmem_bind(kvm, new, mem->guest_memfd, mem->guest_memfd_offset);
> > + } else if (change == KVM_MR_FLAGS_ONLY) {
> > + r = kvm_gmem_change_flags(kvm, old, new, mem->guest_memfd,
> > + mem->guest_memfd_offset);
> > + }
> > if (r)
> > goto out;
> > }
> > @@ -2117,7 +2117,7 @@ static int kvm_set_memory_region(struct kvm *kvm,
> > return 0;
> >
> > out_unbind:
> > - if (mem->flags & KVM_MEM_GUEST_MEMFD)
> > + if ((mem->flags & KVM_MEM_GUEST_MEMFD) && change == KVM_MR_CREATE)
> > kvm_gmem_unbind(new);
>
> This is wrong. If kvm_set_memslot() failed, the old memslot needs to be bound
> back to the guest_memfd instance. Hmm, but KVM can't guarantee success. So
The old memslot is still bound to guest_memfd in the sense that f->bindings
still has the same memslot id+as_id assigned to the memslot's gpa range.
There's no rebinding happening when flags are changed because the memslot's
id + as_id are not changed.
I think we might not be on the same page regarding what the previous patch
does, that is where I've changed how guest_memfd keeps track of the
memslots. Instead of storing a pointer, it now stores the memslot id and
as_id, and uses a search in the active memslots (kvm->memslots) to get the
active memslot by using id and as_id:
slot = id_to_memslot(__kvm_memslots(kvm, as_id), id)
If userspace punches a hole or kvm_gmem_error_folio() is called while a
memslot flags update is in progress, __kvm_gmem_invalidate_{start,end}()
might observe the old memslot (with the old flags), but I believe that's
ok, because that's also what happens when the MMU notifiers trigger
invalidation during a memslot update.
> unless there's reason why the bind() can't happen under slots_arch_lock, I think
> the way to handle this is to only bind once success is guaranteed. It'll require
> plumbing the fd+offset into kvm_set_memslot().
>
> Or I guess add the "fd" to kvm_memory_slot.gmem? I kinda like that, because then
> we can require that userspace really is just updating flags, and not switching
> the fd (to a the same file).
>
>
> E.g. something like this?
>
> diff --git include/linux/kvm_host.h include/linux/kvm_host.h
> index ab8cfaec82d3..82385eb9a82e 100644
> --- include/linux/kvm_host.h
> +++ include/linux/kvm_host.h
> @@ -610,6 +610,7 @@ struct kvm_memory_slot {
> * reference via kvm_gmem_get_file() is protected by
> * either kvm->slots_lock or kvm->srcu.
> */
> + int fd;
> struct file *file;
> pgoff_t pgoff;
> } gmem;
> diff --git virt/kvm/kvm_main.c virt/kvm/kvm_main.c
> index e44c20c04961..0729e7c94816 100644
> --- virt/kvm/kvm_main.c
> +++ virt/kvm/kvm_main.c
> @@ -1946,6 +1946,20 @@ static int kvm_set_memslot(struct kvm *kvm,
> return r;
> }
>
> + if (new->flags & KVM_MEM_GUEST_MEMFD) {
> + if (change == KVM_MR_CREATE)
> + r = kvm_gmem_bind(...);
> + else if (WARN_ON_ONCE(change == KVM_MR_MOVE))
> + r = -EINVAL;
> + else if (change == KVM_MR_FLAGS_ONLY)
> + r = kvm_gmem_rebind(...);
> +
> + if (r) {
> + mutex_unlock(&kvm->slots_arch_lock);
> + return r;
> + }
> + }
> +
> /*
> * 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
> @@ -2104,21 +2118,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;
> - }
> + new->gmem.fd = mem->guest_memfd;
> + new->gmem.pgoff = mem->guest_memfd_offset >> PAGE_SHIFT;
>
> r = kvm_set_memslot(kvm, old, new, change);
> 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;
I can explore that, for sure, thank you for the review!
Alex
More information about the linux-arm-kernel
mailing list