From mboxrd@z Thu Jan  1 00:00:00 1970
From: Kirti Wankhede <kwankhede@nvidia.com>
Subject: Re: [PATCH v7 3/4] vfio iommu: Add support for mediated devices
Date: Thu, 29 Sep 2016 20:36:30 +0530
Message-ID: <c205f9d9-581f-e213-3f2d-38dfd7f05bba@nvidia.com>
References: <1472097235-6332-1-git-send-email-kwankhede@nvidia.com>
 <1472097235-6332-4-git-send-email-kwankhede@nvidia.com>
 <57EC79B3.2060305@intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Cc: <alex.williamson@redhat.com>, <pbonzini@redhat.com>,
        <kraxel@redhat.com>, <cjia@nvidia.com>, <qemu-devel@nongnu.org>,
        <kvm@vger.kernel.org>, <kevin.tian@intel.com>,
        <bjsdjshi@linux.vnet.ibm.com>,
        "Xiao, Guangrong" <guangrong.xiao@intel.com>
To: Jike Song <jike.song@intel.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from hqemgate14.nvidia.com ([216.228.121.143]:8712 "EHLO
        hqemgate14.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S932280AbcI2PHK (ORCPT <rfc822;kvm@vger.kernel.org>);
        Thu, 29 Sep 2016 11:07:10 -0400
In-Reply-To: <57EC79B3.2060305@intel.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>



On 9/29/2016 7:47 AM, Jike Song wrote:
> +Guangrong
> 
> On 08/25/2016 11:53 AM, Kirti Wankhede wrote:

...

>> +static long vfio_iommu_type1_pin_pages(void *iommu_data,
>> +				       unsigned long *user_pfn,
>> +				       long npage, int prot,
>> +				       unsigned long *phys_pfn)
>> +{
>> +	struct vfio_iommu *iommu = iommu_data;
>> +	struct vfio_domain *domain;
>> +	int i, j, ret;
>> +	long retpage;
>> +	unsigned long remote_vaddr;
>> +	unsigned long *pfn = phys_pfn;
>> +	struct vfio_dma *dma;
>> +	bool do_accounting = false;
>> +
>> +	if (!iommu || !user_pfn || !phys_pfn)
>> +		return -EINVAL;
>> +
>> +	mutex_lock(&iommu->lock);
>> +
>> +	if (!iommu->local_domain) {
>> +		ret = -EINVAL;
>> +		goto pin_done;
>> +	}
>> +
>> +	domain = iommu->local_domain;
>> +
>> +	/*
>> +	 * If iommu capable domain exist in the container then all pages are
>> +	 * already pinned and accounted. Accouting should be done if there is no
>> +	 * iommu capable domain in the container.
>> +	 */
>> +	do_accounting = !IS_IOMMU_CAPABLE_DOMAIN_IN_CONTAINER(iommu);
>> +
>> +	for (i = 0; i < npage; i++) {
>> +		struct vfio_pfn *p;
>> +		dma_addr_t iova;
>> +
>> +		iova = user_pfn[i] << PAGE_SHIFT;
>> +
>> +		dma = vfio_find_dma(iommu, iova, 0);
>> +		if (!dma) {
>> +			ret = -EINVAL;
>> +			goto pin_unwind;
>> +		}
>> +
>> +		remote_vaddr = dma->vaddr + iova - dma->iova;
>> +
>> +		retpage = __vfio_pin_pages_local(domain, remote_vaddr, prot,
>> +						 &pfn[i], do_accounting);
> 
> Hi Kirti,
> 
> Here you call __vfio_pin_pages_local() > vaddr_get_pfn() > GUP regardless
> whether the vaddr already pinned or not. That probably means, if the caller 
> calls vfio_pin_pages() with a GPA for multiple times, you get memory leaks.
> 
> GUP always increases the page refcnt.
> 
> FWIW, I would like to have the pfn_list_lock implemented with key == iova,
> so you can always try to find the PFN for a given iova, and pin it only if
> not found.
> 

I didn't get how there would be a memory leak.

Right, GUP increases refcnt, so if vfio_pin_pages() is called for
multiple types for same GPA, refcnt would be incremented. In
vfio_iommu_type1_pin_pages() pinned pages list is maintained with
ref_count. If pfn is already in list, ref_count is incremented and same
is used while unpining pages.

Kirti