Re: [PATCH 05/26] vfio: KVM: Pass get/put helpers from KVM to VFIO, don't do circular lookup
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!
Re: [PATCH 05/26] vfio: KVM: Pass get/put helpers from KVM to VFIO, don't do circular lookup
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. Jason
Re: [PATCH 05/26] vfio: KVM: Pass get/put helpers from KVM to VFIO, don't do circular lookup
On Mon, Sep 18, 2023, Jason Gunthorpe wrote: > On Mon, Sep 18, 2023 at 08:49:57AM -0700, Sean Christopherson wrote: > > On Mon, Sep 18, 2023, Jason Gunthorpe wrote: > > > On Fri, Sep 15, 2023 at 05:30:57PM -0700, Sean Christopherson wrote: > > > > Explicitly pass KVM's get/put helpers to VFIO when attaching a VM to > > > > VFIO instead of having VFIO do a symbol lookup back into KVM. Having > > > > both > > > > KVM and VFIO do symbol lookups increases the overall complexity and > > > > places > > > > an unnecessary dependency on KVM (from VFIO) without adding any value. > > > > > > > > Signed-off-by: Sean Christopherson > > > > --- > > > > drivers/vfio/vfio.h | 2 ++ > > > > drivers/vfio/vfio_main.c | 74 +++- > > > > include/linux/vfio.h | 4 ++- > > > > virt/kvm/vfio.c | 9 +++-- > > > > 4 files changed, 47 insertions(+), 42 deletions(-) > > > > > > I don't mind this, but Christoph had disliked my prior attempt to do > > > this with function pointers.. > > > > > > The get can be inlined, IIRC, what about putting a pointer to the put > > > inside the kvm struct? > > > > That wouldn't allow us to achieve our goal, which is to hide the details of > > "struct kvm" from VFIO (and the rest of the kernel). > > > What's the objection to handing VFIO a function pointer? > > Hmm, looks like it was this thread: > > > https://lore.kernel.org/r/0-v1-33906a626da1+16b0-vfio_kvm_no_group_...@nvidia.com > > Your rational looks a little better to me. > > > > The the normal kvm get/put don't have to exported symbols at all? > > > > The export of kvm_get_kvm_safe() can go away (I forgot to do that in this > > series), > > but kvm_get_kvm() will hang around as it's needed by KVM sub-modules (PPC > > and x86), > > KVMGT (x86), and drivers/s390/crypto/vfio_ap_ops.c (no idea what to call > > that beast). > > My thought would be to keep it as an inline, there should be some way > to do that without breaking your desire to hide the bulk of the kvm > struct content. Like put the refcount as the first element in the > struct and just don't ifdef it away?. That doesn't work because of the need to invoke kvm_destroy_vm() when the last reference is put, i.e. all of kvm_destroy_vm() would need to be inlined (LOL) or VFIO would need to do a symbol lookup on kvm_destroy_vm(), which puts back us at square one. 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 :-( One thought would be to have vac.ko (tentative name), which is the "base" module that will hold the KVM/virtualization bits that need to be singletons, i.e. can't be per-KVM, provide the symbols needed for VFIO to manage references. But that just ends up moving the module reference trickiness into VAC+KVM, e.g. vac.ko would still need to be handed a function pointer in order to call back into the correct kvm.ko code. Hrm, but I suspect the vac.ko <=> kvm.ko interactions will need to deal with module shenanigans anyways, and the shenanigans would only affect crazy people like us that actually want multiple KVM modules. I'll plan on going that route. The very worst case scenario is that it just punts this conversation down to a possibile future. Dropping this patch and the previous prep patch won't meaningful affect the goals of this series, as only kvm_get_kvm_safe() and kvm_get_kvm() would need to be exposed outside of #ifdef __KVM__. Then we can figure out what to do with them if/when the whole multi-KVM thing comes along.
Re: [PATCH 05/26] vfio: KVM: Pass get/put helpers from KVM to VFIO, don't do circular lookup
On Fri, 15 Sep 2023 17:30:57 -0700
Sean Christopherson wrote:
> Explicitly pass KVM's get/put helpers to VFIO when attaching a VM to
> VFIO instead of having VFIO do a symbol lookup back into KVM. Having both
> KVM and VFIO do symbol lookups increases the overall complexity and places
> an unnecessary dependency on KVM (from VFIO) without adding any value.
>
> Signed-off-by: Sean Christopherson
> ---
> drivers/vfio/vfio.h | 2 ++
> drivers/vfio/vfio_main.c | 74 +++-
> include/linux/vfio.h | 4 ++-
> virt/kvm/vfio.c | 9 +++--
> 4 files changed, 47 insertions(+), 42 deletions(-)
Reviewed-by: Alex Williamson
> diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> index a1f741365075..eec51c7ee822 100644
> --- a/drivers/vfio/vfio.h
> +++ b/drivers/vfio/vfio.h
> @@ -19,6 +19,8 @@ struct vfio_container;
>
> struct vfio_kvm_reference {
> struct kvm *kvm;
> + bool(*get_kvm)(struct kvm *kvm);
> + void(*put_kvm)(struct kvm *kvm);
> spinlock_t lock;
> };
>
> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> index e77e8c6aae2f..1f58ab6dbcd2 100644
> --- a/drivers/vfio/vfio_main.c
> +++ b/drivers/vfio/vfio_main.c
> @@ -16,7 +16,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -1306,38 +1305,22 @@ EXPORT_SYMBOL_GPL(vfio_file_enforced_coherent);
> void vfio_device_get_kvm_safe(struct vfio_device *device,
> struct vfio_kvm_reference *ref)
> {
> - void (*pfn)(struct kvm *kvm);
> - bool (*fn)(struct kvm *kvm);
> - bool ret;
> -
> lockdep_assert_held(>dev_set->lock);
>
> + /*
> + * Note! The "kvm" and "put_kvm" pointers *must* be transferred to the
> + * device so that the device can put its reference to KVM. KVM can
> + * invoke vfio_device_set_kvm() to detach from VFIO, i.e. nullify all
> + * pointers in @ref, even if a device holds a reference to KVM! That
> + * also means that detaching KVM from VFIO only prevents "new" devices
> + * from using KVM, it doesn't invalidate KVM references in existing
> + * devices.
> + */
> spin_lock(>lock);
> -
> - if (!ref->kvm)
> - goto out;
> -
> - pfn = symbol_get(kvm_put_kvm);
> - if (WARN_ON(!pfn))
> - goto out;
> -
> - fn = symbol_get(kvm_get_kvm_safe);
> - if (WARN_ON(!fn)) {
> - symbol_put(kvm_put_kvm);
> - goto out;
> + if (ref->kvm && ref->get_kvm(ref->kvm)) {
> + device->kvm = ref->kvm;
> + device->put_kvm = ref->put_kvm;
> }
> -
> - ret = fn(ref->kvm);
> - symbol_put(kvm_get_kvm_safe);
> - if (!ret) {
> - symbol_put(kvm_put_kvm);
> - goto out;
> - }
> -
> - device->put_kvm = pfn;
> - device->kvm = ref->kvm;
> -
> -out:
> spin_unlock(>lock);
> }
>
> @@ -1353,28 +1336,37 @@ void vfio_device_put_kvm(struct vfio_device *device)
>
> device->put_kvm(device->kvm);
> device->put_kvm = NULL;
> - symbol_put(kvm_put_kvm);
> -
> clear:
> device->kvm = NULL;
> }
>
> static void vfio_device_set_kvm(struct vfio_kvm_reference *ref,
> - struct kvm *kvm)
> + struct kvm *kvm,
> + bool (*get_kvm)(struct kvm *kvm),
> + void (*put_kvm)(struct kvm *kvm))
> {
> + if (WARN_ON_ONCE(kvm && (!get_kvm || !put_kvm)))
> + return;
> +
> spin_lock(>lock);
> ref->kvm = kvm;
> + ref->get_kvm = get_kvm;
> + ref->put_kvm = put_kvm;
> spin_unlock(>lock);
> }
>
> -static void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
> +static void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm,
> +bool (*get_kvm)(struct kvm *kvm),
> +void (*put_kvm)(struct kvm *kvm))
> {
> #if IS_ENABLED(CONFIG_VFIO_GROUP)
> - vfio_device_set_kvm(>kvm_ref, kvm);
> + vfio_device_set_kvm(>kvm_ref, kvm, get_kvm, put_kvm);
> #endif
> }
>
> -static void vfio_device_file_set_kvm(struct file *file, struct kvm *kvm)
> +static void vfio_device_file_set_kvm(struct file *file, struct kvm *kvm,
> + bool (*get_kvm)(struct kvm *kvm),
> + void (*put_kvm)(struct kvm *kvm))
> {
> struct vfio_device_file *df = file->private_data;
>
> @@ -1383,27 +1375,31 @@ static void vfio_device_file_set_kvm(struct file
> *file, struct kvm *kvm)
>* be propagated to vfio_device::kvm when the file is bound to
>* iommufd successfully in the vfio device cdev path.
>*/
> - vfio_device_set_kvm(>kvm_ref, kvm);
> + vfio_device_set_kvm(>kvm_ref, kvm, get_kvm, put_kvm);
> }
>
Re: [PATCH 05/26] vfio: KVM: Pass get/put helpers from KVM to VFIO, don't do circular lookup
On Mon, Sep 18, 2023 at 08:49:57AM -0700, Sean Christopherson wrote: > On Mon, Sep 18, 2023, Jason Gunthorpe wrote: > > On Fri, Sep 15, 2023 at 05:30:57PM -0700, Sean Christopherson wrote: > > > Explicitly pass KVM's get/put helpers to VFIO when attaching a VM to > > > VFIO instead of having VFIO do a symbol lookup back into KVM. Having both > > > KVM and VFIO do symbol lookups increases the overall complexity and places > > > an unnecessary dependency on KVM (from VFIO) without adding any value. > > > > > > Signed-off-by: Sean Christopherson > > > --- > > > drivers/vfio/vfio.h | 2 ++ > > > drivers/vfio/vfio_main.c | 74 +++- > > > include/linux/vfio.h | 4 ++- > > > virt/kvm/vfio.c | 9 +++-- > > > 4 files changed, 47 insertions(+), 42 deletions(-) > > > > I don't mind this, but Christoph had disliked my prior attempt to do > > this with function pointers.. > > > > The get can be inlined, IIRC, what about putting a pointer to the put > > inside the kvm struct? > > That wouldn't allow us to achieve our goal, which is to hide the details of > "struct kvm" from VFIO (and the rest of the kernel). > What's the objection to handing VFIO a function pointer? Hmm, looks like it was this thread: https://lore.kernel.org/r/0-v1-33906a626da1+16b0-vfio_kvm_no_group_...@nvidia.com Your rational looks a little better to me. > > The the normal kvm get/put don't have to exported symbols at all? > > The export of kvm_get_kvm_safe() can go away (I forgot to do that in this > series), > but kvm_get_kvm() will hang around as it's needed by KVM sub-modules (PPC and > x86), > KVMGT (x86), and drivers/s390/crypto/vfio_ap_ops.c (no idea what to call that > beast). My thought would be to keep it as an inline, there should be some way to do that without breaking your desire to hide the bulk of the kvm struct content. Like put the refcount as the first element in the struct and just don't ifdef it away?. Jason
Re: [PATCH 05/26] vfio: KVM: Pass get/put helpers from KVM to VFIO, don't do circular lookup
On Mon, Sep 18, 2023, Jason Gunthorpe wrote: > On Fri, Sep 15, 2023 at 05:30:57PM -0700, Sean Christopherson wrote: > > Explicitly pass KVM's get/put helpers to VFIO when attaching a VM to > > VFIO instead of having VFIO do a symbol lookup back into KVM. Having both > > KVM and VFIO do symbol lookups increases the overall complexity and places > > an unnecessary dependency on KVM (from VFIO) without adding any value. > > > > Signed-off-by: Sean Christopherson > > --- > > drivers/vfio/vfio.h | 2 ++ > > drivers/vfio/vfio_main.c | 74 +++- > > include/linux/vfio.h | 4 ++- > > virt/kvm/vfio.c | 9 +++-- > > 4 files changed, 47 insertions(+), 42 deletions(-) > > I don't mind this, but Christoph had disliked my prior attempt to do > this with function pointers.. > > The get can be inlined, IIRC, what about putting a pointer to the put > inside the kvm struct? That wouldn't allow us to achieve our goal, which is to hide the details of "struct kvm" from VFIO (and the rest of the kernel). What's the objection to handing VFIO a function pointer? > The the normal kvm get/put don't have to exported symbols at all? The export of kvm_get_kvm_safe() can go away (I forgot to do that in this series), but kvm_get_kvm() will hang around as it's needed by KVM sub-modules (PPC and x86), KVMGT (x86), and drivers/s390/crypto/vfio_ap_ops.c (no idea what to call that beast). Gah, KVMGT doesn't actually need to call get/put, that can be handled by kvm_page_track_register_notifier(). I am planning on making exports for sub-modules conditional on there actually being submodules, so that's 2 of the 3 gone, but tackling the s390 crypto driver is an entirely different story.
Re: [PATCH 05/26] vfio: KVM: Pass get/put helpers from KVM to VFIO, don't do circular lookup
On Fri, Sep 15, 2023 at 05:30:57PM -0700, Sean Christopherson wrote: > Explicitly pass KVM's get/put helpers to VFIO when attaching a VM to > VFIO instead of having VFIO do a symbol lookup back into KVM. Having both > KVM and VFIO do symbol lookups increases the overall complexity and places > an unnecessary dependency on KVM (from VFIO) without adding any value. > > Signed-off-by: Sean Christopherson > --- > drivers/vfio/vfio.h | 2 ++ > drivers/vfio/vfio_main.c | 74 +++- > include/linux/vfio.h | 4 ++- > virt/kvm/vfio.c | 9 +++-- > 4 files changed, 47 insertions(+), 42 deletions(-) I don't mind this, but Christoph had disliked my prior attempt to do this with function pointers.. The get can be inlined, IIRC, what about putting a pointer to the put inside the kvm struct? The the normal kvm get/put don't have to exported symbols at all? Jason
[PATCH 05/26] vfio: KVM: Pass get/put helpers from KVM to VFIO, don't do circular lookup
Explicitly pass KVM's get/put helpers to VFIO when attaching a VM to
VFIO instead of having VFIO do a symbol lookup back into KVM. Having both
KVM and VFIO do symbol lookups increases the overall complexity and places
an unnecessary dependency on KVM (from VFIO) without adding any value.
Signed-off-by: Sean Christopherson
---
drivers/vfio/vfio.h | 2 ++
drivers/vfio/vfio_main.c | 74 +++-
include/linux/vfio.h | 4 ++-
virt/kvm/vfio.c | 9 +++--
4 files changed, 47 insertions(+), 42 deletions(-)
diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
index a1f741365075..eec51c7ee822 100644
--- a/drivers/vfio/vfio.h
+++ b/drivers/vfio/vfio.h
@@ -19,6 +19,8 @@ struct vfio_container;
struct vfio_kvm_reference {
struct kvm *kvm;
+ bool(*get_kvm)(struct kvm *kvm);
+ void(*put_kvm)(struct kvm *kvm);
spinlock_t lock;
};
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
index e77e8c6aae2f..1f58ab6dbcd2 100644
--- a/drivers/vfio/vfio_main.c
+++ b/drivers/vfio/vfio_main.c
@@ -16,7 +16,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -1306,38 +1305,22 @@ EXPORT_SYMBOL_GPL(vfio_file_enforced_coherent);
void vfio_device_get_kvm_safe(struct vfio_device *device,
struct vfio_kvm_reference *ref)
{
- void (*pfn)(struct kvm *kvm);
- bool (*fn)(struct kvm *kvm);
- bool ret;
-
lockdep_assert_held(>dev_set->lock);
+ /*
+* Note! The "kvm" and "put_kvm" pointers *must* be transferred to the
+* device so that the device can put its reference to KVM. KVM can
+* invoke vfio_device_set_kvm() to detach from VFIO, i.e. nullify all
+* pointers in @ref, even if a device holds a reference to KVM! That
+* also means that detaching KVM from VFIO only prevents "new" devices
+* from using KVM, it doesn't invalidate KVM references in existing
+* devices.
+*/
spin_lock(>lock);
-
- if (!ref->kvm)
- goto out;
-
- pfn = symbol_get(kvm_put_kvm);
- if (WARN_ON(!pfn))
- goto out;
-
- fn = symbol_get(kvm_get_kvm_safe);
- if (WARN_ON(!fn)) {
- symbol_put(kvm_put_kvm);
- goto out;
+ if (ref->kvm && ref->get_kvm(ref->kvm)) {
+ device->kvm = ref->kvm;
+ device->put_kvm = ref->put_kvm;
}
-
- ret = fn(ref->kvm);
- symbol_put(kvm_get_kvm_safe);
- if (!ret) {
- symbol_put(kvm_put_kvm);
- goto out;
- }
-
- device->put_kvm = pfn;
- device->kvm = ref->kvm;
-
-out:
spin_unlock(>lock);
}
@@ -1353,28 +1336,37 @@ void vfio_device_put_kvm(struct vfio_device *device)
device->put_kvm(device->kvm);
device->put_kvm = NULL;
- symbol_put(kvm_put_kvm);
-
clear:
device->kvm = NULL;
}
static void vfio_device_set_kvm(struct vfio_kvm_reference *ref,
- struct kvm *kvm)
+ struct kvm *kvm,
+ bool (*get_kvm)(struct kvm *kvm),
+ void (*put_kvm)(struct kvm *kvm))
{
+ if (WARN_ON_ONCE(kvm && (!get_kvm || !put_kvm)))
+ return;
+
spin_lock(>lock);
ref->kvm = kvm;
+ ref->get_kvm = get_kvm;
+ ref->put_kvm = put_kvm;
spin_unlock(>lock);
}
-static void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
+static void vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm,
+ bool (*get_kvm)(struct kvm *kvm),
+ void (*put_kvm)(struct kvm *kvm))
{
#if IS_ENABLED(CONFIG_VFIO_GROUP)
- vfio_device_set_kvm(>kvm_ref, kvm);
+ vfio_device_set_kvm(>kvm_ref, kvm, get_kvm, put_kvm);
#endif
}
-static void vfio_device_file_set_kvm(struct file *file, struct kvm *kvm)
+static void vfio_device_file_set_kvm(struct file *file, struct kvm *kvm,
+bool (*get_kvm)(struct kvm *kvm),
+void (*put_kvm)(struct kvm *kvm))
{
struct vfio_device_file *df = file->private_data;
@@ -1383,27 +1375,31 @@ static void vfio_device_file_set_kvm(struct file *file,
struct kvm *kvm)
* be propagated to vfio_device::kvm when the file is bound to
* iommufd successfully in the vfio device cdev path.
*/
- vfio_device_set_kvm(>kvm_ref, kvm);
+ vfio_device_set_kvm(>kvm_ref, kvm, get_kvm, put_kvm);
}
/**
* vfio_file_set_kvm - Link a kvm with VFIO drivers
* @file: VFIO group file or VFIO device file
* @kvm: KVM to link
+ * @get_kvm: Callback to get a reference to @kvm
+ * @put_kvm: Callback to put a reference to @kvm
*
* When a
