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 :-)
