Hi Sean and Vlastimil,
On Mon, 17 Mar 2025 at 14:39, Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> On Mon, Mar 17, 2025, Vlastimil Babka wrote:
> > On 3/13/25 14:49, Ackerley Tng wrote:
> > >> +#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
> >
> > Yeah, this is not great. The vfio code isn't setting a good example to follow :(
>
> +1000
>
> I haven't been following guest_memfd development, so I've no idea what the context
> of this patch is, but...
>
> NAK to any approach that requires symbol_get(). Not only is it beyond gross,
> it's also broken on x86 as it fails to pin the vendor module, i.e. kvm-amd.ko or
> kvm-intel.ko.
>
> > > 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.
> >
> > Isn't there some "natural" dependency between things such that at the point
> > the KVM module is able to unload itself, no guest_memfd areas should be
> > existing anymore at that point, and thus also not any pages that would use
> > this callback should exist?
>
> Yes. File-backed VMAs hold a reference to the file (e.g. see get_file() usage
> in vma.c), and keeping the guest_memfd file alive in turn prevents kvm.ko from
> being unloaded.
>
> The "magic" is this bit of code in kvm_gmem_init():
>
> kvm_gmem_fops.owner = module;
>
> The fops->owner pointer is then processed by the try_get_module() call in
> __anon_inode_getfile() to obtain a reference to the module which owns the fops.
> The module reference won't be put until the file is fully closed/released; see
> __fput() => fops_put().
>
> On x86, that pins not only kvm.ko, but also the vendor module, because the
> @module passed to kvm_gmem_init() points at the vendor module, not at kvm.ko.
>
> If that's not working, y'all broke something :-)
Thank you for your feedback and for clarifying things. You're right,
with a reference to the module held, no one should be able to unload
it as long as there are in-flight references, no stragglers.
Nothing is broken. Will fix this on the respin.
Cheers,
/fuad
