Re: [PATCH] vfio: Use file-based reference counting for KVM

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

Pre-existing issues:
- [High] Missing SRCU read lock when iterating over KVM memslots in `kvm_enable_external_write_tracking`.
--

--- Patch [1]: [PATCH] vfio: Use file-based reference counting for KVM ---
commit 6e2d7f7af50df153e78728c6ab103209dafd02a4
Author: Steffen Eiden <[email protected]>
Subject: vfio: Use file-based reference counting for KVM

This commit replaces manual module reference counting with file-based
reference counting for KVM integration in VFIO. It passes struct file
pointers instead of struct kvm pointers throughout the interface to
leverage the kernel's existing file reference counting mechanism.

Link: https://lore.kernel.org/all/[email protected]/
Link: https://lore.kernel.org/all/[email protected]

> diff --git a/arch/x86/kvm/mmu/page_track.c b/arch/x86/kvm/mmu/page_track.c
> index 7e8195a311bb0..98f133e4f0b32 100644
> --- a/arch/x86/kvm/mmu/page_track.c
> +++ b/arch/x86/kvm/mmu/page_track.c
> @@ -237,10 +237,11 @@ static int kvm_enable_external_write_tracking(struct kvm *kvm)

[Severity: High]
This is a pre-existing issue, but does kvm_enable_external_write_tracking()
need to hold srcu_read_lock(&kvm->srcu) when iterating over the memslots
array?

Looking at the function, it retrieves and iterates over the memslots array
while only holding the slots_arch_lock mutex:

mutex_lock(&kvm->slots_arch_lock);
...
for (i = 0; i < kvm_arch_nr_memslot_as_ids(kvm); i++) {
    slots = __kvm_memslots(kvm, i);
    kvm_for_each_memslot(slot, bkt, slots) {
        r = kvm_page_track_write_tracking_alloc(slot);
        if (r)
            goto out_unlock;
    }
}

Without holding srcu_read_lock(&kvm->srcu) or the writer-side slots_lock,
could a concurrent memslot update swap and free the memslots array while
the loop is traversing it, causing a use-after-free?

>   * register the notifier so that event interception for the tracked guest
>   * pages can be received.
>   */
> -int kvm_page_track_register_notifier(struct kvm *kvm,
> +int kvm_page_track_register_notifier(struct file *file,
>  				     struct kvm_page_track_notifier_node *n)

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1