Re: [PATCH 14/24] KVM: Introduce a generation number for memory attributes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] xas_next() causes O(N) RCU stalls in kvm_range_has_any_memory_attributes()
- [High] Unconditional increment of kvm->mem_attrs.generation causes global cache invalidation
- [Medium] Data race and potential torn read of kvm->mem_attrs.generation
- [Low] Access to mem_attrs outside of #ifdef CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES breaks compilation
--

--- Patch [14]: [PATCH 14/24] KVM: Introduce a generation number for memory attributes ---
commit 1b3b4442db618c511fecddd4a021c24933baeaa1
Author: Nicolas Saenz Julienne <nsaenz@amazon.com>
Subject: KVM: Introduce a generation number for memory attributes

Introduce a generation number to track memory attribute modifications.
This will allow KVM components to invalidate any assumptions they might
have about guest's physical addresses and their permissions when memory
attributes change.

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 122af87d6f9a5..1fad3eb303c17 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h

[ ... ]

> @@ -2559,29 +2560,49 @@ static inline bool kvm_mem_attributes_may_exec(u64 attrs)
>  	return !(attrs & KVM_MEMORY_ATTRIBUTE_NX);
>  }
>  
> -static inline bool kvm_memslots_check_gen(struct kvm *kvm, u64 slots_generation, struct kvm_memslots **p_slots)
> +static inline bool kvm_memslots_check_gen(struct kvm *kvm, u64 slots_generation,
> +					  u64 attrs_generation, struct kvm_memslots **p_slots)
>  {
>  	struct kvm_memslots *slots = *p_slots = kvm_memslots(kvm);
> -	return slots->generation == slots_generation;
> +	return slots->generation == slots_generation && kvm->mem_attrs.generation == attrs_generation;

[Severity: Low]
Does this break compilation when CONFIG_KVM_GENERIC_MEMORY_ATTRIBUTES is
disabled? The mem_attrs field in struct kvm is guarded by this config,
but it is accessed unconditionally here. Should this use the
kvm_mem_attributes_generation() helper instead?

[Severity: Medium]
Is there a data race on kvm->mem_attrs.generation here?

kvm_vm_set_mem_attributes() updates the generation counter using
smp_store_release(). Since kvm_memslots_check_gen() reads it concurrently
in lockless fast-paths, a plain C read could lead to torn reads on
some architectures. Would it be safer to use READ_ONCE() or the
kvm_mem_attributes_generation() helper?

>  }

[ ... ]

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 1afaec56ecb37..2fe4087319adf 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c

[ ... ]

> +bool kvm_range_has_any_memory_attributes(struct kvm *kvm, gfn_t start, gfn_t end,
> +					unsigned long mask)
> +{
> +	XA_STATE(xas, &kvm->mem_attrs.array, start);
> +	void *entry;
> +
> +	mask &= kvm_supported_mem_attributes(kvm);
> +	if (!mask)
> +		return false;
> +
> +	if (end == start + 1)
> +		return !!(kvm_get_memory_attributes(kvm, start) & mask);
> +
> +	guard(rcu)();
> +	for (;;) {
> +		do {
> +			entry = xas_next(&xas);
> +		} while (xas_retry(&xas, entry));
> +
> +		if (xas.xa_index >= end)
> +			break;
> +
> +		if (xa_to_value(entry) & mask)
> +			return true;
> +	}
> +
> +	return false;
> +}

[Severity: High]
Can this loop cause CPU soft lockups and RCU stalls when querying a large,
sparse memory range?

If the requested range is empty, xas_next() will return NULL and increment
the internal array index by 1 step-by-step. For millions of pages, this
will iterate millions of times under guard(rcu)().

Should this use xas_find() or xas_for_each() to skip empty entries in O(1)
time instead?

[ ... ]

> @@ -2594,10 +2645,16 @@ static int kvm_vm_set_mem_attributes(struct kvm *kvm, gfn_t start, gfn_t end,
>  		cond_resched();
>  	}
>  
> +	/* Pairs with acquire in kvm_mem_attributes_generation() */
> +	smp_store_release(&kvm->mem_attrs.generation,
> +			  kvm->mem_attrs.generation + 1);

[Severity: High]
The commit message states: "There shouldn't be any performance loss for
memory attributes that don't require synchronization ... because the
attributes generation will always remain unchanged."

However, kvm->mem_attrs.generation is unconditionally incremented here,
even if the sync flag is false. Will this global increment invalidate all
gfn_to_hva_cache and gfn_to_pfn_cache entries across the entire VM
unnecessarily? Should this be conditional on the sync flag being true?

> +
>  	kvm_handle_gfn_range(kvm, &post_set_range);
>  
>  out_unlock:

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