[PATCH] x86/kvm: Use RCU for svm_vm_data_hash

[PATCH] x86/kvm: Use RCU for svm_vm_data_hash

[Sairaj Kodilkar]

GALOG notifier iterates svm_vm_data_hash to find the `struct kvm_vcpu`
for the given vm id and vcpu id. Holding spinlock is not ideal here since
it allows only a single reader to iterate over the hash. This degrades
performance when interrupt rate is high and large number of readers are
waiting for the spinlock. Hence replace spinlock with the RCU.

Signed-off-by: Sairaj Kodilkar <sarunkod@amd.com>
---

Tested on a 2-socket AMD EPYC 9965 host against the same kernel without
this change:

  - 12 VMs, each with 32 vCPUs and one passed-through NVMe device
  - vCPUs pinned NUMA-local, at 1:1, 1:2, 1:3 and 1:4 pCPU:vCPU
  - fio in each guest: randread, bs=4k, iodepth=32, numjobs=32,
    ioengine=libaio, direct=1, 60s

Time spent in the hash lookup in avic_ga_log_notifier(), measured with
a temporary tracepoint around the lookup, averaged over 12 VMs and
3 runs:

             |         mean           |          p99         |
             |------------------------|----------------------|
  pCPU:vCPU  | spinlock  |     RCU    | spinlock  |     RCU  |
-------------|-----------|------------|-----------|----------|
  1:1        | 409.8 ns  |  132.6 ns  |   960 ns  |  750 ns  |
  1:2        | 977.3 ns  |   21.9 ns  |  3530 ns  |   30 ns  |
  1:3        | 747.5 ns  |   23.7 ns  |  2150 ns  |   40 ns  |
  1:4        | 614.8 ns  |   23.5 ns  |  1610 ns  |   40 ns  |

Total IOPS was unchanged at every ratio.

---
 arch/x86/kvm/svm/avic.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
index 96ca39c0045f..9e2c11d60798 100644
--- a/arch/x86/kvm/svm/avic.c
+++ b/arch/x86/kvm/svm/avic.c
@@ -250,7 +250,6 @@ static void avic_deactivate_vmcb(struct vcpu_svm *svm)
  */
 static int avic_ga_log_notifier(u32 ga_tag)
 {
-	unsigned long flags;
 	struct kvm_svm *kvm_svm;
 	struct kvm_vcpu *vcpu = NULL;
 	u32 vm_id = AVIC_GATAG_TO_VMID(ga_tag);
@@ -259,14 +258,14 @@ static int avic_ga_log_notifier(u32 ga_tag)
 	pr_debug("SVM: %s: vm_id=%#x, vcpu_idx=%#x\n", __func__, vm_id, vcpu_idx);
 	trace_kvm_avic_ga_log(vm_id, vcpu_idx);
 
-	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
-	hash_for_each_possible(svm_vm_data_hash, kvm_svm, hnode, vm_id) {
+	rcu_read_lock();
+	hash_for_each_possible_rcu(svm_vm_data_hash, kvm_svm, hnode, vm_id) {
 		if (kvm_svm->avic_vm_id != vm_id)
 			continue;
 		vcpu = kvm_get_vcpu(&kvm_svm->kvm, vcpu_idx);
 		break;
 	}
-	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
+	rcu_read_unlock();
 
 	/* Note:
 	 * At this point, the IOMMU should have already set the pending
@@ -319,13 +318,14 @@ static void avic_add_vm_to_ga_log_list(struct kvm *kvm)
 	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
 	struct kvm_svm *k2;
 	u32 vm_id;
+	unsigned long flags;
 
 	lockdep_assert_held(&kvm->lock);
 
 	if (kvm_svm->avic_vm_id)
 		return;
 
-	guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
+	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
  again:
 	vm_id = next_vm_id = (next_vm_id + 1) & AVIC_VM_ID_MASK;
 	if (vm_id == 0) { /* id is 1-based, zero is not okay */
@@ -340,7 +340,8 @@ static void avic_add_vm_to_ga_log_list(struct kvm *kvm)
 		}
 	}
 	kvm_svm->avic_vm_id = vm_id;
-	hash_add(svm_vm_data_hash, &kvm_svm->hnode, kvm_svm->avic_vm_id);
+	hash_add_rcu(svm_vm_data_hash, &kvm_svm->hnode, kvm_svm->avic_vm_id);
+	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
 }
 
 int avic_vcpu_precreate(struct kvm *kvm)
