Re: [PATCH] KVM: x86/mmu: Protect noncoherent DMA zaps with SRCU

On Sun, 2026-08-23 at 03:02 +0800, Chengfeng Ye wrote:
> Protect the noncoherent DMA zap with KVM's SRCU so that memslots and
> their architecture-specific metadata remain alive if the rmap walk
> drops mmu_lock to reschedule.
> 
> The VFIO noncoherent-DMA path invokes
> kvm_arch_register_noncoherent_dma() without holding slots_lock or an
> SRCU read lock. kvm_zap_gfn_range() can then enter
> __walk_slot_rmaps(), which retains pointers to a memslot and its rmap
> while cond_resched_rwlock_write() temporarily drops mmu_lock.

kvm_zap_gfn_range() can invoke 

   slots = __kvm_memslots(kvm, i);

internally (from kvm_rmap_zap_gfn_range()).  AFAICT this itself is enough that
the caller should either grab read lock of KVM's SRCU or slots_lock.

For instance, __kvm_set_or_clear_apicv_inhibit() is already holding KVM's SRCU
read lock when calling kvm_zap_gfn_range() to zap the GFN of guest's APIC page
(which is only 1 page thus won't yield).

> 
> The race looks like this:
> 
>   CPU 0: VFIO coherency update          CPU 1: memslot delete
>   ----------------------------          ---------------------
>   kvm_vfio_set_attr()
>     kvm_arch_register_noncoherent_dma()
>       kvm_zap_gfn_range()
>         __walk_slot_rmaps()
>           iterator.rmap = slot->arch.rmap
>           cond_resched_rwlock_write()
>             drop mmu_lock
> 
>                                         KVM_SET_USER_MEMORY_REGION(DELETE)
>                                           kvm_arch_flush_shadow_memslot()
>                                             zap SPTEs under mmu_lock
>                                           kvm_swap_active_memslots()
>                                             synchronize_srcu_expedited()

Isn't kvm_swap_active_memslots() called _before_
kvm_arch_flush_shadow_memslot()?


>                                           kvm_free_memslot()
>                                             vfree(slot->arch.rmap[i])
>                                             kfree(slot)
> 
>           reacquire mmu_lock
>           slot_rmap_walk_next()
>             read freed iterator.rmap
> 
> The delete path is allowed to free the old memslot because the zap path
> holds no SRCU read lock. synchronize_srcu_expedited() therefore does not
> wait for the rmap walk before kvm_free_memslot() releases the old slot
> and its rmap array. When the zap resumes, slot_rmap_walk_next() reads
> from freed memory.
> 
> KASAN reported:
> 
>   BUG: KASAN: vmalloc-out-of-bounds in
>   slot_rmap_walk_next+0x82/0x1c0
>   Read of size 8 at addr ffffc900005c1008
> 
>   Call Trace:
>    slot_rmap_walk_next+0x82/0x1c0
>    __kvm_rmap_zap_gfn_range+0x17a/0x280
>    kvm_zap_gfn_range+0x2a6/0x6a0
>    kvm_vfio_set_attr+0x576/0x770
>    kvm_device_ioctl+0x1ff/0x3b0
>    __x64_sys_ioctl+0x134/0x1c0
> 
> Hold SRCU across the zap so that memslot deletion waits for the walk to
> finish before freeing the old slot, without changing zap behavior.
> 
> Fixes: 362ff6dca541 ("KVM: x86/mmu: Zap KVM TDP when noncoherent DMA assignment starts/stops")
> Cc: [email protected]
> Signed-off-by: Chengfeng Ye <[email protected]>

Code change LGTM, feel free to add:

Reviewed-by: Kai Huang <[email protected]>

Nit: the short-log uses "x86/mmu" as topic, but maybe "x86" is more appropriate
since the code change is more like at x86 level?

> ---
>  arch/x86/kvm/x86.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 69469bbdc84a..2114553f3159 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -14092,8 +14092,12 @@ static void kvm_noncoherent_dma_assignment_start_or_stop(struct kvm *kvm)
>  	 *
>  	 * If KVM always honors guest PAT, however, there is nothing to do.
>  	 */
> -	if (kvm_check_has_quirk(kvm, KVM_X86_QUIRK_IGNORE_GUEST_PAT))
> +	if (kvm_check_has_quirk(kvm, KVM_X86_QUIRK_IGNORE_GUEST_PAT)) {
> +		int idx = srcu_read_lock(&kvm->srcu);
> +
>  		kvm_zap_gfn_range(kvm, gpa_to_gfn(0), gpa_to_gfn(~0ULL));
> +		srcu_read_unlock(&kvm->srcu, idx);
> +	}
>  }