KVM: guest_memfd: fix unbind race and NUMA selftests

[Severity: High]
This isn't a bug introduced by this patch, but does ignoring the return value of xa_store_range() here and in kvm_gmem_bind() lead to a use-after-free?

Looking at kvm_gmem_bind() in virt/kvm/guest_memfd.c, xa_store_range() is called with GFP_KERNEL:

virt/kvm/guest_memfd.c:kvm_gmem_bind() {
	...
	xa_store_range(&f->bindings, start, end - 1, slot, GFP_KERNEL);
	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.
	 */
	r = 0;
	...
}

If memory allocation fails during this operation, xa_store_range() returns an error pointer, but the return value is ignored. This masks ENOMEM errors and leads to an inconsistent state where the slot points to the file but the file doesn't track the slot.

Because the slot is missing from the file's XArray, when the file is later closed, kvm_gmem_release() iterates over f->bindings but fails to find this slot, leaving slot->gmem.file as a dangling pointer.

When the VM destroys the memslot, kvm_gmem_unbind() accesses this dangling pointer and calls get_file_active() on it. Can this cause a use-after-free on the freed struct file, or improperly increment the refcount of an unrelated, reused memory object?

kvm_gmem_unbind() skips mapping->invalidate_lock when the guest_memfd file is already dying. All other paths that modify f->bindings hold that lock.

kvm_gmem_invalidate_{start,end}() checks f->bindings independently to decide whether to begin or end KVM MMU invalidations. So, the bindings must remain stable between the two calls. If a binding is removed in that window, start increments mmu_invalidate_in_progress but end does not decrement it. Example, unbind race with memory failure:

CPU 0: memory failure CPU 1: memslot delete
---------------------------------- ---------------------------
(guest_memfd file is dying)
kvm_gmem_error_folio()
kvm_gmem_invalidate_start()
finds binding
mmu_invalidate_in_progress++
kvm_gmem_unbind()
get_file_active() fails store NULL in bindings
kvm_gmem_invalidate_end()
no binding found
counter stays elevated

mmu_invalidate_retry() then returns 1 forever, so guest page faults retry without ever installing a mapping and the guest hangs.

Take the invalidate lock in the dying-file path too. This prevents unbind from removing a binding and leaking mmu_invalidate_in_progress. This is safe because any caller that reaches this path holds slots_lock, so kvm_gmem_release() cannot nullify the slot->gmem.file, until kvm_gmem_unbind() finishes.

Reported-by Sashiko <[email protected]>

Closes https://lore.kernel.org/all/[email protected] open here

Fixes ae431059e75d ("KVM: guest_memfd: Remove bindings on memslot deletion when gmem is dying")

Signed-off-by Shivank Garg <[email protected]>

---
 virt/kvm/guest_memfd.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

virt/kvm/guest_memfd.c +4 -6

