On Sun, Dec 03, 2023, Jason Gunthorpe wrote: > On Fri, Dec 01, 2023 at 04:51:55PM -0800, Sean Christopherson wrote: > > > There's one more wrinkle: this patch is buggy in that it doesn't ensure the > > liveliness > > of KVM-the-module, i.e. nothing prevents userspace from unloading kvm.ko > > while VFIO > > still holds a reference to a kvm structure, and so invoking ->put_kvm() > > could jump > > into freed code. To fix that, KVM would also need to pass along a module > > pointer :-( > > Maybe we should be refcounting the struct file not the struct kvm? > > Then we don't need special helpers and it keeps the module alive correctly.
Huh. It took my brain a while to catch up, but this seems comically obvious in hindsight. I *love* this approach, both conceptually and from a code perspective. Handing VFIO (and any other external entities) a file makes it so that KVM effectively interacts with users via files, regardless of whether the user lives in userspace or the kernel. That makes it easier to reason about the safety of operations, e.g. in addition to ensuring KVM-the-module is pinned, having a file pointer allows KVM to verify that the incoming pointer does indeed represent a VM. Which isn't necessary by any means, but it's a nice sanity check. >From a code perspective, it's far cleaner than manually grabbing module >references, and having only a file pointers makes it a wee bit harder for non-KVM code to poke into KVM, e.g. keeps us honest. Assuming nothing blows up in testing, I'll go this route for v2. Thanks!
