Re: [PATCH] KVM: Release reserved xarray entries if reserving memory attributes fails

[Sean Christopherson <seanjc@google.com>]

On Thu, Aug 27, 2026, Zeng Chi wrote:
> From: Zeng Chi <zengchi@kylinos.cn>
> 
> kvm_vm_set_mem_attributes() reserves an xarray entry for every gfn in
> the range before modifying any attributes, so that the actual updates
> can't fail partway through.  But if one of the reservations fails, e.g.
> due to -ENOMEM, the entries that were already reserved are left behind,
> as the error path bails without releasing them.
> 
> A reserved entry is XA_ZERO_ENTRY, not NULL.

Lovely.

> xa_load() hides the difference, but kvm_range_has_memory_attributes() uses
> xas_find() to check whether a range has no attributes at all, and xas_find()
> returns zero entries as-is.  As a result, a leaked reservation makes KVM
> think the range has attributes set even though kvm_get_memory_attributes()
> reports none.  On x86, the next time mixed-attribute tracking is recomputed
> for the range (memslot creation, or a later attribute change that straddles
> the 2MiB page), hugepage_has_attrs() treats a fully shared 2MiB range as
> having mixed attributes and refuses to map it with a hugepage, until
> userspace happens to set attributes on the range again.

I'm inclined to fix kvm_range_has_memory_attributes() instead of unwinding the
reservation.  Because this isn't a memory leak per se, e.g. if it weren't for
the false negative in kvm_range_has_memory_attributes(), I would say this is a
complete non-issue (there's no leak, just a maybe-unused reservation).
working as intended.

I think it would be this?

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 65eb26a0520d..a01b2af1cb17 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2447,8 +2447,9 @@ bool kvm_range_has_memory_attributes(struct kvm *kvm, gfn_t start, gfn_t end,
 		return (kvm_get_memory_attributes(kvm, start) & mask) == attrs;
 
 	guard(rcu)();
-	if (!attrs)
-		return !xas_find(&xas, end - 1);
+
+	if (!attrs && !xas_find(&xas, end - 1))
+		return true;
 
 	for (index = start; index < end; index++) {
 		do {

Side topic, does storing NULL even require an entry?  Based on the above behavior,
I assume not.  So can't we also do?  This feels like deja vu though...

@@ -2573,7 +2574,7 @@ static int kvm_vm_set_mem_attributes(struct kvm *kvm, gfn_t start, gfn_t end,
 	 * Reserve memory ahead of time to avoid having to deal with failures
 	 * partway through setting the new attributes.
 	 */
-	for (i = start; i < end; i++) {
+	for (i = start; entry && i < end; i++) {
 		r = xa_reserve(&kvm->mem_attr_array, i, GFP_KERNEL_ACCOUNT);
 		if (r)
 			goto out_unlock;