From f81a6d12bf8b262f4c8ce5e856a4d399d97612ee Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:20:18 -0800 Subject: [PATCH 1/5] KVM: Open code kvm_set_memory_region() into its sole caller (ioctl() API) Open code kvm_set_memory_region() into its sole caller in preparation for adding a dedicated API for setting internal memslots. Oppurtunistically use the fancy new guard(mutex) to avoid a local 'r' variable. Cc: Tao Su Reviewed-by: Xiaoyao Li Reviewed-by: Claudio Imbrenda Acked-by: Christoph Schlameuss Link: https://lore.kernel.org/r/20250111002022.1230573-2-seanjc@google.com Signed-off-by: Sean Christopherson --- include/linux/kvm_host.h | 2 -- virt/kvm/kvm_main.c | 15 ++------------- 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 401439bb21e3..7443de24b1d9 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1192,8 +1192,6 @@ enum kvm_mr_change { KVM_MR_FLAGS_ONLY, }; -int kvm_set_memory_region(struct kvm *kvm, - const struct kvm_userspace_memory_region2 *mem); int __kvm_set_memory_region(struct kvm *kvm, const struct kvm_userspace_memory_region2 *mem); void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot); diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index de2c11dae231..eb3d0a385077 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2058,25 +2058,14 @@ int __kvm_set_memory_region(struct kvm *kvm, } EXPORT_SYMBOL_GPL(__kvm_set_memory_region); -int kvm_set_memory_region(struct kvm *kvm, - const struct kvm_userspace_memory_region2 *mem) -{ - int r; - - mutex_lock(&kvm->slots_lock); - r = __kvm_set_memory_region(kvm, mem); - mutex_unlock(&kvm->slots_lock); - return r; -} -EXPORT_SYMBOL_GPL(kvm_set_memory_region); - static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm, struct kvm_userspace_memory_region2 *mem) { if ((u16)mem->slot >= KVM_USER_MEM_SLOTS) return -EINVAL; - return kvm_set_memory_region(kvm, mem); + guard(mutex)(&kvm->slots_lock); + return __kvm_set_memory_region(kvm, mem); } #ifndef CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT From d131f0042f466d558f7b75af5e100b62a91a5803 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:20:19 -0800 Subject: [PATCH 2/5] KVM: Assert slots_lock is held when setting memory regions Add proper lockdep assertions in __kvm_set_memory_region() and __x86_set_memory_region() instead of relying comments. Opportunistically delete __kvm_set_memory_region()'s entire function comment as the API doesn't allocate memory or select a gfn, and the "mostly for framebuffers" comment hasn't been true for a very long time. Cc: Tao Su Reviewed-by: Xiaoyao Li Reviewed-by: Claudio Imbrenda Acked-by: Christoph Schlameuss Link: https://lore.kernel.org/r/20250111002022.1230573-3-seanjc@google.com Signed-off-by: Sean Christopherson --- arch/x86/kvm/x86.c | 3 ++- virt/kvm/kvm_main.c | 10 ++-------- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 1b04092ec76a..a861287a67bd 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -12793,7 +12793,8 @@ void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, struct kvm_memslots *slots = kvm_memslots(kvm); struct kvm_memory_slot *slot; - /* Called with kvm->slots_lock held. */ + lockdep_assert_held(&kvm->slots_lock); + if (WARN_ON(id >= KVM_MEM_SLOTS_NUM)) return ERR_PTR_USR(-EINVAL); diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index eb3d0a385077..7d25b50cb298 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1926,14 +1926,6 @@ static bool kvm_check_memslot_overlap(struct kvm_memslots *slots, int id, return false; } -/* - * Allocate some memory and give it an address in the guest physical address - * space. - * - * Discontiguous memory is allowed, mostly for framebuffers. - * - * Must be called holding kvm->slots_lock for write. - */ int __kvm_set_memory_region(struct kvm *kvm, const struct kvm_userspace_memory_region2 *mem) { @@ -1945,6 +1937,8 @@ int __kvm_set_memory_region(struct kvm *kvm, int as_id, id; int r; + lockdep_assert_held(&kvm->slots_lock); + r = check_memory_region_flags(kvm, mem); if (r) return r; From 156bffdb2b49fc0c869bf160a57378886f5fa92d Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:20:20 -0800 Subject: [PATCH 3/5] KVM: Add a dedicated API for setting KVM-internal memslots Add a dedicated API for setting internal memslots, and have it explicitly disallow setting userspace memslots. Setting a userspace memslots without a direct command from userspace would result in all manner of issues. No functional change intended. Cc: Tao Su Cc: Claudio Imbrenda Cc: Christian Borntraeger Reviewed-by: Xiaoyao Li Reviewed-by: Claudio Imbrenda Acked-by: Christoph Schlameuss Link: https://lore.kernel.org/r/20250111002022.1230573-4-seanjc@google.com Signed-off-by: Sean Christopherson --- arch/x86/kvm/x86.c | 2 +- include/linux/kvm_host.h | 4 ++-- virt/kvm/kvm_main.c | 15 ++++++++++++--- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index a861287a67bd..36b5d06e3904 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -12827,7 +12827,7 @@ void __user * __x86_set_memory_region(struct kvm *kvm, int id, gpa_t gpa, m.guest_phys_addr = gpa; m.userspace_addr = hva; m.memory_size = size; - r = __kvm_set_memory_region(kvm, &m); + r = kvm_set_internal_memslot(kvm, &m); if (r < 0) return ERR_PTR_USR(r); } diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 7443de24b1d9..8707d25a2e5b 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1192,8 +1192,8 @@ enum kvm_mr_change { KVM_MR_FLAGS_ONLY, }; -int __kvm_set_memory_region(struct kvm *kvm, - const struct kvm_userspace_memory_region2 *mem); +int kvm_set_internal_memslot(struct kvm *kvm, + const struct kvm_userspace_memory_region2 *mem); void kvm_arch_free_memslot(struct kvm *kvm, struct kvm_memory_slot *slot); void kvm_arch_memslots_updated(struct kvm *kvm, u64 gen); int kvm_arch_prepare_memory_region(struct kvm *kvm, diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 7d25b50cb298..e1be2e4e6c9f 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1926,8 +1926,8 @@ static bool kvm_check_memslot_overlap(struct kvm_memslots *slots, int id, return false; } -int __kvm_set_memory_region(struct kvm *kvm, - const struct kvm_userspace_memory_region2 *mem) +static int __kvm_set_memory_region(struct kvm *kvm, + const struct kvm_userspace_memory_region2 *mem) { struct kvm_memory_slot *old, *new; struct kvm_memslots *slots; @@ -2050,7 +2050,16 @@ int __kvm_set_memory_region(struct kvm *kvm, kfree(new); return r; } -EXPORT_SYMBOL_GPL(__kvm_set_memory_region); + +int kvm_set_internal_memslot(struct kvm *kvm, + const struct kvm_userspace_memory_region2 *mem) +{ + if (WARN_ON_ONCE(mem->slot < KVM_USER_MEM_SLOTS)) + return -EINVAL; + + return __kvm_set_memory_region(kvm, mem); +} +EXPORT_SYMBOL_GPL(kvm_set_internal_memslot); static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm, struct kvm_userspace_memory_region2 *mem) From 344315e93dbc9c4733d5c9fd605ecfc46ef97180 Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:20:21 -0800 Subject: [PATCH 4/5] KVM: x86: Drop double-underscores from __kvm_set_memory_region() Now that there's no outer wrapper for __kvm_set_memory_region() and it's static, drop its double-underscore prefix. No functional change intended. Cc: Tao Su Reviewed-by: Xiaoyao Li Reviewed-by: Claudio Imbrenda Acked-by: Christoph Schlameuss Link: https://lore.kernel.org/r/20250111002022.1230573-5-seanjc@google.com Signed-off-by: Sean Christopherson --- arch/x86/kvm/x86.c | 2 +- include/linux/kvm_host.h | 2 +- virt/kvm/kvm_main.c | 8 ++++---- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 36b5d06e3904..82f389e3910d 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -12928,7 +12928,7 @@ static int kvm_alloc_memslot_metadata(struct kvm *kvm, /* * Clear out the previous array pointers for the KVM_MR_MOVE case. The - * old arrays will be freed by __kvm_set_memory_region() if installing + * old arrays will be freed by kvm_set_memory_region() if installing * the new memslot is successful. */ memset(&slot->arch, 0, sizeof(slot->arch)); diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index 8707d25a2e5b..dcb59d6e8acb 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -1183,7 +1183,7 @@ struct kvm_memory_slot *kvm_vcpu_gfn_to_memslot(struct kvm_vcpu *vcpu, gfn_t gfn * -- just change its flags * * Since flags can be changed by some of these operations, the following - * differentiation is the best we can do for __kvm_set_memory_region(): + * differentiation is the best we can do for kvm_set_memory_region(): */ enum kvm_mr_change { KVM_MR_CREATE, diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index e1be2e4e6c9f..ecd4a66b22f3 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1926,8 +1926,8 @@ static bool kvm_check_memslot_overlap(struct kvm_memslots *slots, int id, return false; } -static int __kvm_set_memory_region(struct kvm *kvm, - const struct kvm_userspace_memory_region2 *mem) +static int kvm_set_memory_region(struct kvm *kvm, + const struct kvm_userspace_memory_region2 *mem) { struct kvm_memory_slot *old, *new; struct kvm_memslots *slots; @@ -2057,7 +2057,7 @@ int kvm_set_internal_memslot(struct kvm *kvm, if (WARN_ON_ONCE(mem->slot < KVM_USER_MEM_SLOTS)) return -EINVAL; - return __kvm_set_memory_region(kvm, mem); + return kvm_set_memory_region(kvm, mem); } EXPORT_SYMBOL_GPL(kvm_set_internal_memslot); @@ -2068,7 +2068,7 @@ static int kvm_vm_ioctl_set_memory_region(struct kvm *kvm, return -EINVAL; guard(mutex)(&kvm->slots_lock); - return __kvm_set_memory_region(kvm, mem); + return kvm_set_memory_region(kvm, mem); } #ifndef CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT From 0cc3cb2151f9830274e7bef39a23dc1da1ecd34a Mon Sep 17 00:00:00 2001 From: Sean Christopherson Date: Fri, 10 Jan 2025 16:20:22 -0800 Subject: [PATCH 5/5] KVM: Disallow all flags for KVM-internal memslots Disallow all flags for KVM-internal memslots as all existing flags require some amount of userspace interaction to have any meaning. In addition to guarding against KVM goofs, explicitly disallowing dirty logging of KVM- internal memslots will (hopefully) allow exempting KVM-internal memslots from the KVM_MEM_MAX_NR_PAGES limit, which appears to exist purely because the dirty bitmap operations use a 32-bit index. Cc: Xiaoyao Li Cc: Claudio Imbrenda Cc: Christian Borntraeger Reviewed-by: Xiaoyao Li Reviewed-by: Claudio Imbrenda Acked-by: Christoph Schlameuss Link: https://lore.kernel.org/r/20250111002022.1230573-6-seanjc@google.com Signed-off-by: Sean Christopherson --- virt/kvm/kvm_main.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index ecd4a66b22f3..a8a84bf450f9 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -2057,6 +2057,9 @@ int kvm_set_internal_memslot(struct kvm *kvm, if (WARN_ON_ONCE(mem->slot < KVM_USER_MEM_SLOTS)) return -EINVAL; + if (WARN_ON_ONCE(mem->flags)) + return -EINVAL; + return kvm_set_memory_region(kvm, mem); } EXPORT_SYMBOL_GPL(kvm_set_internal_memslot);