index f0e5da490866..f848120af84b 100644
@@ -721,6 +721,8 @@ static void __kvm_gmem_unbind(struct kvm_memory_slot *slot, struct gmem_file *f)
 
 void kvm_gmem_unbind(struct kvm_memory_slot *slot)
 {
+	struct file *gmem_file;
+
 	/*
 	 * Nothing to do if the underlying file was _already_ closed, as
 	 * kvm_gmem_release() invalidates and nullifies all bindings.

@@ -733,21 +735,24 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
 	/*
 	 * However, if the file is _being_ closed, then the bindings need to be
 	 * removed as kvm_gmem_release() might not run until after the memslot
-	 * is freed.  Note, modifying the bindings is safe even though the file
-	 * is dying as kvm_gmem_release() nullifies slot->gmem.file under
+	 * is freed.  Note, dereferencing the dying file is safe as
+	 * kvm_gmem_release() nullifies slot->gmem.file under
 	 * slots_lock, and only puts its reference to KVM after destroying all
 	 * bindings.  I.e. reaching this point means kvm_gmem_release() hasn't
 	 * yet destroyed the bindings or freed the gmem_file, and can't do so
 	 * until the caller drops slots_lock.
 	 */
-	if (!file) {
-		__kvm_gmem_unbind(slot, slot->gmem.file->private_data);
-		return;
-	}

Quoted 71 lines · Shivank Garg

kvm_gmem_unbind() skips mapping->invalidate_lock when the guest_memfd file is already dying. All other paths that modify f->bindings hold that lock.

kvm_gmem_invalidate_{start,end}() checks f->bindings independently to decide whether to begin or end KVM MMU invalidations. So, the bindings must remain stable between the two calls. If a binding is removed in that window, start increments mmu_invalidate_in_progress but end does not decrement it. Example, unbind race with memory failure:

CPU 0: memory failure CPU 1: memslot delete
---------------------------------- ---------------------------
(guest_memfd file is dying)
kvm_gmem_error_folio()
kvm_gmem_invalidate_start()
finds binding
mmu_invalidate_in_progress++
kvm_gmem_unbind()
get_file_active() fails store NULL in bindings
kvm_gmem_invalidate_end()
no binding found
counter stays elevated

mmu_invalidate_retry() then returns 1 forever, so guest page faults retry without ever installing a mapping and the guest hangs.

Take the invalidate lock in the dying-file path too. This prevents unbind from removing a binding and leaking mmu_invalidate_in_progress. This is safe because any caller that reaches this path holds slots_lock, so kvm_gmem_release() cannot nullify the slot->gmem.file, until kvm_gmem_unbind() finishes.

Reported-by Sashiko <[email protected]>

Closes https://lore.kernel.org/all/[email protected] open here

Fixes ae431059e75d ("KVM: guest_memfd: Remove bindings on memslot deletion when gmem is dying")

Signed-off-by Shivank Garg <[email protected]>

---
 virt/kvm/guest_memfd.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

virt/kvm/guest_memfd.c +4 -6

index f0e5da490866..f848120af84b 100644
@@ -721,6 +721,8 @@ static void __kvm_gmem_unbind(struct kvm_memory_slot *slot, struct gmem_file *f)
 
 void kvm_gmem_unbind(struct kvm_memory_slot *slot)
 {
+	struct file *gmem_file;
+
 	/*
 	 * Nothing to do if the underlying file was _already_ closed, as
 	 * kvm_gmem_release() invalidates and nullifies all bindings.

@@ -733,21 +735,24 @@ void kvm_gmem_unbind(struct kvm_memory_slot *slot)
 	/*
 	 * However, if the file is _being_ closed, then the bindings need to be
 	 * removed as kvm_gmem_release() might not run until after the memslot
-	 * is freed.  Note, modifying the bindings is safe even though the file
-	 * is dying as kvm_gmem_release() nullifies slot->gmem.file under
+	 * is freed.  Note, dereferencing the dying file is safe as
+	 * kvm_gmem_release() nullifies slot->gmem.file under
 	 * slots_lock, and only puts its reference to KVM after destroying all
 	 * bindings.  I.e. reaching this point means kvm_gmem_release() hasn't
 	 * yet destroyed the bindings or freed the gmem_file, and can't do so
 	 * until the caller drops slots_lock.
 	 */
-	if (!file) {
-		__kvm_gmem_unbind(slot, slot->gmem.file->private_data);
-		return;
-	}

virt/kvm/guest_memfd.c void kvm_gmem_unbind(struct kvm_memory_slot *slot) · line 751

Quoted 10 lines

virt/kvm/guest_memfd.c +6 -3

+	gmem_file = file ?: slot->gmem.file;
 
-	filemap_invalidate_lock(file->f_mapping);
-	__kvm_gmem_unbind(slot, file->private_data);
-	filemap_invalidate_unlock(file->f_mapping);
+	/*
+	 * Take the invalidate lock even for a dying file.  Otherwise,
+	 * kvm_gmem_invalidate_start() can find the binding and increment
+	 * mmu_invalidate_in_progress while kvm_gmem_invalidate_end() misses
+	 * the removed binding and skips decrement.

Hmm, so as called out in commit ae431059e75d ("KVM: guest_memfd: Remove bindings on memslot deletion when gmem is dying"), this assumes that file->f_mapping and everything "underneath" remains valid for dying files, which makes me a bit uncomfortable.

Deliberately don't acquire filemap invalid lock when the file is dying as the lifecycle of f_mapping is outside the purview of KVM. Dereferencing the mapping is *probably* fine, but there's no need to invalidate anything as memslot deletion is responsible for zapping SPTEs, and the only code that can access the dying file is kvm_gmem_release(), whose core code is mutually exclusive with unbinding.

Oh, but kvm_gmem_release() takes the same filemap_invalidate_unlock() and holding slots_lock guarantees that this code would run before release() if it sees a non-null slot->gmem.file, i.e. past me's concern is completely unfounded.

Rather than make this seem like something special, IMO we should treat this as a more normal thing. kvm->slots_lock is already load bearing, might as well double down on that. I.e. the exceptional part is doing all the work even though the file is dying, but the flows themselves should be identical.

E.g.

virt/kvm/guest_memfd.c +11 -25

index b596486d184c..5cc043466c89 100644
@@ -668,48 +668,40 @@ int kvm_gmem_bind(struct kvm *kvm, struct kvm_memory_slot *slot,
 	return r;
 }
 
-static void __kvm_gmem_unbind(struct kvm_memory_slot *slot, struct gmem_file *f)
+void kvm_gmem_unbind(struct kvm_memory_slot *slot)
 {
+	struct file *file = slot->gmem.file;
 	unsigned long start = slot->gmem.pgoff;
 	unsigned long end = start + slot->npages;
+	struct gmem_file *f;
 
-	xa_store_range(&f->bindings, start, end - 1, NULL, GFP_KERNEL);
-
-	/*
-	 * synchronize_srcu(&kvm->srcu) ensured that kvm_gmem_get_pfn()
-	 * cannot see this memslot.
-	 */
-	WRITE_ONCE(slot->gmem.file, NULL);
-}
-
-void kvm_gmem_unbind(struct kvm_memory_slot *slot)
-{
 	/*
 	 * Nothing to do if the underlying file was _already_ closed, as
 	 * kvm_gmem_release() invalidates and nullifies all bindings.
 	 */
-	if (!slot->gmem.file)
+	if (!file)
 		return;
 
-	CLASS(gmem_get_file, file)(slot);
-
 	/*
 	 * However, if the file is _being_ closed, then the bindings need to be
 	 * removed as kvm_gmem_release() might not run until after the memslot
-	 * is freed.  Note, modifying the bindings is safe even though the file
-	 * is dying as kvm_gmem_release() nullifies slot->gmem.file under
-	 * slots_lock, and only puts its reference to KVM after destroying all
-	 * bindings.  I.e. reaching this point means kvm_gmem_release() hasn't
-	 * yet destroyed the bindings or freed the gmem_file, and can't do so
-	 * until the caller drops slots_lock.
+	 * is freed.  Modifying the bindings is safe even if the file is dying
+	 * as kvm_gmem_release() nullifies slot->gmem.file under slots_lock,
+	 * and only puts its reference to KVM after destroying all bindings.
+	 * I.e. reaching this point means kvm_gmem_release() hasn't destroyed
+	 * the bindings or freed the gmem_file and can't do so until the caller
+	 * drops slots_lock, so there's no need to verify the file is live.
 	 */
-	if (!file) {
-		__kvm_gmem_unbind(slot, slot->gmem.file->private_data);
-		return;
-	}
+	f = file->private_data;
 
 	filemap_invalidate_lock(file->f_mapping);

The xAPIC IPI test uses MPOL_F_MEMS_ALLOWED to get the memory nodes available to the current process. Move the query to numaif.h as get_numa_mem_nodes() so other KVM selftests can use it.

Call get_mempolicy() directly instead of using the assert-on-failure kvm_get_mempolicy() wrapper. The xAPIC test already asserts its two node requirement.

Signed-off-by Shivank Garg <[email protected]>

---
 tools/testing/selftests/kvm/include/numaif.h     | 12 ++++++++++++
 tools/testing/selftests/kvm/x86/xapic_ipi_test.c |  6 ++----
 2 files changed, 14 insertions(+), 4 deletions(-)

tools/testing/selftests/kvm/include/numaif.h +12 -0

index 29572a6d789c..55124e2330ab 100644
@@ -75,6 +75,18 @@ static bool is_numa_available(void)
 		(errno != ENOSYS && errno != EPERM);
 }
 
+static inline unsigned long get_numa_mem_nodes(void)
+{
+	unsigned long nodemask = 0;
+
+	/* Get set of first 64 numa nodes available */
+	if (get_mempolicy(NULL, &nodemask, BITS_PER_TYPE(nodemask), NULL,
+			  MPOL_F_MEMS_ALLOWED))
+		return 0;
+
+	return nodemask;
+}
+
 static inline bool is_multi_numa_node_system(void)
 {
 	return is_numa_available() && get_max_numa_node() >= 1;

tools/testing/selftests/kvm/x86/xapic_ipi_test.c +1 -1

index 769d8d95ab2c..66dcf36398aa 100644
@@ -251,7 +251,7 @@ void do_migrations(struct test_data_page *data, int run_secs, int delay_usecs,
 		   u64 *pipis_rcvd)
 {
 	long pages_not_moved;
-	unsigned long nodemask = 0;
+	unsigned long nodemask;
 	unsigned long nodemasks[BITS_PER_TYPE(nodemask)];
 	int nodes = 0;
 	time_t start_time, last_update, now;

Split out the guest_memfd folio migration series [1], where migration part needs more discussion. These fixes, reported by Sashiko bot [2][3], are independent of it and can be reviewed on their own.

[1] https://lore.kernel.org/all/[email protected]
[2] https://lore.kernel.org/all/[email protected]
[3] https://lore.kernel.org/all/[email protected]

Signed-off-by Shivank Garg <[email protected]>

---
Shivank Garg (5):
KVM: guest_memfd: take the invalidate lock when unbinding a dying file
KVM: selftests: fix maxnode arguments in xapic_ipi_test
KVM: selftests: use BITS_PER_TYPE() for NUMA masks
KVM: selftests: add get_numa_mem_nodes()
KVM: selftests: use allowed NUMA nodes in guest_memfd_test

 tools/testing/selftests/kvm/guest_memfd_test.c   | 86 ++++++++++++++++--------
 tools/testing/selftests/kvm/include/numaif.h     | 52 ++------------
 tools/testing/selftests/kvm/x86/xapic_ipi_test.c | 14 ++--
 virt/kvm/guest_memfd.c                           | 23 ++++---
 4 files changed, 84 insertions(+), 91 deletions(-)

---
base-commit: 2709dd5ae32f0828f386327c76bba9f39f63a1c6
change-id: 20260822-shivank-gmem-fix-split-b8a9057cf987

Best regards,

Quoted 26 lines · Shivank Garg

Split out the guest_memfd folio migration series [1], where migration part needs more discussion. These fixes, reported by Sashiko bot [2][3], are independent of it and can be reviewed on their own.

[1] https://lore.kernel.org/all/[email protected]
[2] https://lore.kernel.org/all/[email protected]
[3] https://lore.kernel.org/all/[email protected]

Signed-off-by Shivank Garg <[email protected]>

---
Shivank Garg (5):
KVM: guest_memfd: take the invalidate lock when unbinding a dying file
KVM: selftests: fix maxnode arguments in xapic_ipi_test
KVM: selftests: use BITS_PER_TYPE() for NUMA masks
KVM: selftests: add get_numa_mem_nodes()
KVM: selftests: use allowed NUMA nodes in guest_memfd_test

 tools/testing/selftests/kvm/guest_memfd_test.c   | 86 ++++++++++++++++--------
 tools/testing/selftests/kvm/include/numaif.h     | 52 ++------------
 tools/testing/selftests/kvm/x86/xapic_ipi_test.c | 14 ++--
 virt/kvm/guest_memfd.c                           | 23 ++++---
 4 files changed, 84 insertions(+), 91 deletions(-)

---
base-commit: 2709dd5ae32f0828f386327c76bba9f39f63a1c6
change-id: 20260822-shivank-gmem-fix-split-b8a9057cf987

Quoted 1 line

Best regards,

Hi Sean, Paolo,

Would you consider queuing this for next (7.4)?

Quoted 29 lines · Shivank Garg

Quoted 27 lines · Shivank Garg

Split out the guest_memfd folio migration series [1], where migration part needs more discussion. These fixes, reported by Sashiko bot [2][3], are independent of it and can be reviewed on their own.

[1] https://lore.kernel.org/all/[email protected]
[2] https://lore.kernel.org/all/[email protected]
[3] https://lore.kernel.org/all/[email protected]

Signed-off-by Shivank Garg <[email protected]>

---
Shivank Garg (5):
KVM: guest_memfd: take the invalidate lock when unbinding a dying file
KVM: selftests: fix maxnode arguments in xapic_ipi_test
KVM: selftests: use BITS_PER_TYPE() for NUMA masks
KVM: selftests: add get_numa_mem_nodes()
KVM: selftests: use allowed NUMA nodes in guest_memfd_test

 tools/testing/selftests/kvm/guest_memfd_test.c   | 86 ++++++++++++++++--------
 tools/testing/selftests/kvm/include/numaif.h     | 52 ++------------
 tools/testing/selftests/kvm/x86/xapic_ipi_test.c | 14 ++--
 virt/kvm/guest_memfd.c                           | 23 ++++---
 4 files changed, 84 insertions(+), 91 deletions(-)

---
base-commit: 2709dd5ae32f0828f386327c76bba9f39f63a1c6
change-id: 20260822-shivank-gmem-fix-split-b8a9057cf987

Best regards,

Quoted 3 lines

Hi Sean, Paolo,

Would you consider queuing this for next (7.4)?

No, I'm expecting a new version based on the discussion in patch 1.

I also don't understand why the selftests changes are bundled in with what appears to be a completely unrelated bug fix.