Re: [PATCH v4 5/6] KVM: arm64: Add HDBSS fault handling and buffer flush

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Acquiring `kvm->mmu_lock` inside the preempt notifier (`kvm_sched_out` -> `kvm_arch_vcpu_put`) creates an ABBA deadlock with the runqueue lock.
- [High] `kvm_flush_hdbss_buffer` calls `kvm_vcpu_mark_page_dirty` without holding `srcu_read_lock(&kvm->srcu)`, leading to Use-After-Free.
- [High] `kvm_arch_sync_dirty_log` silently misses dirty pages for vCPUs outside guest mode due to `KVM_REQUEST_WAIT` semantics.
- [High] Flushing the entire HDBSS buffer in a single tight loop can overflow the KVM dirty ring, resulting in dropped dirty pages.
- [High] Directly accessing the EL2 register `SYS_HDBSSPROD_EL2` from the host kernel will crash in nVHE mode.
- [Medium] Missing immediate Context Synchronization Event (`isb()`) after writing to control-plane system register `SYS_HDBSSPROD_EL2`.
--

--- Patch [5]: [PATCH v4 5/6] KVM: arm64: Add HDBSS fault handling and buffer flush ---
commit ac61acd739dfa5c2e5304969807c386e60452069
Author: eillon <yezhenyu2@huawei.com>
Date:   Thu Jul 9 10:40:25 2026 +0000

    KVM: arm64: Add HDBSS fault handling and buffer flush

This patch adds handling for HDBSS (Hardware Dirty Bit State Stream) faults,
triggering flush requests when the hardware buffer becomes full. It implements
kvm_flush_hdbss_buffer to sync the hardware buffer to the dirty bitmap, which
is invoked during vcpu_put, request checks, and dirty log synchronization.

> diff --git a/arch/arm64/kvm/arm.c b/arch/arm64/kvm/arm.c
> index bf6688245d83a..566953a4e23a1 100644
> --- a/arch/arm64/kvm/arm.c
> +++ b/arch/arm64/kvm/arm.c
> @@ -755,6 +755,9 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  		kvm_vcpu_put_hw_mmu(vcpu);
>  	kvm_arm_vmid_clear_active();
>  
> +	if (vcpu->kvm->arch.enable_hdbss)
> +		kvm_flush_hdbss_buffer(vcpu);
> +

[Severity: Critical]
Could this create an ABBA deadlock with the runqueue lock?

When the scheduler preempts a vCPU thread, it invokes the KVM preempt
notifier kvm_sched_out() with the runqueue lock (rq->lock) held and
interrupts disabled. This calls kvm_arch_vcpu_put(), which in turn
calls kvm_flush_hdbss_buffer().

The flush function then acquires vcpu->kvm->mmu_lock. Because other
standard KVM execution paths acquire mmu_lock and may subsequently
wake up tasks (which acquires rq->lock), this establishes an unavoidable
locking inversion between rq->lock and mmu_lock.

[Severity: High]
Is it safe to call kvm_flush_hdbss_buffer() here without holding the
KVM SRCU read lock?

The kvm_flush_hdbss_buffer() function calls kvm_vcpu_mark_page_dirty(),
which dereferences the KVM memslots array via kvm_vcpu_gfn_to_memslot().

When this flush is invoked from kvm_arch_vcpu_put() during vcpu_put(),
kvm->srcu is not held. The mmu_lock does not protect the memslots array
structure itself, which could lead to a use-after-free if memslots are
concurrently modified.

[ ... ]

