Re: [PATCH v6 03/10] KVM: guest_memfd: Handle kvm_gmem_handle_folio_put() for KVM as a module

[Fuad Tabba <tabba@google.com>]

Hi Ackerley,

On Thu, 13 Mar 2025 at 13:49, Ackerley Tng <ackerleytng@google.com> wrote:
>
> Fuad Tabba <tabba@google.com> writes:
>
> > In some architectures, KVM could be defined as a module. If there is a
> > pending folio_put() while KVM is unloaded, the system could crash. By
> > having a helper check for that and call the function only if it's
> > available, we are able to handle that case more gracefully.
> >
> > Signed-off-by: Fuad Tabba <tabba@google.com>
> >
> > ---
> >
> > This patch could be squashed with the previous one of the maintainers
> > think it would be better.
> > ---
> >  include/linux/kvm_host.h |  5 +----
> >  mm/swap.c                | 20 +++++++++++++++++++-
> >  virt/kvm/guest_memfd.c   |  8 ++++++++
> >  3 files changed, 28 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> > index 7788e3625f6d..3ad0719bfc4f 100644
> > --- a/include/linux/kvm_host.h
> > +++ b/include/linux/kvm_host.h
> > @@ -2572,10 +2572,7 @@ long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu,
> >  #endif
> >
> >  #ifdef CONFIG_KVM_GMEM_SHARED_MEM
> > -static inline void kvm_gmem_handle_folio_put(struct folio *folio)
> > -{
> > -     WARN_ONCE(1, "A placeholder that shouldn't trigger. Work in progress.");
> > -}
> > +void kvm_gmem_handle_folio_put(struct folio *folio);
> >  #endif
> >
> >  #endif
> > diff --git a/mm/swap.c b/mm/swap.c
> > index 241880a46358..27dfd75536c8 100644
> > --- a/mm/swap.c
> > +++ b/mm/swap.c
> > @@ -98,6 +98,24 @@ static void page_cache_release(struct folio *folio)
> >               unlock_page_lruvec_irqrestore(lruvec, flags);
> >  }
> >
> > +#ifdef CONFIG_KVM_GMEM_SHARED_MEM
> > +static void gmem_folio_put(struct folio *folio)
> > +{
> > +#if IS_MODULE(CONFIG_KVM)
> > +     void (*fn)(struct folio *folio);
> > +
> > +     fn = symbol_get(kvm_gmem_handle_folio_put);
> > +     if (WARN_ON_ONCE(!fn))
> > +             return;
> > +
> > +     fn(folio);
> > +     symbol_put(kvm_gmem_handle_folio_put);
> > +#else
> > +     kvm_gmem_handle_folio_put(folio);
> > +#endif
> > +}
> > +#endif
>
> Sorry about the premature sending earlier!
>
> I was thinking about having a static function pointer in mm/swap.c that
> will be filled in when KVM is loaded and cleared when KVM is unloaded.
>
> One benefit I see is that it'll avoid the lookup that symbol_get() does
> on every folio_put(), but some other pinning on KVM would have to be
> done to prevent KVM from being unloaded in the middle of
> kvm_gmem_handle_folio_put() call.
>
> Do you/anyone else see pros/cons either way?

To be honest, I didn't do it that way (or even think of doing it that
way) since that's how vfio does it, e.g.,:

https://elixir.bootlin.com/linux/v6.14-rc6/source/virt/kvm/vfio.c#L38

Like you said, your suggestion would require something like
try_module_get()/put() to ensure that it's not unloaded. Is that
better? I don't know... Maybe someone with more experience could chime
in.

Thanks!
/fuad


> > +
> >  static void free_typed_folio(struct folio *folio)
> >  {
> >       switch (folio_get_type(folio)) {
> > @@ -108,7 +126,7 @@ static void free_typed_folio(struct folio *folio)
> >  #endif
> >  #ifdef CONFIG_KVM_GMEM_SHARED_MEM
> >       case PGTY_guestmem:
> > -             kvm_gmem_handle_folio_put(folio);
> > +             gmem_folio_put(folio);
> >               return;
> >  #endif
> >       default:
> > diff --git a/virt/kvm/guest_memfd.c b/virt/kvm/guest_memfd.c
> > index b2aa6bf24d3a..5fc414becae5 100644
> > --- a/virt/kvm/guest_memfd.c
> > +++ b/virt/kvm/guest_memfd.c
> > @@ -13,6 +13,14 @@ struct kvm_gmem {
> >       struct list_head entry;
> >  };
> >
> > +#ifdef CONFIG_KVM_GMEM_SHARED_MEM
> > +void kvm_gmem_handle_folio_put(struct folio *folio)
> > +{
> > +     WARN_ONCE(1, "A placeholder that shouldn't trigger. Work in progress.");
> > +}
> > +EXPORT_SYMBOL_GPL(kvm_gmem_handle_folio_put);
> > +#endif /* CONFIG_KVM_GMEM_SHARED_MEM */
> > +
> >  /**
> >   * folio_file_pfn - like folio_file_page, but return a pfn.
> >   * @folio: The folio which contains this index.