On Fri, 09 Jun 2023 00:17:34 +0100,
Gavin Shan <gshan@xxxxxxxxxx> wrote:
>
> Hi Marc,
>
> [Cc Andrea/David/Peter Xu]
>
> On 6/9/23 12:31 AM, Marc Zyngier wrote:
> > On Thu, 08 Jun 2023 10:03:48 +0100,
> > Gavin Shan <gshan@xxxxxxxxxx> wrote:
> >>
> >> We run into guest hang in edk2 firmware when KSM is kept as running
> >> on the host. The edk2 firmware is waiting for status 0x80 from QEMU's
> >> pflash device (TYPE_PFLASH_CFI01) during the operation for sector
> >> erasing or buffered write. The status is returned by reading the
> >> memory region of the pflash device and the read request should
> >> have been forwarded to QEMU and emulated by it. Unfortunately, the
> >> read request is covered by an illegal stage2 mapping when the guest
> >> hang issue occurs. The read request is completed with QEMU bypassed and
> >> wrong status is fetched.
> >>
> >> The illegal stage2 mapping is populated due to same page mering by
> >> KSM at (C) even the associated memory slot has been marked as invalid
> >> at (B).
> >>
> >> CPU-A CPU-B
> >> ----- -----
> >> ioctl(kvm_fd, KVM_SET_USER_MEMORY_REGION)
> >> kvm_vm_ioctl_set_memory_region
> >> kvm_set_memory_region
> >> __kvm_set_memory_region
> >> kvm_set_memslot(kvm, old, NULL, KVM_MR_DELETE)
> >> kvm_invalidate_memslot
> >> kvm_copy_memslot
> >> kvm_replace_memslot
> >> kvm_swap_active_memslots (A)
> >> kvm_arch_flush_shadow_memslot (B)
> >> same page merging by KSM
> >> kvm_mmu_notifier_change_pte
> >> kvm_handle_hva_range
> >> __kvm_handle_hva_range (C)
> >>
> >> Fix the issue by skipping the invalid memory slot at (C) to avoid the
> >> illegal stage2 mapping. Without the illegal stage2 mapping, the read
> >> request for the pflash's status is forwarded to QEMU and emulated by
> >> it. The correct pflash's status can be returned from QEMU to break
> >> the infinite wait in edk2 firmware.
> >
> > Huh, nice one :-(.
> >
>
> Yeah, it's a sneaky one :)
>
> >>
> >> Cc: stable@xxxxxxxxxxxxxxx # v5.13+
> >> Fixes: 3039bcc74498 ("KVM: Move x86's MMU notifier memslot walkers to generic code")
> >> Reported-by: Shuai Hu <hshuai@xxxxxxxxxx>
> >> Reported-by: Zhenyu Zhang <zhenyzha@xxxxxxxxxx>
> >> Signed-off-by: Gavin Shan <gshan@xxxxxxxxxx>
> >> ---
> >> virt/kvm/kvm_main.c | 3 +++
> >> 1 file changed, 3 insertions(+)
> >>
> >> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> >> index 479802a892d4..7f81a3a209b6 100644
> >> --- a/virt/kvm/kvm_main.c
> >> +++ b/virt/kvm/kvm_main.c
> >> @@ -598,6 +598,9 @@ static __always_inline int __kvm_handle_hva_range(struct kvm *kvm,
> >> unsigned long hva_start, hva_end;
> >> slot = container_of(node, struct
> >> kvm_memory_slot, hva_node[slots->node_idx]);
> >> + if (slot->flags & KVM_MEMSLOT_INVALID)
> >> + continue;
> >> +
> >> hva_start = max(range->start, slot->userspace_addr);
> >> hva_end = min(range->end, slot->userspace_addr +
> >> (slot->npages << PAGE_SHIFT));
> >
> > I don't immediately see what makes it safer. If we're not holding one
> > of slots_{,arch_}lock in the notifier, we can still race against the
> > update, can't we? I don't think holding the srcu lock helps us here.
> >
[...]
> change_pte() is always surrounded by invalidate_range_start and
> invalidate_range_end(). It means kvm->mn_active_invalidate_count is always
> larger than zero when change_pte() is called. With this condition
> (kvm->mn_active_invalidate_count > 0), The swapping between the inactive
> and active memory slots by kvm_swap_active_memslots() can't be done.
> So there are two cases for one memory slot when change_pte() is called:
> (a) it has been marked as KVM_MEMSLOT_INVALID in the active memory slots
> by kvm_invalidate_memslot(), called before invalidate_range_start();
> (b) the memory slot has been deleted from the active memory slots. We're
> only concerned by (a) when the active memory slots are iterated in
> __kvm_handle_hva_range().
OK, so to sum it up:
- the memslot cannot be swapped while we're walking the active
memslots because kvm->mn_active_invalidate_count is elevated, and
kvm_swap_active_memslots() will busy loop until this has reached 0
again
- holding the srcu read_lock prevents an overlapping memslot update
from being published at the wrong time (synchronize_srcu_expedited()
in kvm_swap_active_memslots())
If the above holds, then I agree the fix looks correct. I'd definitely
want to see some of this rationale captured in the commit message
though.
Thanks,
M.
>
> static void kvm_mmu_notifier_change_pte(...)
> {
> :
> /*
> * .change_pte() must be surrounded by .invalidate_range_{start,end}().
> * If mmu_invalidate_in_progress is zero, then no in-progress
> * invalidations, including this one, found a relevant memslot at
> * start(); rechecking memslots here is unnecessary. Note, a false
> * positive (count elevated by a different invalidation) is sub-optimal
> * but functionally ok.
> */
> WARN_ON_ONCE(!READ_ONCE(kvm->mn_active_invalidate_count));
> if (!READ_ONCE(kvm->mmu_invalidate_in_progress))
> return;
> :
> }
>
>
> The srcu lock in __kvm_handle_hva_range() prevents the swapping of
> the active and inactive memory slots by kvm_swap_active_memslots(). For
> this particular case, it's not relevant because the swapping between
> the inactive and active memory slots has been done for once, before
> invalidate_range_start() is called.
--
Without deviation from the norm, progress is not possible.
