Re: [ 49/62] KVM: unmap pages from the iommu when slots are removed

[Alex Williamson <alex.williamson@redhat.com>]

On Fri, 2012-04-27 at 16:54 -0500, Jonathan Nieder wrote:
> Hi,
> 
> Greg KH wrote:
> 
> > 3.3-stable review patch.
> [...]
> > commit 32f6daad4651a748a58a3ab6da0611862175722f upstream.
> >
> > We've been adding new mappings, but not destroying old mappings.
> > This can lead to a page leak
> 
> Does 3.0.y need this?

Yep, same issues exists there.

>   The patch does not apply as is to 3.0.y because
> the latter lacks v3.3-rc1~131^2~41 ("KVM: introduce kvm_for_each_memslot
> macro"), but a backport is straightforward.
> 
> Completely untested.  Test results and other comments welcome.

Looks correct to me.

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

Thanks,
Alex

> -- >8 --
> From: Alex Williamson <alex.williamson@redhat.com>
> Date: Wed, 11 Apr 2012 09:51:49 -0600
> 
> commit 32f6daad4651a748a58a3ab6da0611862175722f upstream.
> 
> 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>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  include/linux/kvm_host.h |    6 ++++++
>  virt/kvm/iommu.c         |   12 ++++++++----
>  virt/kvm/kvm_main.c      |    5 +++--
>  3 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 31ebb59cbd2f..82d5476e69cc 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -554,6 +554,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,
> @@ -567,6 +568,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 62a9caf0563c..fb0f6e469bb4 100644
> --- a/virt/kvm/iommu.c
> +++ b/virt/kvm/iommu.c
> @@ -285,6 +285,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 i, idx;
> @@ -293,10 +298,9 @@ static int kvm_iommu_unmap_memslots(struct kvm *kvm)
>  	idx = srcu_read_lock(&kvm->srcu);
>  	slots = kvm_memslots(kvm);
>  
> -	for (i = 0; i < slots->nmemslots; i++) {
> -		kvm_iommu_put_pages(kvm, slots->memslots[i].base_gfn,
> -				    slots->memslots[i].npages);
> -	}
> +	for (i = 0; i < slots->nmemslots; i++)
> +		kvm_iommu_unmap_pages(kvm, &slots->memslots[i]);
> +
>  	srcu_read_unlock(&kvm->srcu, idx);
>  
>  	return 0;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 96ebc0679415..6b39ba9540e8 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -796,12 +796,13 @@ skip_lpage:
>  	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 = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL);