[PATCH v2 0/2] kvm: iommu unmap fixes

[PATCH v2 0/2] kvm: iommu unmap fixes

[Alex Williamson]

This is a documentation change only from the previous version.
After discussing it, there is a potential page leak as noted
in the updated changelog for the first patch.  Thanks,

Alex

---

Alex Williamson (2):
      kvm: unpin guest and free iommu domain after deassign last device
      kvm: unmap pages from the iommu when slots are removed


 include/linux/kvm_host.h |    6 ++++++
 virt/kvm/assigned-dev.c  |    3 +++
 virt/kvm/iommu.c         |    8 +++++++-
 virt/kvm/kvm_main.c      |    5 +++--
 4 files changed, 19 insertions(+), 3 deletions(-)

[PATCH v2 1/2] kvm: unmap pages from the iommu when slots are removed

[Alex Williamson]

We've been adding new mappings, but not destroying old mappings.
This can lead to a page leak as pages are pinned using
get_user_pages, but only unpinned with put_page if they still
exist in the memslots list on vm shutdown.  A memslot that is
destroyed while an iommu domain is enabled for the guest will
therefore result in an elevated page reference count that is
never cleared.

Additionally, without this fix, the iommu is only programmed
with the first translation for a gpa.  This can result in
peer-to-peer errors if a mapping is destroyed and replaced by a
new mapping at the same gpa as the iommu will still be pointing
to the original, pinned memory address.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 include/linux/kvm_host.h |    6 ++++++
 virt/kvm/iommu.c         |    7 ++++++-
 virt/kvm/kvm_main.c      |    5 +++--
 3 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 665a260..72cbf08 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -596,6 +596,7 @@ void kvm_free_irq_source_id(struct kvm *kvm, int irq_source_id);
 
 #ifdef CONFIG_IOMMU_API
 int kvm_iommu_map_pages(struct kvm *kvm, struct kvm_memory_slot *slot);
+void kvm_iommu_unmap_pages(struct kvm *kvm, struct kvm_memory_slot *slot);
 int kvm_iommu_map_guest(struct kvm *kvm);
 int kvm_iommu_unmap_guest(struct kvm *kvm);
 int kvm_assign_device(struct kvm *kvm,
@@ -609,6 +610,11 @@ static inline int kvm_iommu_map_pages(struct kvm *kvm,
 	return 0;
 }
 
+static inline void kvm_iommu_unmap_pages(struct kvm *kvm,
+					 struct kvm_memory_slot *slot)
+{
+}
+
 static inline int kvm_iommu_map_guest(struct kvm *kvm)
 {
 	return -ENODEV;
diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
index a457d21..fec1723 100644
--- a/virt/kvm/iommu.c
+++ b/virt/kvm/iommu.c
@@ -310,6 +310,11 @@ static void kvm_iommu_put_pages(struct kvm *kvm,
 	}
 }
 
+void kvm_iommu_unmap_pages(struct kvm *kvm, struct kvm_memory_slot *slot)
+{
+	kvm_iommu_put_pages(kvm, slot->base_gfn, slot->npages);
+}
+
 static int kvm_iommu_unmap_memslots(struct kvm *kvm)
 {
 	int idx;
@@ -320,7 +325,7 @@ static int kvm_iommu_unmap_memslots(struct kvm *kvm)
 	slots = kvm_memslots(kvm);
 
 	kvm_for_each_memslot(memslot, slots)
-		kvm_iommu_put_pages(kvm, memslot->base_gfn, memslot->npages);
+		kvm_iommu_unmap_pages(kvm, memslot);
 
 	srcu_read_unlock(&kvm->srcu, idx);
 
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 42b7393..9739b53 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -808,12 +808,13 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	if (r)
 		goto out_free;
 
-	/* map the pages in iommu page table */
+	/* map/unmap the pages in iommu page table */
 	if (npages) {
 		r = kvm_iommu_map_pages(kvm, &new);
 		if (r)
 			goto out_free;
-	}
+	} else
+		kvm_iommu_unmap_pages(kvm, &old);
 
 	r = -ENOMEM;
 	slots = kmemdup(kvm->memslots, sizeof(struct kvm_memslots),

[PATCH v2 2/2] kvm: unpin guest and free iommu domain after deassign last device

[Alex Williamson]

Unpin the guest and free the iommu domain if there are no longer
any devices attached.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

 virt/kvm/assigned-dev.c |    3 +++
 virt/kvm/iommu.c        |    1 +
 2 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/virt/kvm/assigned-dev.c b/virt/kvm/assigned-dev.c
index 01f572c..01e7c37 100644
--- a/virt/kvm/assigned-dev.c
+++ b/virt/kvm/assigned-dev.c
@@ -765,6 +765,9 @@ static int kvm_vm_ioctl_deassign_device(struct kvm *kvm,
 
 	kvm_free_assigned_device(kvm, match);
 
+	if (list_empty(&kvm->arch.assigned_dev_head))
+		kvm_iommu_unmap_guest(kvm);
+
 out:
 	mutex_unlock(&kvm->lock);
 	return r;
diff --git a/virt/kvm/iommu.c b/virt/kvm/iommu.c
index fec1723..ee4c236 100644
--- a/virt/kvm/iommu.c
+++ b/virt/kvm/iommu.c
@@ -342,5 +342,6 @@ int kvm_iommu_unmap_guest(struct kvm *kvm)
 
 	kvm_iommu_unmap_memslots(kvm);
 	iommu_domain_free(domain);
+	kvm->arch.iommu_domain = NULL;
 	return 0;
 }

Re: [PATCH v2 0/2] kvm: iommu unmap fixes

[Greg KH]

On Wed, Apr 11, 2012 at 09:51:43AM -0600, Alex Williamson wrote:
> This is a documentation change only from the previous version.
> After discussing it, there is a potential page leak as noted
> in the updated changelog for the first patch.  Thanks,

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
for how to do this properly.

</formletter>

Re: [PATCH v2 0/2] kvm: iommu unmap fixes

[Alex Williamson]

On Wed, 2012-04-11 at 08:59 -0700, Greg KH wrote:
> On Wed, Apr 11, 2012 at 09:51:43AM -0600, Alex Williamson wrote:
> > This is a documentation change only from the previous version.
> > After discussing it, there is a potential page leak as noted
> > in the updated changelog for the first patch.  Thanks,
> 
> <formletter>
> 
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree.  Please read Documentation/stable_kernel_rules.txt
> for how to do this properly.
> 
> </formletter>

Sorry, Greg.  Thanks,

Alex

Re: [PATCH v2 1/2] kvm: unmap pages from the iommu when slots are removed

[Marcelo Tosatti]

On Wed, Apr 11, 2012 at 09:51:49AM -0600, Alex Williamson wrote:
> We've been adding new mappings, but not destroying old mappings.
> This can lead to a page leak as pages are pinned using
> get_user_pages, but only unpinned with put_page if they still
> exist in the memslots list on vm shutdown.  A memslot that is
> destroyed while an iommu domain is enabled for the guest will
> therefore result in an elevated page reference count that is
> never cleared.
> 
> Additionally, without this fix, the iommu is only programmed
> with the first translation for a gpa.  This can result in
> peer-to-peer errors if a mapping is destroyed and replaced by a
> new mapping at the same gpa as the iommu will still be pointing
> to the original, pinned memory address.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>

Applied, thanks.