Re: [Qemu-devel] [PATCH v7 3/4] vfio iommu: Add support for mediated devices

[Jike Song]

On 09/29/2016 11:06 PM, Kirti Wankhede wrote:
> 
> 
> 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.
> 

Let's have a close look at vfio_unpin_pfn:

        static int vfio_unpin_pfn(struct vfio_domain *domain,
                                  struct vfio_pfn *vpfn, bool do_accounting)
        {
                __vfio_unpin_pages_for_mdev(domain, vpfn->pfn, vpfn->prot,
                                            do_accounting);

                if (atomic_dec_and_test(&vpfn->ref_count))
                        vfio_remove_from_pfn_list(domain, vpfn);

                return 1;
        }

Here you didn't call __vfio_unpin_pages_for_mdev -- thereby put_page -- for
vpfn->ref_count times. If page->_refcount increased by GUP for (N) times, here
you only set it back to (N-1).

--
Thanks,
Jike