@@ -369,13 +370,14 @@ int avic_vcpu_precreate(struct kvm *kvm)
 void avic_vm_pre_destroy(struct kvm *kvm)
 {
 	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
+	unsigned long flags;
 
 	if (WARN_ON_ONCE(!enable_apicv) || !kvm_svm->avic_vm_id)
 		return;
 
-	guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
-
-	hash_del(&kvm_svm->hnode);
+	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
+	hash_del_rcu(&kvm_svm->hnode);
+	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
 }
 
 void avic_vm_destroy(struct kvm *kvm)
-- 
2.34.1

Re: [PATCH] x86/kvm: Use RCU for svm_vm_data_hash

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] KVM: SVM: UAF in avic_ga_log_notifier() and
  avic_vm_pre_destroy() with RCU
- [Low] KVM: SVM: unnecessary guard() removal in
  avic_add_vm_to_ga_log_list()
--

commit 460915a518276bc08e3b21ecfcfa94bc27d07493
Author: Sairaj Kodilkar <sarunkod@amd.com>

x86/kvm: Use RCU for svm_vm_data_hash

This commit replaces the spinlock-protected lookup of svm_vm_data_hash in
avic_ga_log_notifier() with an RCU read-side critical section, and updates
insertion and removal to use hash_add_rcu() and hash_del_rcu().

> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
> index 3b037e3855234..ba12aa5872255 100644
> --- a/arch/x86/kvm/svm/avic.c
> +++ b/arch/x86/kvm/svm/avic.c
> @@ -250,7 +250,6 @@ static void avic_deactivate_vmcb(struct vcpu_svm *svm)
>   */
>  static int avic_ga_log_notifier(u32 ga_tag)
>  {
> -	unsigned long flags;
>  	struct kvm_svm *kvm_svm;
>  	struct kvm_vcpu *vcpu = NULL;
>  	u32 vm_id = AVIC_GATAG_TO_VMID(ga_tag);
> @@ -259,14 +258,14 @@ static int avic_ga_log_notifier(u32 ga_tag)
>  	pr_debug("SVM: %s: vm_id=%#x, vcpu_idx=%#x\n", __func__, vm_id, vcpu_idx);
>  	trace_kvm_avic_ga_log(vm_id, vcpu_idx);
>  
> -	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
> -	hash_for_each_possible(svm_vm_data_hash, kvm_svm, hnode, vm_id) {
> +	rcu_read_lock();
> +	hash_for_each_possible_rcu(svm_vm_data_hash, kvm_svm, hnode, vm_id) {
>  		if (kvm_svm->avic_vm_id != vm_id)
>  			continue;
>  		vcpu = kvm_get_vcpu(&kvm_svm->kvm, vcpu_idx);
>  		break;
>  	}
> -	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
> +	rcu_read_unlock();
>  
>  	/* Note:
>  	 * At this point, the IOMMU should have already set the pending
> @@ -319,13 +318,14 @@ static void avic_add_vm_to_ga_log_list(struct kvm *kvm)
>  	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
>  	struct kvm_svm *k2;
>  	u32 vm_id;
> +	unsigned long flags;
>  
>  	lockdep_assert_held(&kvm->lock);
>  
>  	if (kvm_svm->avic_vm_id)
>  		return;
>  
> -	guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
> +	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
>   again:
>  	vm_id = next_vm_id = (next_vm_id + 1) & AVIC_VM_ID_MASK;
>  	if (vm_id == 0) { /* id is 1-based, zero is not okay */
> @@ -340,7 +340,8 @@ static void avic_add_vm_to_ga_log_list(struct kvm *kvm)
>  		}
>  	}
>  	kvm_svm->avic_vm_id = vm_id;
> -	hash_add(svm_vm_data_hash, &kvm_svm->hnode, kvm_svm->avic_vm_id);
> +	hash_add_rcu(svm_vm_data_hash, &kvm_svm->hnode, kvm_svm->avic_vm_id);
> +	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
>  }

[Severity: Low]
This isn't a bug, but commits 4fa9a3767d1a ("KVM: SVM: Do all per-VM AVIC
initialization during vCPU precreation phase") and 78684b65fcc0 ("KVM: SVM:
Remove VM from the GA Log notifier list before VM destruction") switched
avic_add_vm_to_ga_log_list() and avic_vm_pre_destroy() to
guard(spinlock_irqsave)(&svm_vm_data_hash_lock), which can still be kept
here instead of open-coding spin_lock_irqsave() and spin_unlock_irqrestore().

