Re: [PATCH 06/24] KVM: x86: Avoid warning when installing non-private memory attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The defensive sanitization of memory attributes is ineffective, containing a dead store and failing to prevent inconsistent xarray state.
--

--- Patch [6]: [PATCH 06/24] KVM: x86: Avoid warning when installing non-private memory attributes ---
commit 08f754e61d84422c72539df0f4a05fb7d05a02f5
Author: Nicolas Saenz Julienne <nsaenz@amazon.com>

KVM: x86: Avoid warning when installing non-private memory attributes

This commit adds a check to warn and clear KVM_MEMORY_ATTRIBUTE_PRIVATE in
both kvm_arch_pre_set_memory_attributes() and
kvm_arch_post_set_memory_attributes() when private memory is not supported.
It replaces the unconditional WARN_ON_ONCE(!kvm_arch_has_private_mem(kvm))
checks that were previously throwing warnings.

> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 424f4e1136822..055e0b45a8ee9 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -8078,9 +8078,15 @@ static void hugepage_set_mixed(struct kvm_memory_slot *slot, gfn_t gfn,
>  bool kvm_arch_pre_set_memory_attributes(struct kvm *kvm,
>  					struct kvm_gfn_range *range)
>  {
> +	unsigned long attrs = range->arg.attributes;
>  	struct kvm_memory_slot *slot = range->slot;
>  	int level;
>  
> +	if (!kvm_arch_has_private_mem(kvm)) {
> +		WARN_ON(attrs & KVM_MEMORY_ATTRIBUTE_PRIVATE);
> +		attrs &= ~KVM_MEMORY_ATTRIBUTE_PRIVATE;
> +	}
> +

[Severity: Medium]
Is this attrs update effective, or does it result in a dead store?

Looking further down in kvm_arch_pre_set_memory_attributes(), the unmap
filter logic evaluates the unmodified range->arg.attributes instead of using
this newly sanitized attrs variable:

	/* Unmap the old attribute page. */
	if (range->arg.attributes & KVM_MEMORY_ATTRIBUTE_PRIVATE)
		range->attr_filter = KVM_FILTER_SHARED;
	else
		range->attr_filter = KVM_FILTER_PRIVATE;

Does this mean the invalid attributes are still processed if they manage to
reach this path?

[ ... ]

> @@ -8165,16 +8168,17 @@ bool kvm_arch_post_set_memory_attributes(struct kvm *kvm,
>  	lockdep_assert_held_write(&kvm->mmu_lock);
>  	lockdep_assert_held(&kvm->slots_lock);
>  
> +	if (!kvm_arch_has_private_mem(kvm)) {
> +		WARN_ON(attrs & KVM_MEMORY_ATTRIBUTE_PRIVATE);
> +		attrs &= ~KVM_MEMORY_ATTRIBUTE_PRIVATE;
> +	}
> +

[Severity: Medium]
Could sanitizing attrs locally here lead to inconsistent state evaluations
later on?

In virt/kvm/kvm_main.c:kvm_vm_set_mem_attributes(), the unmodified attributes
argument is unconditionally stored into the global xarray before the
arch-specific callbacks run:

	r = xa_err(xa_store(&kvm->mem_attr_array, i, entry,
			    GFP_KERNEL_ACCOUNT));

If kvm_arch_post_set_memory_attributes() passes this locally sanitized attrs
variable into hugepage_has_attrs(), will it erroneously compare the sanitized
value against the unsanitized values stored in the xarray?

>  	/*
>  	 * Calculate which ranges can be mapped with hugepages even if the slot
>  	 * can't map memory PRIVATE.  KVM mustn't create a SHARED hugepage over
>  	 * a range that has PRIVATE GFNs, and conversely converting a range to
>  	 * SHARED may now allow hugepages.
> -	 */
> -	if (WARN_ON_ONCE(!kvm_arch_has_private_mem(kvm)))
> -		return false;
> -
> -	/*
>  	 * The sequence matters here: upper levels consume the result of lower
>  	 * level's scanning.
>  	 */

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