Re: [PATCH] RISC-V: KVM: Convert IMSIC vsfile_lock to a raw spinlock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] KVM: riscv: unordered MMIO write in
  kvm_riscv_vcpu_aia_imsic_inject()
- [High] KVM: riscv: race on IRQ_VS_EXT in
  kvm_riscv_vcpu_aia_imsic_inject()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] RISC-V: KVM: double-free in
  kvm_riscv_vcpu_aia_imsic_update() [1]
--

commit 7416cdfbe4d0521c182f4faca63b3ac6e4ae60b3
Author: Anup Patel <anup.patel@oss.qualcomm.com>

RISC-V: KVM: Convert IMSIC vsfile_lock to a raw spinlock

This commit converts imsic->vsfile_lock from rwlock_t to raw_spinlock_t,
removes vsfile_hgei_lock, moves imsic_swfile_extirq_update() outside
vsfile_lock, and switches kvm_riscv_vcpu_aia_imsic_inject() to
writel_relaxed().

> diff --git a/arch/riscv/kvm/aia_imsic.c b/arch/riscv/kvm/aia_imsic.c
> index 302938972aa0e..0b9d15721d52f 100644
> --- a/arch/riscv/kvm/aia_imsic.c
> +++ b/arch/riscv/kvm/aia_imsic.c

[ ... ]

> @@ -726,10 +715,10 @@ int kvm_riscv_vcpu_aia_imsic_update(struct kvm_vcpu *vcpu)
>  		return 1;
>  
>  	/* Read old IMSIC VS-file details */
> -	read_lock_irqsave(&imsic->vsfile_lock, flags);
> +	raw_spin_lock_irqsave(&imsic->vsfile_lock, flags);
>  	old_vsfile_hgei = imsic->vsfile_hgei;
>  	old_vsfile_cpu = imsic->vsfile_cpu;
> -	read_unlock_irqrestore(&imsic->vsfile_lock, flags);
> +	raw_spin_unlock_irqrestore(&imsic->vsfile_lock, flags);

[Severity: Critical]
This problem wasn't introduced by this patch, but can [1] cause a
double free of old_vsfile_hgei when kvm_riscv_vcpu_aia_imsic_update()
races with kvm_riscv_vcpu_aia_imsic_release()?

kvm_riscv_vcpu_aia_imsic_update() caches old_vsfile_hgei and
old_vsfile_cpu under imsic->vsfile_lock and drops the lock before later
freeing the old HGEI slot:

