On Sun, 2021-10-31 at 07:52 +0100, Paolo Bonzini wrote:
> On 30/10/21 09:58, David Woodhouse wrote:
> > > Absolutely! The fixed version of kvm_map_gfn should not do any
> > > map/unmap, it should do it eagerly on MMU notifier operations.
>
> > Staring at this some more... what*currently* protects a
> > gfn_to_pfn_cache when the page tables change — either because userspace
> > either mmaps something else over the same HVA, or the underlying page
> > is just swapped out and restored?
>
> kvm_cache_gfn_to_pfn calls gfn_to_pfn_memslot, which pins the page.
Empirially, this breaks the test case in the series I sent out last
night, because the kernel is looking at the *wrong* shared info page
after userspace maps a new one at that HVA:
diff --git a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
index c0a3b9cc2a86..5c41043d34d6 100644
--- a/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
+++ b/tools/testing/selftests/kvm/x86_64/xen_shinfo_test.c
@@ -204,6 +204,11 @@ int main(int argc, char *argv[])
SHINFO_REGION_GPA, SHINFO_REGION_SLOT, 2, 0);
virt_map(vm, SHINFO_REGION_GVA, SHINFO_REGION_GPA, 2);
+ struct shared_info *shinfo = addr_gpa2hva(vm, SHINFO_VADDR);
+
+ int zero_fd = open("/dev/zero", O_RDONLY);
+ TEST_ASSERT(zero_fd != -1, "Failed to open /dev/zero");
+
struct kvm_xen_hvm_config hvmc = {
.flags = KVM_XEN_HVM_CONFIG_INTERCEPT_HCALL,
.msr = XEN_HYPERCALL_MSR,
@@ -222,6 +227,9 @@ int main(int argc, char *argv[])
};
vm_ioctl(vm, KVM_XEN_HVM_SET_ATTR, &ha);
+ void *m = mmap(shinfo, PAGE_SIZE, PROT_READ|PROT_WRITE, MAP_FIXED|MAP_PRIVATE, zero_fd, 0);
+ TEST_ASSERT(m == shinfo, "Failed to map /dev/zero over shared info");
+
struct kvm_xen_vcpu_attr vi = {
.type = KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO,
.u.gpa = VCPU_INFO_ADDR,
@@ -295,8 +303,6 @@ int main(int argc, char *argv[])
sigaction(SIGALRM, &sa, NULL);
}
- struct shared_info *shinfo = addr_gpa2hva(vm, SHINFO_VADDR);
-
struct vcpu_info *vinfo = addr_gpa2hva(vm, VCPU_INFO_VADDR);
vinfo->evtchn_upcall_pending = 0;
And then this *fixes* it again, by spotting the unmap and invalidating
the cached mapping:
diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index 1a64ba5b9437..f7e94d8d21a6 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -25,6 +25,7 @@
#include "kvm_emulate.h"
#include "cpuid.h"
#include "spte.h"
+#include "xen.h"
#include <linux/kvm_host.h>
#include <linux/types.h>
@@ -1588,6 +1589,20 @@ bool kvm_unmap_gfn_range(struct kvm *kvm, struct kvm_gfn_range *range)
{
bool flush = false;
+ if (static_branch_unlikely(&kvm_xen_enabled.key) &&
+ kvm->arch.xen.shinfo_set) {
+ printk("Invalidate %llx-%llx\n", range->start, range->end);
+ write_lock(&kvm->arch.xen.shinfo_lock);
+ if (kvm->arch.xen.shinfo_cache.gfn >= range->start &&
+ kvm->arch.xen.shinfo_cache.gfn < range->end) {
+ kvm->arch.xen.shared_info = NULL;
+ /* We have to break the GFN to PFN cache too or it'll just
+ * remap the old page anyway. XX: Do this on the xen.c side */
+ kvm->arch.xen.shinfo_cache.generation = -1;
+ }
+ write_unlock(&kvm->arch.xen.shinfo_lock);
+ }
+
if (kvm_memslots_have_rmaps(kvm))
flush = kvm_handle_gfn_range(kvm, range, kvm_unmap_rmapp);
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
