Re: [PATCH RFC 09/10] KVM: x86: Add guarded_by annotations for kvm_arch, kvm_hv, and ioapic

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Mixing goto-based error handling and scope-based cleanup in the same function.

Pre-existing issues:
- [High] Lockless traversal of the global vm_list can race with VM creation and destruction.
--

commit b3fea266e46bae40374748855ac1c207f8a8f74f
Author: Marco Elver <elver@google.com>

    KVM: x86: Add guarded_by annotations for kvm_arch, kvm_hv, and ioapic
    
    Add __guarded_by annotations to a subset of fields across x86 state
    (struct kvm_hv, struct kvm_arch, and struct kvm_ioapic) where the
    protecting locks reside in the same struct scope. Mark deliberate
    lockless updates with data_race().

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index de7c6eb1a64f5..af03911f0c4fa 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9813,8 +9813,8 @@ int kvm_arch_enable_virtualization_cpu(void)
>  			 * you may have some problem.  Solving this issue is
>  			 * left as an exercise to the reader.
>  			 */
> -			kvm->arch.last_tsc_nsec = 0;
> -			kvm->arch.last_tsc_write = 0;
> +			data_race(kvm->arch.last_tsc_nsec = 0);
> +			data_race(kvm->arch.last_tsc_write = 0);

[Severity: High]
This isn't a bug introduced by this patch, but does this lockless traversal of
the global vm_list in kvm_arch_enable_virtualization_cpu() race with VM
creation and destruction?

If a user concurrently creates a VM (kvm_create_vm) or destroys a VM
(kvm_destroy_vm) while CPU hotplug is running via kvm_online_cpu(), the
list_add() or list_del() executed under kvm_lock will race with this traversal.
This can potentially lead to list corruption, use-after-free, or infinite
loops.

While this patch masks static analyzer warnings by adding data_race()
annotations here, shouldn't the broader list traversal safety be addressed?

[ ... ]

> @@ -9927,8 +9927,8 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  	kvm->arch.enable_pmu = enable_pmu && !kvm->arch.has_protected_pmu;
>  
>  #if IS_ENABLED(CONFIG_HYPERV)
> -	spin_lock_init(&kvm->arch.hv_root_tdp_lock);
> -	kvm->arch.hv_root_tdp = INVALID_PAGE;
> +	scoped_guard(spinlock_init, &kvm->arch.hv_root_tdp_lock)
> +		kvm->arch.hv_root_tdp = INVALID_PAGE;

[Severity: Low]
Does introducing scoped_guard() here mix scope-based cleanup with goto-based
error handling in the same function?

kvm_arch_init_vm() already relies on goto labels (e.g., goto out_uninit_mmu,
goto out_cleanup_page_track) for its error paths. According to the subsystem
coding standards, a function must exclusively use either goto-based cleanup or
scope-based cleanup helpers, but never mix both to prevent confusing ownership
semantics and lifecycle bugs.

Could this be kept as goto-based, or should the entire function be converted
to scope-based cleanup?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910162343.4092060-1-elver@google.com?part=9