Re: [PATCH 16/45] KVM: adjust for presence of more than one attribute

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] KVM: premature true return in kvm_range_has_vm_memory_attributes()
--

commit bc4dd6682729ee9a313a549f79ade166886ce9b6
Author: Paolo Bonzini <pbonzini@redhat.com>

KVM: adjust for presence of more than one attribute

This commit removes private-memory WARN checks in x86 MMU code and adjusts
kvm_range_has_vm_memory_attributes() so it no longer assumes
KVM_MEMORY_ATTRIBUTE_PRIVATE is the only valid memory attribute.

Link: https://patch.msgid.link/20260918135030.171564-13-pbonzini@redhat.com

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 2fd595a901bd1..17341c689d11a 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -2490,10 +2490,12 @@ bool kvm_range_has_vm_memory_attributes(struct kvm *kvm, gfn_t start, gfn_t end,
>  					unsigned long mask, unsigned long attrs)
>  {
>  	XA_STATE(xas, &kvm->mem_attr_array, start);
> +	unsigned long supported_attrs;
>  	unsigned long index;
>  	void *entry;
>  
> -	mask &= kvm_supported_vm_mem_attributes(kvm);
> +	supported_attrs = kvm_supported_vm_mem_attributes(kvm);
> +	mask &= supported_attrs;
>  	if (attrs & ~mask)
>  		return false;
>  
> @@ -2522,8 +2524,10 @@ bool kvm_range_has_vm_memory_attributes(struct kvm *kvm, gfn_t start, gfn_t end,
>  	 */
>  	for (index = start; index < end; index++) {
>  		do {
> -			entry = attrs ? xas_next(&xas) :
> -					xas_next_entry(&xas, end - 1);
> +			if (!attrs && mask == supported_attrs)
> +				entry = xas_next_entry(&xas, end - 1);
> +			else
> +				entry = xas_next(&xas);
>  		} while (xas_retry(&xas, entry));
>  
>  		if (!entry)

[Severity: Medium]
When attrs == 0 and mask != supported_attrs, doesn't switching to
xas_next(&xas) cause kvm_range_has_vm_memory_attributes() to return true
prematurely on the first GFN that has no entry in kvm->mem_attr_array?

Unlike xas_next_entry(&xas, end - 1), which scans forward up to end - 1 for
the next present entry, xas_next(&xas) only inspects the single slot at the
current index and returns NULL as soon as that single GFN has no attributes.

Immediately after the loop in kvm_range_has_vm_memory_attributes():

		if (!entry)
			return !attrs;

		WARN_ON_ONCE(!xa_to_value(entry));

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

Because !entry returns !attrs (which is true when attrs == 0), encountering
any empty slot in [start, end) immediately returns true without checking the
remaining GFNs in [index + 1, end).

Additionally, if xas_next(&xas) encounters a reserved XA_ZERO_ENTRY while
attrs == 0, xas_retry(&xas, entry) returns true without resetting xas, so
the next xas_next(&xas) call in the do-while loop advances xas.xa_index to
index + 1 and can fail the xas.xa_index != index check.

Since empty and XA_ZERO_ENTRY slots have effective attributes of 0 and
always satisfy (0 & mask) == 0 for any mask, should this still use
xas_next_entry(&xas, end - 1) whenever !attrs, and instead only check
xas.xa_index != index when attrs != 0 (or advance index to xas.xa_index)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102174130.8779-1-pbonzini@redhat.com?part=16