> @@ -1971,7 +1977,15 @@ long kvm_arch_vcpu_unlocked_ioctl(struct file *filp, unsigned int ioctl,
>  
>  void kvm_arch_sync_dirty_log(struct kvm *kvm, struct kvm_memory_slot *memslot)
>  {
> +	/*
> +	 * Flush all CPUs' dirty log buffers to the dirty_bitmap.  Called
> +	 * before reporting dirty_bitmap to userspace. Send a request with
> +	 * KVM_REQUEST_WAIT to flush buffer synchronously.
> +	 */
> +	if (!kvm->arch.enable_hdbss)
> +		return;
>  
> +	kvm_make_all_cpus_request(kvm, KVM_REQ_FLUSH_HDBSS | KVM_REQUEST_WAIT);
>  }

[Severity: High]
Will this wait for vCPUs that have exited to userspace?

The KVM_REQUEST_WAIT flag causes KVM to wait only for vCPUs that are
currently executing in guest mode. If a vCPU has exited the guest to host
userspace, it is not in guest mode, but its HDBSS hardware buffer might
still contain unflushed dirty pages.

Since the flush request is pended but not waited for, could this return
immediately and cause userspace to read an incomplete dirty bitmap during
live migration?

> diff --git a/arch/arm64/kvm/dirty_bit.c b/arch/arm64/kvm/dirty_bit.c
> index 6c7a6ef66b5aa..002366337637c 100644
> --- a/arch/arm64/kvm/dirty_bit.c
> +++ b/arch/arm64/kvm/dirty_bit.c
> @@ -50,3 +50,65 @@ void kvm_arm_vcpu_free_hdbss(struct kvm_vcpu *vcpu)

[ ... ]

> +void kvm_flush_hdbss_buffer(struct kvm_vcpu *vcpu)
> +{
> +	int idx, curr_idx;
> +	u64 *hdbss_buf;
> +	struct kvm *kvm = vcpu->kvm;
> +
> +	if (!kvm->arch.enable_hdbss)
> +		return;
> +
> +	curr_idx = HDBSSPROD_IDX(read_sysreg_s(SYS_HDBSSPROD_EL2));

[Severity: High]
Can this safely execute in nVHE mode?

This reads the SYS_HDBSSPROD_EL2 register directly from host context.
In KVM nVHE mode, the host kernel executes at EL1. Accessing an EL2
register from EL1 will trigger an undefined instruction exception and
panic the host.

> +
> +	/* Do nothing if HDBSS buffer is empty or br_el2 is NULL */
> +	if (curr_idx == 0 || vcpu->arch.hdbss.hdbssbr_el2 == 0)
> +		return;
> +
> +	hdbss_buf = page_address(phys_to_page(vcpu->arch.hdbss.base_phys));
> +	if (!hdbss_buf)
> +		return;
> +
> +	guard(write_lock_irqsave)(&vcpu->kvm->mmu_lock);
> +	for (idx = 0; idx < curr_idx; idx++) {
> +		u64 gpa;
> +
> +		gpa = hdbss_buf[idx];
> +		if (!(gpa & HDBSS_ENTRY_VALID))
> +			continue;
> +
> +		gpa &= HDBSS_ENTRY_IPA;
> +		kvm_vcpu_mark_page_dirty(vcpu, gpa >> PAGE_SHIFT);

[Severity: High]
Does this tight loop risk overflowing the dirty ring?

When the KVM dirty ring is enabled, kvm_vcpu_mark_page_dirty() pushes
entries to the per-vCPU ring. The dirty ring signals userspace
cooperatively when it reaches its soft limit.

Because the entire HDBSS buffer is flushed in one tight loop, the soft
limit signal is ignored until the loop finishes. If the number of HDBSS
entries exceeds the remaining free slots before the hard limit, could
the ring push fail and silently drop the dirty pages?

> +	}
> +
> +	/* reset HDBSS index */
> +	write_sysreg_s(0, SYS_HDBSSPROD_EL2);
> +	vcpu->arch.hdbss.hdbssprod_el2 = 0;
> +	isb();

[Severity: Medium]
Is the context synchronization event misplaced here?

The architectural rules state that every write to a control-plane system
register must be followed by an isb() as the very next instruction.

There is a memory store (vcpu->arch.hdbss.hdbssprod_el2 = 0) placed
between the write_sysreg_s() and the isb(). Could this leave the CPU
operating under a stale configuration for several cycles?

> +}
> +
> +int kvm_handle_hdbss_fault(struct kvm_vcpu *vcpu)
> +{
> +	u64 prod;
> +	u64 fsc;
> +
> +	prod = read_sysreg_s(SYS_HDBSSPROD_EL2);

[Severity: High]
Like in kvm_flush_hdbss_buffer() above, does reading SYS_HDBSSPROD_EL2
from the host kernel risk triggering an undefined instruction exception
if the system is running in nVHE mode?

> +	fsc = FIELD_GET(HDBSSPROD_EL2_FSC_MASK, prod);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709104026.2612599-1-zhengtian10@huawei.com?part=5