Re: [PATCH] RISC-V: KVM: Serialize IMSIC attributes with vCPU migration

[sashiko-bot@kernel.org]

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

New issues:
- [High] Uninterruptible `mutex_lock` in device ioctl causes unkillable task hang.

Pre-existing issues:
- [High] Hardware state corruption due to missing CPU check in `kvm_riscv_vcpu_aia_imsic_put()`.
--

--- Patch [1]: [PATCH] RISC-V: KVM: Serialize IMSIC attributes with vCPU migration ---
commit 477c5bd7ef196223bb86313755b20f69e286864b
Author: Xie Bo <xb@ultrarisc.com>
RISC-V: KVM: Serialize IMSIC attributes with vCPU migration

KVM device ioctls are not serialized against KVM_RUN. This patch serializes
the entire IMSIC attribute operation with the target vCPU mutex to prevent
the VS-file from being migrated and recycled until the attribute access
completes.

> diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
> index d38f5de0834c5..2a4f88efee17f 100644
> --- a/arch/riscv/kvm/aia_imsic.c
> +++ b/arch/riscv/kvm/aia_imsic.c
> @@ -969,9 +969,13 @@ int kvm_riscv_aia_imsic_rw_attr(struct kvm *kvm, unsigned long type,
>  	if (!vcpu)
>  		return -ENODEV;
>  
> +	mutex_lock(&vcpu->mutex);

[Severity: High]
Does this uninterruptible mutex_lock() cause an unkillable task hang if the
target vCPU is currently executing a guest via KVM_RUN and holding its
vcpu->mutex?

Since it is reachable by userspace via KVM_SET_DEVICE_ATTR or
KVM_GET_DEVICE_ATTR, could a user program racing with KVM_RUN block
indefinitely in D-state?

Should this use mutex_lock_killable() instead to prevent a local denial
of service that triggers the hung task watchdog?

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but looking at
kvm_riscv_vcpu_aia_imsic_put(), does it blindly set CSR_HGEIE on the local
CPU without verifying that imsic->vsfile_cpu matches the current
smp_processor_id()?

arch/riscv/kvm/aia_imsic.c:kvm_riscv_vcpu_aia_imsic_put() {
    ...
    if (imsic->vsfile_cpu > -1)
        csr_set(CSR_HGEIE, BIT(imsic->vsfile_hgei));
    ...
}

If a vCPU blocks in kvm_vcpu_block(), it can be migrated to a different CPU
upon wakeup. If it loops and sleeps again without returning to KVM_RUN
(where imsic->vsfile_cpu is updated), kvm_sched_out() will be invoked on the
new CPU.

Could this call kvm_riscv_vcpu_aia_imsic_put() and write to CSR_HGEIE on the
new CPU using imsic->vsfile_hgei allocated for the old CPU, resulting in
cross-CPU hardware state corruption affecting interrupts of other KVM guests?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721073612.137259-1-xb@ultrarisc.com?part=1