CPU0                                  CPU1
kvm_riscv_vcpu_aia_imsic_update()     kvm_riscv_vcpu_aia_imsic_release()
  raw_spin_lock_irqsave(...)
  old_vsfile_hgei = imsic->vsfile_hgei;
  old_vsfile_cpu = imsic->vsfile_cpu;
  raw_spin_unlock_irqrestore(...)
                                        raw_spin_lock_irqsave(...)
                                        old_vsfile_hgei = imsic->vsfile_hgei;
                                        old_vsfile_cpu = imsic->vsfile_cpu;
                                        imsic->vsfile_cpu = -1;
                                        imsic->vsfile_hgei = -1;
                                        raw_spin_unlock_irqrestore(...)
                                        ...
                                        kvm_riscv_aia_free_hgei(...)
  ...
  if (old_vsfile_cpu >= 0) {
    ...
    kvm_riscv_aia_free_hgei(...)  <-- double free

If a concurrent CPU hotplug or vCPU reset runs
kvm_riscv_vcpu_aia_imsic_release() in that window, both paths free the
same HGEI slot.  If that slot was reallocated to another VM in between,
the second free corrupts cross-VM interrupt state.

[ ... ]

> @@ -970,17 +965,26 @@ int kvm_riscv_vcpu_aia_imsic_inject(struct kvm_vcpu *vcpu,
>  	if (imsic->nr_msis <= iid)
>  		return -EINVAL;
>  
> -	read_lock_irqsave(&imsic->vsfile_lock, flags);
> +	raw_spin_lock_irqsave(&imsic->vsfile_lock, flags);
>  
>  	if (imsic->vsfile_cpu >= 0) {
> -		writel(iid, imsic->vsfile_va + IMSIC_MMIO_SETIPNUM_LE);
> +		/*
> +		 * We can use writel_relaxed() over here because the
> +		 * raw_spin_unlock() below will do mmiowb_spin_unlock()
> +		 * which in-turn drains the write-buffer ensuring the
> +		 * write is visible to the CPU owning the IMSIC VS-file.
> +		 */
> +		writel_relaxed(iid, imsic->vsfile_va + IMSIC_MMIO_SETIPNUM_LE);

[Severity: High]
Does replacing writel() with writel_relaxed() leave the MMIO write to
imsic->vsfile_va unordered with raw_spin_unlock_irqrestore()?

mmiowb_spin_unlock() only emits mmiowb() when ms->mmiowb_pending is set:

include/asm-generic/mmiowb.h:mmiowb_spin_unlock() {
	struct mmiowb_state *ms = __mmiowb_state();

	if (unlikely(ms->mmiowb_pending)) {
		ms->mmiowb_pending = 0;
		mmiowb();
	}
}

On RISC-V, writel() calls __io_bw() and __io_aw() (which calls
mmiowb_set_pending()), whereas writel_relaxed() uses __io_rbw() and
__io_raw(), which are both no-ops.  Because writel_relaxed() never sets
ms->mmiowb_pending, raw_spin_unlock_irqrestore() does not emit mmiowb()
(RISCV_FENCE(o, w)).  If the vCPU concurrently migrates or releases its
VS-file in kvm_riscv_vcpu_aia_imsic_update() or
kvm_riscv_vcpu_aia_imsic_release(), the store to IMSIC_MMIO_SETIPNUM_LE
can arrive after imsic_vsfile_read() has cleared the old VS-file and
kvm_riscv_aia_free_hgei() has freed the slot, losing the guest interrupt
or delivering a spurious MSI to another vCPU.

>  	} else {
>  		eix = &imsic->swfile->eix[iid / BITS_PER_TYPE(u64)];
>  		set_bit(iid & (BITS_PER_TYPE(u64) - 1), eix->eip);
> -		imsic_swfile_extirq_update(vcpu);
> +		swfile_update = true;
>  	}
>  
> -	read_unlock_irqrestore(&imsic->vsfile_lock, flags);
> +	raw_spin_unlock_irqrestore(&imsic->vsfile_lock, flags);
> +
> +	if (swfile_update)
> +		imsic_swfile_extirq_update(vcpu);

[Severity: High]
Can moving imsic_swfile_extirq_update() outside imsic->vsfile_lock race
with kvm_riscv_vcpu_aia_imsic_update() and leave IRQ_VS_EXT stuck pending?

imsic_swfile_read() clears imsic->swfile and unsets IRQ_VS_EXT without
taking swfile_extirq_lock, relying on vsfile_lock to serialize against
SW-file producers:

CPU0                                  CPU1
kvm_riscv_vcpu_aia_imsic_inject()     kvm_riscv_vcpu_aia_imsic_update()
  raw_spin_lock_irqsave(&vsfile_lock)
  /* vsfile_cpu < 0 */
  set_bit(..., eix->eip)
  swfile_update = true
  raw_spin_unlock_irqrestore(...)
  imsic_swfile_extirq_update()
    raw_spin_lock_irqsave(&extirq_lock)
    /* reads eidelivery && topei != 0 */
                                        raw_spin_lock_irqsave(&vsfile_lock)
                                        imsic->vsfile_cpu = vcpu->cpu
                                        raw_spin_unlock_irqrestore(...)
                                        imsic_swfile_read(..., true, ...)
                                          memset(imsic->swfile, 0, ...)
                                          kvm_riscv_vcpu_unset_interrupt(
                                              vcpu, IRQ_VS_EXT)
    kvm_riscv_vcpu_set_interrupt(
        vcpu, IRQ_VS_EXT)  <-- re-asserts IRQ_VS_EXT

Once the vCPU is using a hardware VS-file (HSTATUS.VGEIN > 0), guest
accesses to CSR_VSTOPEI are handled by hardware without trapping, so
nothing clears IRQ_VS_EXT in CSR_HVIP and the guest vCPU is wedged in a
spurious external interrupt storm.

[1] https://sashiko.dev/#/bug/linux-0a58883c-0da3-49e7-847e-a71b508f0638

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114135056.202556-1-anup.patel@oss.qualcomm.com?part=1