Re: [PATCH 24/45] 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] KVM: RCU stall in kvm_range_has_any_vm_memory_attributes() via xas_next()
- [Medium] KVM: unconditional generation bump and error-path sync in kvm_set_vm_mem_attributes()
- [Low] KVM: loongarch: missing closing parenthesis in kvm_check_gen() conditions
- [Low] KVM: stale attrs_generation on error in __kvm_gfn_to_hva_cache_init()
--

commit c500cbe0be12ea140c87b086c8905238fbefe1ea
Author: Nicolas Saenz Julienne <nsaenz@amazon.com>

KVM: Introduce a generation number for memory attributes

This commit introduces a generation number for VM memory attributes so that
KVM components using gfn_to_hva_cache and gfn_to_pfn_cache can detect memory
attribute updates and synchronize via KVM SRCU when attributes requiring
synchronization are modified.

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

> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index c3e71ef983bcb..6301e726a536f 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c
> @@ -163,7 +163,8 @@ static void kvm_update_stolen_time(struct kvm_vcpu *vcpu)
>  		return;
>  
>  	gpa &= KVM_STEAL_PHYS_MASK;
> -	if (unlikely(!kvm_check_gen(vcpu->kvm, ghc->generation) || gpa != ghc->gpa ||
> +	if (unlikely(!kvm_check_gen(vcpu->kvm, ghc->slots_generation, ghc->attrs_generation) ||
> +		     gpa != ghc->gpa ||
>  		     kvm_is_error_hva(ghc->hva) || !ghc->memslot) {
>  		if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gpa, sizeof(*st))) {
>  			ghc->gpa = INVALID_GPA;
> @@ -1817,7 +1818,8 @@ static void kvm_vcpu_set_pv_preempted(struct kvm_vcpu *vcpu)
>  
>  	gpa &= KVM_STEAL_PHYS_MASK;
>  	ghc = &vcpu->arch.st.cache;
> -	if (unlikely(!kvm_check_gen(vcpu->kvm, ghc->generation) || gpa != ghc->gpa ||
> +	if (unlikely(!kvm_check_gen(vcpu->kvm, ghc->slots_generation, ghc->attrs_generation) ||
> +		     gpa != ghc->gpa ||
>  		     kvm_is_error_hva(ghc->hva) || !ghc->memslot) {

[Severity: Low]
As reported by Sashiko on v2 [1], does this fail to build on LoongArch due to
a missing closing parenthesis in both kvm_update_stolen_time() and
kvm_vcpu_set_pv_preempted()?

Both conditions open two parentheses at if (unlikely( before calling
!kvm_check_gen(vcpu->kvm, ghc->slots_generation, ghc->attrs_generation), but
only close one parenthesis after !ghc->memslot before the opening brace.

[ ... ]

> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index da12f4d43c911..6aa6a0a55b070 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c

[ ... ]

> @@ -2558,6 +2559,53 @@ bool kvm_range_has_vm_memory_attributes(struct kvm *kvm, gfn_t start, gfn_t end,
>  	return true;
>  }
>  
> +/*
> + * Returns true if _any_ gfns in the range [@start, @end) have attributes that
> + * match _any_ bit in @mask.
> + */
> +bool kvm_range_has_any_vm_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_vm_mem_attributes(kvm);
> +	if (!mask)
> +		return false;
> +
> +	if (end == start + 1)
> +		return !!(kvm_get_vm_memory_attributes(kvm, start) & mask);
> +
> +	guard(rcu)();
> +	for (;;) {
> +		do {
> +			entry = xas_next(&xas);
> +		} while (xas_retry(&xas, entry));
> +
> +		if (xas.xa_node == XAS_BOUNDS || xas.xa_index >= end)
> +			break;
> +
> +		if (xa_to_value(entry) & mask)
> +			return true;
> +	}

[Severity: High]
Can kvm_range_has_any_vm_memory_attributes() trigger an RCU CPU stall or soft
lockup when userspace invokes KVM_SET_MEMORY_ATTRIBUTES over a large sparse or
unassigned GFN range?

Unassigned GFNs are represented as NULL entries in kvm->mem_attrs.array, where
xa_to_value(NULL) is 0, so NULL entries do not match mask and do not terminate
the loop. However, the loop uses xas_next(&xas), which increments xas.xa_index
by 1 on every call across NULL slots and empty xa_node subtrees whenever the
XArray has an entry at an index >= end keeping xas.xa_node from reaching
XAS_BOUNDS.

Because the entire loop runs inside guard(rcu)() without rescheduling,
scanning a large sparse range will step through every empty GFN index one by
one while holding the RCU read lock. Should this use xas_next_entry(&xas,
end - 1) or xas_for_each(&xas, entry, end - 1) to skip empty slots?

> +
> +	return false;
> +}
> +
> +#define KVM_MEMORY_ATTRIBUTE_NEEDS_SYNC_MASK KVM_MEMORY_ATTRIBUTE_PROT
> +
> +static bool kvm_range_memory_attributes_need_sync(struct kvm *kvm,
> +						  gfn_t start, gfn_t end,
> +						  unsigned long attributes)
> +{
> +	u64 mask = KVM_MEMORY_ATTRIBUTE_NEEDS_SYNC_MASK;
> +
> +	if (attributes & mask)
> +		return true;
> +
> +	return kvm_range_has_any_vm_memory_attributes(kvm, start, end, mask);
> +}
> +
>  static __always_inline void kvm_handle_gfn_range(struct kvm *kvm,
>  						 struct kvm_mmu_notifier_range *range)
>  {
> @@ -2648,20 +2696,25 @@ static int kvm_set_vm_mem_attributes(struct kvm *kvm, gfn_t start, gfn_t end,
>  		.on_lock = kvm_mmu_invalidate_end,
>  		.may_block = true,
>  	};
> +	bool sync = false;
>  	unsigned long i;
>  	void *entry;
>  	int r = 0;
>  
>  	entry = attributes ? xa_mk_value(attributes) : NULL;
>  
> -	trace_kvm_set_vm_mem_attributes(start, end, attributes);
> -
>  	mutex_lock(&kvm->slots_lock);
>  
>  	/* Nothing to do if the entire range has the desired attributes. */
>  	if (kvm_range_has_vm_memory_attributes(kvm, start, end, ~0, attributes))
>  		goto out_unlock;
>  
> +	sync = kvm_range_memory_attributes_need_sync(kvm, start, end,
> +						     attributes);
> +
> +	trace_kvm_set_vm_mem_attributes(start, end, attributes, sync,
> +					kvm->mem_attrs.generation + 1);
> +
>  	/*
>  	 * Reserve memory ahead of time to avoid having to deal with failures
>  	 * partway through setting the new attributes.  Storing NULL never
> @@ -2684,10 +2737,16 @@ static int kvm_set_vm_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);
> +
>  	kvm_handle_gfn_range(kvm, &post_set_range);
>  
>  out_unlock:
>  	mutex_unlock(&kvm->slots_lock);
> +	if (sync)
> +		synchronize_srcu_expedited(&kvm->srcu);

[Severity: Medium]
Should kvm->mem_attrs.generation only be incremented when sync is true, and
should synchronize_srcu_expedited() be skipped if xa_reserve() fails?

The commit message notes that for memory attributes that do not require
synchronization, the attributes generation will always remain unchanged.
However, kvm_set_vm_mem_attributes() unconditionally increments
kvm->mem_attrs.generation via smp_store_release() even when sync is false,
which invalidates all gfn_to_hva_cache and gfn_to_pfn_cache entries across the
VM (and causes non-refreshing cache checks such as
kvm_steal_time_set_preempted() to bail out) when toggling attributes like
KVM_MEMORY_ATTRIBUTE_PRIVATE.

In addition, because sync is computed and trace_kvm_set_vm_mem_attributes() is
called with kvm->mem_attrs.generation + 1 before the xa_reserve() loop:

	for (i = start; entry && i < end; i++) {
		r = xa_reserve(&kvm->mem_attrs.array, i, GFP_KERNEL_ACCOUNT);
		if (r)
			goto out_unlock;

		cond_resched();
	}

if xa_reserve() fails and jumps to out_unlock while sync is true,
kvm_set_vm_mem_attributes() still runs synchronize_srcu_expedited(&kvm->srcu)
even though no attributes or generation numbers were updated.

[ ... ]

> @@ -3582,7 +3641,8 @@ int kvm_vcpu_write_guest(struct kvm_vcpu *vcpu, gpa_t gpa, const void *data,
>  }
>  EXPORT_SYMBOL_FOR_KVM_INTERNAL(kvm_vcpu_write_guest);
>  
> -static int __kvm_gfn_to_hva_cache_init(struct kvm_memslots *slots,
> +static int __kvm_gfn_to_hva_cache_init(struct kvm *kvm,
> +				       struct kvm_memslots *slots,
>  				       struct gfn_to_hva_cache *ghc,
>  				       gpa_t gpa, unsigned long len)
>  {
> @@ -3592,8 +3652,8 @@ static int __kvm_gfn_to_hva_cache_init(struct kvm_memslots *slots,
>  	gfn_t nr_pages_needed = end_gfn - start_gfn + 1;
>  	gfn_t nr_pages_avail;
>  
> -	/* Update ghc->generation before performing any error checks. */
> -	ghc->generation = slots->generation;
> +	/* Update ghc->slots_generation before performing any error checks. */
> +	ghc->slots_generation = slots->generation;
>  
>  	if (start_gfn > end_gfn) {
>  		ghc->hva = KVM_HVA_ERR_BAD;
> @@ -3612,6 +3672,8 @@ static int __kvm_gfn_to_hva_cache_init(struct kvm_memslots *slots,
>  			return -EFAULT;
>  	}
>  
> +	ghc->attrs_generation = kvm_mem_attributes_generation(kvm);

[Severity: Low]
Should ghc->attrs_generation be updated alongside ghc->slots_generation before
the error checks in __kvm_gfn_to_hva_cache_init()?

ghc->slots_generation is updated before checking start_gfn > end_gfn or
running the gfn_to_hva_many() loop so that when an invalid GPA sets ghc->hva
to KVM_HVA_ERR_BAD, subsequent calls to kvm_read_guest_offset_cached() or
kvm_write_guest_offset_cached() pass kvm_memslots_check_gen() and immediately
hit the fast-path kvm_is_error_hva(ghc->hva) check.

Because ghc->attrs_generation is only updated after those error returns (which
remains the case after the follow-up commit "KVM: Take memory protections into
account for accesses with cached gfn->hva"), if a cached range fails in the
memslot lookup loop after kvm->mem_attrs.generation has advanced,
ghc->attrs_generation remains stale and every subsequent cached access will
repeatedly fail kvm_memslots_check_gen() and re-run
__kvm_gfn_to_hva_cache_init().

[1] https://lore.kernel.org/r/20260918081543.139871-19-pbonzini@redhat.com

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