Re: [PATCH 4/6] LoongArch: KVM: implement vmid updating logic

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] The separation of VMID from VPID for hardware with `cpu_has_guestid` breaks the handling of `KVM_REQ_TLB_FLUSH` for Stage-2 TLBs.
- [High] `kvm_check_vmid` prematurely clears the deferred `KVM_REQ_TLB_FLUSH_GPA` request when a vCPU allocates a new VMID on a single physical CPU, leading to stale TLB entries if the vCPU migrates.

Pre-existing issues:
- [High] Error paths in `kvm_loongarch_env_init()` and `kvm_loongarch_init()` fail to clean up dynamically allocated memory and unregister callbacks/devices.
--

--- Patch [4]: [PATCH 4/6] LoongArch: KVM: implement vmid updating logic ---
commit e654984b1077924ffa92adca677721884f2ab85a
Author: Bibo Mao <maobibo@loongson.cn>

LoongArch: KVM: implement vmid updating logic
    
This commit separates the VMID calculation from the VPID logic for LoongArch KVM.
It implements a per-physical-CPU VMID array for VMs to handle hardware with
cpu_has_guestid, and introduces functions to manage VMID allocation and wrapping.

> diff --git a/arch/loongarch/kvm/main.c b/arch/loongarch/kvm/main.c
> index 3df46e02594f4..7b3661da89587 100644
> --- a/arch/loongarch/kvm/main.c
> +++ b/arch/loongarch/kvm/main.c
[ ... ]
> @@ -281,9 +281,33 @@ static void kvm_check_vcpuid(struct kvm_vcpu *vcpu)
>  	}
>  }
>  
> -static void kvm_check_vmid(struct kvm_vcpu *vcpu)
> +static void kvm_update_vmid(struct kvm_vcpu *vcpu, int cpu)
[ ... ]
> +static void kvm_check_vmid(struct kvm_vcpu *vcpu)
> +{
> +	int cpu;
> +	unsigned long ver, old, vmid;
> +	struct kvm_context *context;
[ ... ]
> +	cpu = smp_processor_id();
> +	context = per_cpu_ptr(vcpu->kvm->arch.vmcs, cpu);
> +
> +	/*
> +	 * Check if our vmid is of an older version
> +	 */
> +	ver = vcpu->kvm->arch.vmid[cpu] & ~vpid_mask;
> +	old = context->vmid_cache & ~vpid_mask;
> +	if (ver != old) {
> +		kvm_update_vmid(vcpu, cpu);
> +		kvm_clear_request(KVM_REQ_TLB_FLUSH_GPA, vcpu);
> +	}

[Severity: Critical]
Does separating VMID from VPID break the KVM_REQ_TLB_FLUSH handling?

When the KVM core issues a remote TLB flush via kvm_flush_remote_tlbs(),
kvm_check_requests() handles this by setting vcpu->arch.vpid = 0 to force a
new VPID. 

However, this new logic only evaluates the VM's per-physical-CPU VMID array
(vcpu->kvm->arch.vmid[cpu]). Without checking if the VPID was cleared or
resetting the per-cpu VMID state, does this mean KVM will fail to refresh
the VMID and miss flushing the Stage-2 TLB when requested by the core?
This could allow guests to retain access to unmapped or repurposed host
memory during reclaim.

[Severity: High]
Is it safe to clear KVM_REQ_TLB_FLUSH_GPA based purely on a local
per-physical-CPU VMID update?

If a vCPU sets KVM_REQ_TLB_FLUSH_GPA on CPU A and gets preempted, it could
migrate to CPU B. This block would detect an older version, allocate a new
VMID on CPU B, and blindly clear the global per-vCPU KVM_REQ_TLB_FLUSH_GPA
request.

If the vCPU later migrates back to CPU A and reuses its valid old VMID
(since the VMID on CPU A hasn't wrapped), wouldn't the required GPA flush
on CPU A be missed because the request flag was already prematurely cleared
on CPU B?

> +
> +	vcpu->arch.vmid = vcpu->kvm->arch.vmid[cpu] & vpid_mask;
>  }
>  
>  void kvm_check_vpid(struct kvm_vcpu *vcpu)
> @@ -400,6 +439,7 @@ static int kvm_loongarch_env_init(void)
>  	for_each_possible_cpu(cpu) {
>  		context = per_cpu_ptr(vmcs, cpu);
>  		context->vpid_cache = vpid_mask + 1;
> +		context->vmid_cache = vpid_mask + 1;
>  		context->last_vcpu = NULL;
>  	}

[Severity: High]
This is a pre-existing issue, but I noticed that error paths during
initialization do not seem to clean up allocated structures or unregister
callbacks.

arch/loongarch/kvm/main.c:kvm_loongarch_env_init() {
    ...
    ret = kvm_loongarch_register_ipi_device();
    if (ret)
        return ret;
    ...
}

If a function like kvm_loongarch_register_ipi_device() returns an error,
the function returns directly without freeing vmcs or kvm_loongarch_ops,
and without calling kvm_unregister_perf_callbacks().

If the module load is aborted, does this leave perf_guest_cbs pointing to
unmapped memory, potentially causing a use-after-free kernel panic on the
next perf interrupt?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727072109.2151675-1-maobibo@loongson.cn?part=4