Re: [PATCH Part2 RFC v4 25/40] KVM: SVM: Reclaim the guest pages when SEV-SNP VM terminates
From: Sean Christopherson <seanjc@google.com>
Date: 2021-07-16 20:09:28
Also in:
kvm, linux-coco, linux-crypto, linux-mm, lkml, platform-driver-x86
On Wed, Jul 07, 2021, Brijesh Singh wrote:
The guest pages of the SEV-SNP VM maybe added as a private page in the RMP entry (assigned bit is set). The guest private pages must be transitioned to the hypervisor state before its freed.
Isn't this patch needed much earlier in the series, i.e. when the first RMPUPDATE usage goes in?
quoted hunk ↗ jump to hunk
Signed-off-by: Brijesh Singh <redacted> --- arch/x86/kvm/svm/sev.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+)diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index 1f0635ac9ff9..4468995dd209 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c@@ -1940,6 +1940,45 @@ find_enc_region(struct kvm *kvm, struct kvm_enc_region *range) static void __unregister_enc_region_locked(struct kvm *kvm, struct enc_region *region) { + struct rmpupdate val = {}; + unsigned long i, pfn; + struct rmpentry *e; + int level, rc; + + /* + * The guest memory pages are assigned in the RMP table. Unassign it + * before releasing the memory. + */ + if (sev_snp_guest(kvm)) { + for (i = 0; i < region->npages; i++) { + pfn = page_to_pfn(region->pages[i]); + + if (need_resched()) + schedule();
This can simply be "cond_resched();"
+
+ e = snp_lookup_page_in_rmptable(region->pages[i], &level);
+ if (unlikely(!e))
+ continue;
+
+ /* If its not a guest assigned page then skip it. */
+ if (!rmpentry_assigned(e))
+ continue;
+
+ /* Is the page part of a 2MB RMP entry? */
+ if (level == PG_LEVEL_2M) {
+ val.pagesize = RMP_PG_SIZE_2M;
+ pfn &= ~(KVM_PAGES_PER_HPAGE(PG_LEVEL_2M) - 1);
+ } else {
+ val.pagesize = RMP_PG_SIZE_4K;This raises yet more questions (for me) as to the interaction between Page-Size and Hyperivsor-Owned flags in the RMP. It also raises questions on the correctness of zeroing the RMP entry if KVM_SEV_SNP_LAUNCH_START (in the previous patch).
+ }
+
+ /* Transition the page to hypervisor owned. */
+ rc = rmpupdate(pfn_to_page(pfn), &val);
+ if (rc)
+ pr_err("Failed to release pfn 0x%lx ret=%d\n", pfn, rc);This is not robust, e.g. KVM will unpin the memory and release it back to the kernel with a stale RMP entry. Shouldn't this be a WARN+leak situation?
+ } + } + sev_unpin_memory(kvm, region->pages, region->npages); list_del(®ion->list); kfree(region); -- 2.17.1