>  int avic_vcpu_precreate(struct kvm *kvm)
> @@ -369,13 +370,14 @@ int avic_vcpu_precreate(struct kvm *kvm)
>  void avic_vm_pre_destroy(struct kvm *kvm)
>  {
>  	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
> +	unsigned long flags;
>  
>  	if (WARN_ON_ONCE(!enable_apicv) || !kvm_svm->avic_vm_id)
>  		return;
>  
> -	guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
> -
> -	hash_del(&kvm_svm->hnode);
> +	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
> +	hash_del_rcu(&kvm_svm->hnode);
> +	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
>  }

[Severity: High]
Can this cause a use-after-free on struct kvm_svm and struct kvm_vcpu
when a VM is destroyed concurrently with avic_ga_log_notifier()?

CPU0                              CPU1
avic_ga_log_notifier()            kvm_destroy_vm()
  rcu_read_lock()                   ...
  hash_for_each_possible_rcu(...)   avic_vm_pre_destroy()
    vcpu = kvm_get_vcpu(...)          hash_del_rcu(&kvm_svm->hnode)
  rcu_read_unlock()                 ...
                                    kvm_destroy_vcpus()
                                      kmem_cache_free(..., vcpu)
                                    kvm_arch_free_vm()
                                      vfree(kvm)
  kvm_vcpu_wake_up(vcpu)  <-- use-after-free

avic_vm_pre_destroy() unlinks kvm_svm->hnode with hash_del_rcu(), but
nothing waits for an RCU grace period before kvm_destroy_vcpus() frees the
vCPUs and kvm_arch_free_vm() frees struct kvm_svm (the intervening SRCU
synchronizations in kvm_destroy_vm() only cover private SRCU domains). An
in-flight reader on the same hash bucket can therefore dereference freed
kvm_svm or vcpu memory.

In addition, avic_ga_log_notifier() calls rcu_read_unlock() before
kvm_vcpu_wake_up(vcpu). Because kvm_get_vcpu() does not take a reference,
dropping the RCU read lock early leaves vcpu unpinned even if
avic_vm_pre_destroy() waits for a grace period. Calling kvm_vcpu_wake_up()
inside the rcu_read_lock() critical section and adding synchronize_rcu()
after hash_del_rcu() in avic_vm_pre_destroy() would close both windows.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111100246.4425-1-sarunkod@amd.com?part=1

Re: [PATCH] x86/kvm: Use RCU for svm_vm_data_hash

[Sairaj Kodilkar]

On 11/11/2026 3:47 PM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential
> issues to consider:
> - [High] KVM: SVM: UAF in avic_ga_log_notifier() and
>   avic_vm_pre_destroy() with RCU
> - [Low] KVM: SVM: unnecessary guard() removal in
>   avic_add_vm_to_ga_log_list()
> --
> 
> commit 460915a518276bc08e3b21ecfcfa94bc27d07493
> Author: Sairaj Kodilkar <sarunkod@amd.com>
> 
> x86/kvm: Use RCU for svm_vm_data_hash
> 
> This commit replaces the spinlock-protected lookup of svm_vm_data_hash in
> avic_ga_log_notifier() with an RCU read-side critical section, and updates
> insertion and removal to use hash_add_rcu() and hash_del_rcu().
> 
>> diff --git a/arch/x86/kvm/svm/avic.c b/arch/x86/kvm/svm/avic.c
>> index 3b037e3855234..ba12aa5872255 100644
>> --- a/arch/x86/kvm/svm/avic.c
>> +++ b/arch/x86/kvm/svm/avic.c
>> @@ -250,7 +250,6 @@ static void avic_deactivate_vmcb(struct vcpu_svm *svm)
>>   */
>>  static int avic_ga_log_notifier(u32 ga_tag)
>>  {
>> -	unsigned long flags;
>>  	struct kvm_svm *kvm_svm;
>>  	struct kvm_vcpu *vcpu = NULL;
>>  	u32 vm_id = AVIC_GATAG_TO_VMID(ga_tag);
>> @@ -259,14 +258,14 @@ static int avic_ga_log_notifier(u32 ga_tag)
>>  	pr_debug("SVM: %s: vm_id=%#x, vcpu_idx=%#x\n", __func__, vm_id, vcpu_idx);
>>  	trace_kvm_avic_ga_log(vm_id, vcpu_idx);
>>  
>> -	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
>> -	hash_for_each_possible(svm_vm_data_hash, kvm_svm, hnode, vm_id) {
>> +	rcu_read_lock();
>> +	hash_for_each_possible_rcu(svm_vm_data_hash, kvm_svm, hnode, vm_id) {
>>  		if (kvm_svm->avic_vm_id != vm_id)
>>  			continue;
>>  		vcpu = kvm_get_vcpu(&kvm_svm->kvm, vcpu_idx);
>>  		break;
>>  	}
>> -	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
>> +	rcu_read_unlock();
>>  
>>  	/* Note:
>>  	 * At this point, the IOMMU should have already set the pending
>> @@ -319,13 +318,14 @@ static void avic_add_vm_to_ga_log_list(struct kvm *kvm)
>>  	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
>>  	struct kvm_svm *k2;
>>  	u32 vm_id;
>> +	unsigned long flags;
>>  
>>  	lockdep_assert_held(&kvm->lock);
>>  
>>  	if (kvm_svm->avic_vm_id)
>>  		return;
>>  
>> -	guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
>> +	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
>>   again:
>>  	vm_id = next_vm_id = (next_vm_id + 1) & AVIC_VM_ID_MASK;
>>  	if (vm_id == 0) { /* id is 1-based, zero is not okay */
>> @@ -340,7 +340,8 @@ static void avic_add_vm_to_ga_log_list(struct kvm *kvm)
>>  		}
>>  	}
>>  	kvm_svm->avic_vm_id = vm_id;
>> -	hash_add(svm_vm_data_hash, &kvm_svm->hnode, kvm_svm->avic_vm_id);
>> +	hash_add_rcu(svm_vm_data_hash, &kvm_svm->hnode, kvm_svm->avic_vm_id);
>> +	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
>>  }
> 
> [Severity: Low]
> This isn't a bug, but commits 4fa9a3767d1a ("KVM: SVM: Do all per-VM AVIC
> initialization during vCPU precreation phase") and 78684b65fcc0 ("KVM: SVM:
> Remove VM from the GA Log notifier list before VM destruction") switched
> avic_add_vm_to_ga_log_list() and avic_vm_pre_destroy() to
> guard(spinlock_irqsave)(&svm_vm_data_hash_lock), which can still be kept
> here instead of open-coding spin_lock_irqsave() and spin_unlock_irqrestore().
> 

