| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20230928162109.42391b48.alex.williamson@redhat.com>
Date: Thu, 28 Sep 2023 16:21:09 -0600
From: Alex Williamson <alex.williamson@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>, Marc Zyngier <maz@...nel.org>,
Oliver Upton <oliver.upton@...ux.dev>,
Huacai Chen <chenhuacai@...nel.org>,
Michael Ellerman <mpe@...erman.id.au>,
Anup Patel <anup@...infault.org>,
Paul Walmsley <paul.walmsley@...ive.com>,
Palmer Dabbelt <palmer@...belt.com>,
Albert Ou <aou@...s.berkeley.edu>,
Heiko Carstens <hca@...ux.ibm.com>,
Vasily Gorbik <gor@...ux.ibm.com>,
Alexander Gordeev <agordeev@...ux.ibm.com>,
Christian Borntraeger <borntraeger@...ux.ibm.com>,
Janosch Frank <frankja@...ux.ibm.com>,
Claudio Imbrenda <imbrenda@...ux.ibm.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
Peter Zijlstra <peterz@...radead.org>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
Paolo Bonzini <pbonzini@...hat.com>,
Tony Krowiak <akrowiak@...ux.ibm.com>,
Halil Pasic <pasic@...ux.ibm.com>,
Jason Herne <jjherne@...ux.ibm.com>,
Harald Freudenberger <freude@...ux.ibm.com>,
Andy Lutomirski <luto@...nel.org>,
linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.linux.dev,
linux-mips@...r.kernel.org, kvm@...r.kernel.org,
linuxppc-dev@...ts.ozlabs.org, kvm-riscv@...ts.infradead.org,
linux-riscv@...ts.infradead.org, linux-s390@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
Anish Ghulati <aghulati@...gle.com>,
Venkatesh Srinivas <venkateshs@...omium.org>,
Andrew Thornton <andrewth@...gle.com>
Subject: 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 <seanjc@...gle.com> 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 <seanjc@...gle.com>
> ---
> 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 <alex.williamson@...hat.com>
> 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 <linux/fs.h>
> #include <linux/idr.h>
> #include <linux/iommu.h>
> -#include <linux/kvm_host.h>
> #include <linux/list.h>
> #include <linux/miscdevice.h>
> #include <linux/module.h>
> @@ -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(&device->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(&ref->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(&ref->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(&ref->lock);
> ref->kvm = kvm;
> + ref->get_kvm = get_kvm;
> + ref->put_kvm = put_kvm;
> spin_unlock(&ref->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(&group->kvm_ref, kvm);
> + vfio_device_set_kvm(&group->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(&df->kvm_ref, kvm);
> + vfio_device_set_kvm(&df->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 VFIO device is first opened the KVM will be available in
> * device->kvm if one was associated with the file.
> */
> -void vfio_file_set_kvm(struct file *file, struct kvm *kvm)
> +void vfio_file_set_kvm(struct file *file, struct kvm *kvm,
> + bool (*get_kvm)(struct kvm *kvm),
> + void (*put_kvm)(struct kvm *kvm))
> {
> struct vfio_group *group;
>
> group = vfio_group_from_file(file);
> if (group)
> - vfio_group_set_kvm(group, kvm);
> + vfio_group_set_kvm(group, kvm, get_kvm, put_kvm);
>
> if (vfio_device_from_file(file))
> - vfio_device_file_set_kvm(file, kvm);
> + vfio_device_file_set_kvm(file, kvm, get_kvm, put_kvm);
> }
> EXPORT_SYMBOL_GPL(vfio_file_set_kvm);
> #endif
> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> index e80955de266c..35e970e3d3fb 100644
> --- a/include/linux/vfio.h
> +++ b/include/linux/vfio.h
> @@ -312,7 +312,9 @@ static inline bool vfio_file_has_dev(struct file *file, struct vfio_device *devi
> bool vfio_file_is_valid(struct file *file);
> bool vfio_file_enforced_coherent(struct file *file);
> #if IS_ENABLED(CONFIG_KVM)
> -void vfio_file_set_kvm(struct file *file, struct kvm *kvm);
> +void vfio_file_set_kvm(struct file *file, struct kvm *kvm,
> + bool (*get_kvm)(struct kvm *kvm),
> + void (*put_kvm)(struct kvm *kvm));
> #endif
>
> #define VFIO_PIN_PAGES_MAX_ENTRIES (PAGE_SIZE/sizeof(unsigned long))
> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> index ca24ce120906..f14fcbb34bc6 100644
> --- a/virt/kvm/vfio.c
> +++ b/virt/kvm/vfio.c
> @@ -37,13 +37,18 @@ struct kvm_vfio {
>
> static void kvm_vfio_file_set_kvm(struct file *file, struct kvm *kvm)
> {
> - void (*fn)(struct file *file, struct kvm *kvm);
> + void (*fn)(struct file *file, struct kvm *kvm,
> + bool (*get_kvm)(struct kvm *kvm),
> + void (*put_kvm)(struct kvm *kvm));
>
> fn = symbol_get(vfio_file_set_kvm);
> if (!fn)
> return;
>
> - fn(file, kvm);
> + if (kvm)
> + fn(file, kvm, kvm_get_kvm_safe, kvm_put_kvm);
> + else
> + fn(file, kvm, NULL, NULL);
>
> symbol_put(vfio_file_set_kvm);
> }
Powered by blists - more mailing lists