Re: [PATCH] LoongArch: KVM: Implement KVM_GET/SET_SREGS for bulk CSR migration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] Redundant vcpu_load()/vcpu_put() in kvm_arch_vcpu_ioctl_get_sregs causes preempt_notifiers list corruption and a guaranteed kernel crash.
--

--- Patch [1]: [PATCH] LoongArch: KVM: Implement KVM_GET/SET_SREGS for bulk CSR migration ---
commit 0b0da4f87cc239c49e5848ae374209644b4ea927
Author: Tao Cui <cuitao@kylinos.cn>

LoongArch: KVM: Implement KVM_GET/SET_SREGS for bulk CSR migration
    
This commit adds KVM_GET_SREGS and KVM_SET_SREGS ioctls for LoongArch to
allow bulk CSR migration. It defines the kvm_sregs struct as a flat array
to optimize performance compared to multiple KVM_GET/SET_ONE_REG calls.

> diff --git a/arch/loongarch/kvm/vcpu.c b/arch/loongarch/kvm/vcpu.c
> index 20c207d80e31a..0890b68efa2ae 100644
> --- a/arch/loongarch/kvm/vcpu.c
> +++ b/arch/loongarch/kvm/vcpu.c

[ ... ]

> @@ -1001,12 +1001,51 @@ static int kvm_set_reg(struct kvm_vcpu *vcpu, const struct kvm_one_reg *reg)
>  
>  int kvm_arch_vcpu_ioctl_get_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs)
>  {
> -	return -ENOIOCTLCMD;
> +	int i;
> +	unsigned long estat, gintc;
> +	struct loongarch_csrs *csr = vcpu->arch.csr;
> +
> +	/*
> +	 * Pull pending interrupts into ESTAT with a single vcpu_load/put so
> +	 * the ESTAT value read below matches the current interrupt state.
> +	 * This also avoids the per-register load/put side-effect that makes
> +	 * the ONE_REG path's ESTAT snapshot order-sensitive.  The SW CSR
> +	 * reads run under vcpu->mutex, which serialises this ioctl.
> +	 */
> +	preempt_disable();
> +	vcpu_load(vcpu);

[Severity: Critical]
Will this vcpu_load() call corrupt the preempt_notifiers hlist?

The KVM generic ioctl handler kvm_vcpu_ioctl() in virt/kvm/kvm_main.c already
calls vcpu_load() before dispatching KVM_GET_SREGS. Calling vcpu_load() a 
second time here will double-register the preempt notifier, which calls 
hlist_add_head() on a node already at the head of the list, creating a 
circular reference.

> +	kvm_deliver_intr(vcpu);
> +	vcpu->arch.aux_inuse &= ~KVM_LARCH_SWCSR_LATEST;
> +	vcpu_put(vcpu);

[Severity: Critical]
Does this vcpu_put() cause a guaranteed kernel panic?

Because the node's pointers were corrupted by the double vcpu_load(), this
vcpu_put() unregisters the node but poisons its pointers. When the ioctl
returns to the generic kvm_vcpu_ioctl() handler, the outer vcpu_put() will
attempt to unregister the node again. This writes to LIST_POISON2 and
unconditionally triggers a fatal page fault on the host.

> +	preempt_enable();
> +
> +	for (i = 0; i < KVM_LOONGARCH_NR_SREGS; i++) {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260721121842.156025-1-cui.tao@linux.dev?part=1