Right, will remove this

>>  int avic_vcpu_precreate(struct kvm *kvm)
>> @@ -369,13 +370,14 @@ int avic_vcpu_precreate(struct kvm *kvm)
>>  void avic_vm_pre_destroy(struct kvm *kvm)
>>  {
>>  	struct kvm_svm *kvm_svm = to_kvm_svm(kvm);
>> +	unsigned long flags;
>>  
>>  	if (WARN_ON_ONCE(!enable_apicv) || !kvm_svm->avic_vm_id)
>>  		return;
>>  
>> -	guard(spinlock_irqsave)(&svm_vm_data_hash_lock);
>> -
>> -	hash_del(&kvm_svm->hnode);
>> +	spin_lock_irqsave(&svm_vm_data_hash_lock, flags);
>> +	hash_del_rcu(&kvm_svm->hnode);
>> +	spin_unlock_irqrestore(&svm_vm_data_hash_lock, flags);
>>  }
> 
> [Severity: High]
> Can this cause a use-after-free on struct kvm_svm and struct kvm_vcpu
> when a VM is destroyed concurrently with avic_ga_log_notifier()?
> 
> CPU0                              CPU1
> avic_ga_log_notifier()            kvm_destroy_vm()
>   rcu_read_lock()                   ...
>   hash_for_each_possible_rcu(...)   avic_vm_pre_destroy()
>     vcpu = kvm_get_vcpu(...)          hash_del_rcu(&kvm_svm->hnode)
>   rcu_read_unlock()                 ...
>                                     kvm_destroy_vcpus()
>                                       kmem_cache_free(..., vcpu)
>                                     kvm_arch_free_vm()
>                                       vfree(kvm)
>   kvm_vcpu_wake_up(vcpu)  <-- use-after-free
> 
> avic_vm_pre_destroy() unlinks kvm_svm->hnode with hash_del_rcu(), but
> nothing waits for an RCU grace period before kvm_destroy_vcpus() frees the
> vCPUs and kvm_arch_free_vm() frees struct kvm_svm (the intervening SRCU
> synchronizations in kvm_destroy_vm() only cover private SRCU domains). An
> in-flight reader on the same hash bucket can therefore dereference freed
> kvm_svm or vcpu memory.
> 
> In addition, avic_ga_log_notifier() calls rcu_read_unlock() before
> kvm_vcpu_wake_up(vcpu). Because kvm_get_vcpu() does not take a reference,
> dropping the RCU read lock early leaves vcpu unpinned even if
> avic_vm_pre_destroy() waits for a grace period. Calling kvm_vcpu_wake_up()
> inside the rcu_read_lock() critical section and adding synchronize_rcu()
> after hash_del_rcu() in avic_vm_pre_destroy() would close both windows.
> 

Right, will do it

Thanks
Sairaj