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

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

[Zeng Chi]

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

Release the successfully reserved entries on failure.  xa_release() is
a nop for entries that hold a real value, so it's safe to blindly
release all entries in [start, i).

Fixes: 5a475554db1e ("KVM: Introduce per-page memory attributes")
Signed-off-by: Zeng Chi <zengchi@kylinos.cn>
---
 virt/kvm/kvm_main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 65eb26a0520d..20031a832c21 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2575,8 +2575,11 @@ static int kvm_vm_set_mem_attributes(struct kvm *kvm, gfn_t start, gfn_t end,
 	 */
 	for (i = start; i < end; i++) {
 		r = xa_reserve(&kvm->mem_attr_array, i, GFP_KERNEL_ACCOUNT);
-		if (r)
+		if (r) {
+			while (i-- > start)
+				xa_release(&kvm->mem_attr_array, i);
 			goto out_unlock;
+		}
 
 		cond_resched();
 	}
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The error cleanup loop in `kvm_vm_set_mem_attributes` lacks a `cond_resched()` call, which can cause CPU soft lockups when reverting a massive memory range.
--

commit 628b8d791b455e8fdc196cf53b9e1f82e2d7fdb6
Author: Zeng Chi <zengchi@kylinos.cn>

KVM: Release reserved xarray entries if reserving memory attributes fails

This commit fixes a memory leak in kvm_vm_set_mem_attributes() where previously
reserved xarray entries were left behind if a subsequent reservation failed. It
adds a backward loop to release the successfully reserved entries before bailing
out on the error path.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2575,8 +2575,11 @@ static int kvm_vm_set_mem_attributes(struct kvm *kvm, gfn_t start, gfn_t end,
>  	 */
>  	for (i = start; i < end; i++) {
>  		r = xa_reserve(&kvm->mem_attr_array, i, GFP_KERNEL_ACCOUNT);
> -		if (r)
> +		if (r) {
> +			while (i-- > start)
> +				xa_release(&kvm->mem_attr_array, i);

[Severity: High]
Can this backward cleanup loop cause CPU soft lockups if a massive memory
range was requested?

The forward loop correctly yields the CPU via cond_resched() to handle large
ranges, but the unwinding error path does not. 

If userspace invokes KVM_SET_MEMORY_ATTRIBUTES on a massive range and
xa_reserve() eventually fails, this loop will continuously process potentially
millions of entries. On kernels with CONFIG_PREEMPT_VOLUNTARY or
CONFIG_PREEMPT_NONE, could this block scheduling long enough to trigger the
soft lockup watchdog?

>  			goto out_unlock;
> +		}
>  
>  		cond_resched();
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827110558.457891-1-zeng_chi911@163.com?part=1

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

[Sean Christopherson]

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;

[PATCH v2] KVM: Don't treat reserved xarray entries as having memory attributes

[Zeng Chi]

From: Zeng Chi <zengchi@kylinos.cn>

kvm_vm_set_mem_attributes() reserves an xarray entry for every gfn in
the range before storing the new attributes, so that the store loop
can't fail partway through.  If one of the reservations fails, e.g. with
-ENOMEM, the entries that were already reserved are left in the array.
That is harmless as far as xa_reserve() is concerned, as the reserved
entries read back as NULL via xa_load(), but it confuses the "does this
range have no attributes at all" check:

	if (!attrs)
		return !xas_find(&xas, end - 1);

A reserved entry is XA_ZERO_ENTRY, not NULL, and xas_find() returns it
as present.  So a leftover reservation makes KVM report that a fully
shared range has attributes even though kvm_get_memory_attributes()
returns none for every gfn in the range.  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 mixed and
refuses to map it with a hugepage, until userspace happens to set
attributes on the range again.

Walk the range and ignore reserved-but-unset entries when checking for
the absence of attributes, so a leftover reservation is treated the same
as an empty slot.  Note, the generic loop for the attrs != 0 case
already skips zero entries via xas_retry(), i.e. only the !attrs shortcut
was affected.

While at it, skip the reservation loop entirely when clearing attributes,
as storing NULL only erases the entry and never needs to allocate, so no
reservation (and no cleanup of a failed one) is required in that case.

Fixes: 5a475554db1e ("KVM: Introduce per-page memory attributes")
Signed-off-by: Zeng Chi <zengchi@kylinos.cn>
---
 virt/kvm/kvm_main.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 65eb26a0520d..29534bcc7f02 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2447,8 +2447,19 @@ 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) {
+		/*
+		 * Reserved but unset entries (XA_ZERO_ENTRY, e.g. left behind by
+		 * a failed reservation in kvm_vm_set_mem_attributes()) are
+		 * returned as present by xas_find(), but hold no attributes.
+		 * Skip them so that the range is correctly reported as having no
+		 * attributes.
+		 */
+		xas_for_each(&xas, entry, end - 1)
+			if (!xa_is_zero(entry))
+				return false;
+		return true;
+	}
 
 	for (index = start; index < end; index++) {
 		do {
@@ -2571,9 +2582,11 @@ 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.
+	 * partway through setting the new attributes.  Clearing attributes
+	 * only stores NULL, which never needs to allocate, so skip the
+	 * reservations entirely in that case.
 	 */
-	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;
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

Re: [PATCH v2] KVM: Don't treat reserved xarray entries as having memory attributes

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing XArray retry handling causes spurious memory attribute presence reporting
--

commit 4066b9047c8a4867ec41a5a11e687a69b4773334
Author: Zeng Chi <zengchi@kylinos.cn>

KVM: Don't treat reserved xarray entries as having memory attributes

This commit fixes a bug in KVM where failed xarray reservations leave zero
entries in the attributes array, causing fully shared memory ranges to be
incorrectly reported as having mixed attributes. It modifies the attribute
check to explicitly skip zero entries when checking if a range is empty.

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 65eb26a0520d8..29534bcc7f020 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
[ ... ]
> @@ -2447,8 +2447,19 @@ 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) {
> +		/*
> +		 * Reserved but unset entries (XA_ZERO_ENTRY, e.g. left behind by
> +		 * a failed reservation in kvm_vm_set_mem_attributes()) are
> +		 * returned as present by xas_find(), but hold no attributes.
> +		 * Skip them so that the range is correctly reported as having no
> +		 * attributes.
> +		 */
> +		xas_for_each(&xas, entry, end - 1)
> +			if (!xa_is_zero(entry))
> +				return false;

[Severity: Medium]
Does this lockless loop need to handle XArray retry entries?

If kvm_vm_set_mem_attributes() modifies the array concurrently (e.g., using
xa_reserve() or xa_store()), the XArray nodes might split or shrink,
temporarily exposing an XA_RETRY_ENTRY to readers.

While !xa_is_zero(entry) correctly evaluates to false for XA_ZERO_ENTRY, it
evaluates to true if the entry is an internal XA_RETRY_ENTRY. This causes
the loop to treat a retry entry as a non-zero attribute entry and
prematurely return false.

Could this cause KVM to incorrectly report a memory range as having
attributes when it actually has none? For instance, on x86, this might force
callers like hugepage_has_attrs() to permanently mark the range as mixed,
disabling hugepage mappings for the affected GFNs until their attributes are
explicitly modified again.

Should we explicitly restart the traversal when encountering retry entries?

		if (xas_retry(&xas, entry))
			continue;

> +		return true;
> +	}
>  
>  	for (index = start; index < end; index++) {
>  		do {
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828102728.1308266-1-zeng_chi911@163.com?part=1

Re: [PATCH v2] KVM: Don't treat reserved xarray entries as having memory attributes

[Sean Christopherson]

Please don't send a new version of a patch/series while there is active discussion
on the previous version.  As is the case here, there's often not enough context
in the new, standalone patch to carry on the discussion.  And even when there is
enough context, it's annoying to have to read one thread, and then skip over to
a different thread to respond.

On Fri, Aug 28, 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 storing the new attributes, so that the store loop
> can't fail partway through.  If one of the reservations fails, e.g. with
> -ENOMEM, the entries that were already reserved are left in the array.
> That is harmless as far as xa_reserve() is concerned, as the reserved
> entries read back as NULL via xa_load(), but it confuses the "does this
> range have no attributes at all" check:
> 
> 	if (!attrs)
> 		return !xas_find(&xas, end - 1);
> 
> A reserved entry is XA_ZERO_ENTRY, not NULL, and xas_find() returns it
> as present.  So a leftover reservation makes KVM report that a fully
> shared range has attributes even though kvm_get_memory_attributes()
> returns none for every gfn in the range.  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 mixed and
> refuses to map it with a hugepage, until userspace happens to set
> attributes on the range again.
> 
> Walk the range and ignore reserved-but-unset entries when checking for
> the absence of attributes, so a leftover reservation is treated the same
> as an empty slot.  Note, the generic loop for the attrs != 0 case
> already skips zero entries via xas_retry(), i.e. only the !attrs shortcut
> was affected.
> 
> While at it, skip the reservation loop entirely when clearing attributes,
> as storing NULL only erases the entry and never needs to allocate, so no
> reservation (and no cleanup of a failed one) is required in that case.
> 
> Fixes: 5a475554db1e ("KVM: Introduce per-page memory attributes")
> Signed-off-by: Zeng Chi <zengchi@kylinos.cn>
> ---
>  virt/kvm/kvm_main.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 65eb26a0520d..29534bcc7f02 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2447,8 +2447,19 @@ 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) {
> +		/*
> +		 * Reserved but unset entries (XA_ZERO_ENTRY, e.g. left behind by
> +		 * a failed reservation in kvm_vm_set_mem_attributes()) are
> +		 * returned as present by xas_find(), but hold no attributes.
> +		 * Skip them so that the range is correctly reported as having no
> +		 * attributes.
> +		 */
> +		xas_for_each(&xas, entry, end - 1)

Curly braces needed for the outer loop.  And +1 to Sashiko's feedback, both from
a correctness perspective and from a "make boths paths look similar" perspective.

Though even better, we can use the same core logic.  Pulling in your response from
v1:

 : > 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 {
 : >
 : I tried that first, but it doesn't fix the false positive.  Falling through to
 : the generic loop for the !attrs case still returns false for a range that only
 : contains reserved (zero) entries: the loop does
 : 
 :         do {
 :                 entry = xas_next(&xas);
 :         } while (xas_retry(&xas, entry));

The other subtle wrinkle is that the xarray APIs reset the index when no entry is
found (this wasted a good 30 minutes of my time, argh).  I.e. when on entry is
found, then KVM *must not* check the index, because it is effectively invalid.
E.g. I initially wanted to check for xas.xa_index >= end, but that doesn't work.

This code also needs comments, because the xarray APIs have all kinds of sharp
edges (or maybe a better way of looking at things, xarray isn't a great fit for
what KVM is doing here).

Yeesh, speaking of which, simply using xas_next_entry(), as I want to do, would
be slightly suboptimal for non-zero attributes, because xas_next() (confusingly,
IMO) doesn't return the next non-NULL entry, it returns literally the next entry,
whereas xas_next_entry() returns the next non-NULL entry, bounded by the max. I
don't actually care about the performance impact, but I want to document the
behavior, at which point it's just as easy to use next() vs. next_entry().

So after way, waaay too much fiddling, this?  As a bonus, the changelog can call
out that xas_next_entry() is essentially an optimized version of xas_find(),
e.g. to communicate that the effective diff is actually just adding xas_retry().

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 65eb26a0520d..cc94d9881582 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2447,14 +2447,39 @@ 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);
 
+	/*
+	 * Lookup the entry for each index instead of iterating over the xarray
+	 * as KVM deletes/nullifies entries to represent "no attributes", and
+	 * the xas index is effectively invalid when no entry is found.  I.e.
+	 * matching non-zero attributes for *every* entry effectively requires
+	 * a manually lookup for each index.
+	 *
+	 * Skip pre-allocated, reserved entries, or restart the lookup if the
+	 * xarray was concurrently modified, via xas_retry() ("retry" means the
+	 * entry holds an internal xarray value, i.e. is either invalid or NULL
+	 * from the caller's perspective.
+	 *
+	 * Use xas_next() when looking for non-zero attributes to optimize for
+	 * the case where the start of the range (or the entire range) doesn't
+	 * have any attributes, as xas_next() returns literally the next entry,
+	 * whereas xas_next_entry() returns the next non-NULL entry (bounded by
+	 * a maximum index).
+	 */
 	for (index = start; index < end; index++) {
 		do {
-			entry = xas_next(&xas);
+			entry = attrs ? xas_next(&xas) :
+					xas_next_entry(&xas, end - 1);
 		} while (xas_retry(&xas, entry));
 
+		/*
+		 * Don't check the index if there's no entry; as above, the xas
+		 * index is invalid (and if no entry was found, then the entire
+		 * range has no attributes).
+		 */
+		if (!entry)
+			return !attrs;
+
 		if (xas.xa_index != index ||
 		    (xa_to_value(entry) & mask) != attrs)
 			return false;

Re: [PATCH v2] KVM: Don't treat reserved xarray entries as having memory attributes

[Sean Christopherson]

On Fri, Aug 28, 2026, Sean Christopherson wrote:
> So after way, waaay too much fiddling, this?  As a bonus, the changelog can call
> out that xas_next_entry() is essentially an optimized version of xas_find(),
> e.g. to communicate that the effective diff is actually just adding xas_retry().
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 65eb26a0520d..cc94d9881582 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2447,14 +2447,39 @@ 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);
>  
> +	/*
> +	 * Lookup the entry for each index instead of iterating over the xarray
> +	 * as KVM deletes/nullifies entries to represent "no attributes", and
> +	 * the xas index is effectively invalid when no entry is found.  I.e.
> +	 * matching non-zero attributes for *every* entry effectively requires
> +	 * a manually lookup for each index.
> +	 *
> +	 * Skip pre-allocated, reserved entries, or restart the lookup if the
> +	 * xarray was concurrently modified, via xas_retry() ("retry" means the
> +	 * entry holds an internal xarray value, i.e. is either invalid or NULL
> +	 * from the caller's perspective.
> +	 *
> +	 * Use xas_next() when looking for non-zero attributes to optimize for
> +	 * the case where the start of the range (or the entire range) doesn't
> +	 * have any attributes, as xas_next() returns literally the next entry,
> +	 * whereas xas_next_entry() returns the next non-NULL entry (bounded by
> +	 * a maximum index).
> +	 */
>  	for (index = start; index < end; index++) {
>  		do {
> -			entry = xas_next(&xas);
> +			entry = attrs ? xas_next(&xas) :
> +					xas_next_entry(&xas, end - 1);
>  		} while (xas_retry(&xas, entry));
>  
> +		/*
> +		 * Don't check the index if there's no entry; as above, the xas
> +		 * index is invalid (and if no entry was found, then the entire
> +		 * range has no attributes).
> +		 */
> +		if (!entry)
> +			return !attrs;

One "flaw" with this exact code is that if KVM managed to get a non-null, '0'
entry into the xarray, the index check could mismatch and this function could
technically get a false negative.

Swapping the checks would also work:

		if (!attrs)
			return !entry;

but I don't love that that violates the "don't check the index because it's bogus"
statement above.  And practically speaking, KVM should *never* observe a non-NULL
entry with a value of zero, assuming xas_retry() works as I think it does.  So to
harden against KVM changes/goofs, maybe do this as well?

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index cc94d9881582..f009cb3e687d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -2480,6 +2480,8 @@ bool kvm_range_has_memory_attributes(struct kvm *kvm, gfn_t start, gfn_t end,
 		if (!entry)
 			return !attrs;
 
+		WARN_ON_ONCE(!xa_to_value(entry));
+
 		if (xas.xa_index != index ||
 		    (xa_to_value(entry) & mask) != attrs)
 			return false;


> +
>  		if (xas.xa_index != index ||
>  		    (xa_to_value(entry) & mask) != attrs)
>  			return false;
> 

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

[Zeng Chi]

On 2026/8/28 02:37, Sean Christopherson wrote:
> 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
On 2026/8/28 02:37, Sean Christopherson wrote:
> 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.
> 
Agreed, fixing the reader is better.  Reframed that way in v2.

> 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 {
> 
I tried that first, but it doesn't fix the false positive.  Falling through to
the generic loop for the !attrs case still returns false for a range that only
contains reserved (zero) entries: the loop does

	do {
		entry = xas_next(&xas);
	} while (xas_retry(&xas, entry));

and xas_retry() returns true for zero entries (xa_is_zero()), so it skips the
reserved entries, then "xas.xa_index != index" trips and the function returns
false.  So the leaked-reservation range is still reported as having attributes.

I confirmed it with the tools/testing/radix-tree harness (reserve [0, 512),
then query attrs == 0): the original code, the sketch above, and the sketch
with the xas cursor reset all return false, where absent is expected.

What does work is to walk the range and ignore the zero entries explicitly:

	guard(rcu)();
	if (!attrs) {
		xas_for_each(&xas, entry, end - 1)
			if (!xa_is_zero(entry))
				return false;
		return true;
	}

That gives the right answer for {empty, only reservations, a real value present,
reservations + a real value, all set}.

> 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;
Right, storing NULL just erases and never allocates, so no reservation is
needed when clearing.  I folded that into the same patch (the reservation loop
becomes "for (i = start; entry && i < end; i++)").


Thanks,
Zeng Chi