Re: [PATCH] KVM: pfncache: track HVA invalidations for HVA-based caches

[Jeongjun Park]

Hi,

Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Wed, May 20, 2026, Jeongjun Park wrote:
> > HVA-based gfn_to_pfn caches are not necessarily backed by a KVM memslot.
> > When an MMU notifier invalidation targets such an HVA, KVM's global
[....]
> >
> > Reported-by: syzbot+0948c82180d475ad24e2@xxxxxxxxxxxxxxxxxxxxxxxxx
> > Closes: https://lore.kernel.org/all/6a0c5f2c.a00a0220.2c7954.0000.GAE@xxxxxxxxxx
>
> Given that there's no reproducer, how did you test this?
>

For some unknown reason, repro was not generated in syzbot, so I analyzed
the KASAN logs separately to predict a sequence similar to the race
sequence you suggested, and then wrote the repro code myself to test it.

> > Fixes: b9220d32799a ("KVM: x86/xen: allow shared_info to be mapped by fixed HVA")
> > Signed-off-by: Jeongjun Park <aha310510@xxxxxxxxx>
> > ---
> >  include/linux/kvm_types.h |  1 +
> >  virt/kvm/pfncache.c       | 42 ++++++++++++++++++++++++++++++++-------
> >  2 files changed, 36 insertions(+), 7 deletions(-)
> >
> > diff --git a/include/linux/kvm_types.h b/include/linux/kvm_types.h
> > index a568d8e6f4e8..ff3b8aa73561 100644
[....]
> >                       /*
> > @@ -45,9 +62,11 @@ void gfn_to_pfn_cache_invalidate_start(struct kvm *kvm, unsigned long start,
> >                        */
> >
> >                       write_lock_irq(&gpc->lock);
> > -                     if (gpc->valid && !is_error_noslot_pfn(gpc->pfn) &&
>
> Unless I'm forgetting/missing something, it should be impossible for gpc->pfn to
> be garbage if gpc->valid is true.  As a follow-up patch, turn that into a
> WARN_ON_ONCE()?  Assuming I'm not missing something...
>

I agree. As far as I know, if gpc->valid is true, !is_error_noslot_pfn()
cannot be false. Since this check exists in the existing code, I
considered keeping it in gpc_hva_is_valid(), but the method you suggested
seems more accurate, so I think it would be better to just remove
!is_error_noslot_pfn().

> > -                         gpc->uhva >= start && gpc->uhva < end)
> > +                     if (gpc_should_invalidate(gpc, start, end)) {
> > +                             if (kvm_gpc_is_hva_active(gpc))
[....]
>
> Oh!  But we can simply piggyback mn_invalidate_lock and use a per-VM GPC sequence
> counter, then the end() path doesn't need to re-walk GPCs.  That will regress
> GPCs in the presense of unrelated mmu_notifier events, but we already know that's
> a lurking problem, and we know how to fix it[*] (I think).
>
> There's a "sleeping while atomic" problem that will occur on RT if/when GPCs take
> mn_invalidate_lock when refreshing the PFN, thanks to IRQs being disabled, but
> that's not a problem until we implement the range-based optimization, because
> until then, the reader side can be lockless.
>
> [*] https://lore.kernel.org/all/Zw8DINUkJJKDByXE@xxxxxxxxxx
>
> Not yet tested, and I think I'd want to land this at the same time as the
> range-based optimzation, to avoid regressing "normal" GPC usage (only AWS uses
> the HVA-based Xen API, so getting a fix to LTS kernels isn't a priority, AFAIK),
> but this?
>

The approach you suggested seems most appropriate for resolving this bug.
Of course, as you mentioned, modifying it this way would cause a regress
when unrelated mmu_notifier events occur; however, since resolving this
issue would likely require a significant number of patches, this method
appears more suitable for the time being.

I'll send a v2 patch reflecting these changes soon.

> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 0bdfa3699352..d07e3cbff983 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -854,6 +854,8 @@ struct kvm {
>         gfn_t mmu_invalidate_range_start;
>         gfn_t mmu_invalidate_range_end;
>
> +       unsigned long gpc_invalidate_seq;
[....]

Regards,
Jeongjun Park