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

[Sean Christopherson <seanjc@google.com>]

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