Re: [PATCH v4 2/2] vfio/type1: Prune vfio_pin_page_external()

[Kirti Wankhede]

On 4/17/2017 7:12 AM, Alex Williamson wrote:
> With vfio_lock_acct() testing the locked memory limit under mmap_sem,
> it's redundant to do it here for a single page.  We can also reorder
> our tests such that we can avoid testing for reserved pages if we're
> not doing accounting, and test the process CAP_IPC_LOCK only if we
> are doing accounting.  Finally, this function oddly returns 1 on
> success.  Update to return zero on success, -errno on error.  Since
> the function only pins a single page, there's no need to return the
> number of pages pinned.
> 
> N.B. vfio_pin_pages_remote() can pin a large contiguous range of pages
> before calling vfio_lock_acct().  If we were to similarly remove the
> extra test there, a user could temporarily pin far more pages than
> they're allowed.
> 
> Suggested-by: Kirti Wankhede <kwankhede@xxxxxxxxxx>
> Suggested-by: Peter Xu <peterx@xxxxxxxxxx>
> Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> ---
>  drivers/vfio/vfio_iommu_type1.c |   34 +++++-----------------------------
>  1 file changed, 5 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index fb18e4a5df62..07e0e58f22e9 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -479,43 +479,21 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova,
>  static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr,
>  				  unsigned long *pfn_base, bool do_accounting)
>  {
> -	unsigned long limit;
> -	bool lock_cap = has_capability(dma->task, CAP_IPC_LOCK);
>  	struct mm_struct *mm;
>  	int ret;
> -	bool rsvd;
>  
>  	mm = get_task_mm(dma->task);
>  	if (!mm)
>  		return -ENODEV;
>  
>  	ret = vaddr_get_pfn(mm, vaddr, dma->prot, pfn_base);
> -	if (ret)
> -		goto pin_page_exit;
> -
> -	rsvd = is_invalid_reserved_pfn(*pfn_base);
> -	limit = task_rlimit(dma->task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> -
> -	if (!rsvd && !lock_cap && mm->locked_vm + 1 > limit) {
> -		put_pfn(*pfn_base, dma->prot);
> -		pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n",
> -			__func__, dma->task->comm, task_pid_nr(dma->task),
> -			limit << PAGE_SHIFT);
> -		ret = -ENOMEM;
> -		goto pin_page_exit;
> -	}
> -
> -	if (!rsvd && do_accounting) {
> -		ret = vfio_lock_acct(dma->task, 1, lock_cap);
> -		if (ret) {
> +	if (!ret && do_accounting && !is_invalid_reserved_pfn(*pfn_base)) {
> +		ret = vfio_lock_acct(dma->task, 1,
> +				     has_capability(dma->task, CAP_IPC_LOCK));
> +		if (ret)
>  			put_pfn(*pfn_base, dma->prot);
> -			goto pin_page_exit;
> -		}
>  	}
>  
> -	ret = 1;
> -
> -pin_page_exit:
>  	mmput(mm);
>  	return ret;
>  }

Thanks. This looks clean.
Just a nit pick, if vfio_lock_acct() returns -ENOMEM, its better to have
warning about task's mlock limit exceeded, which got removed in the
cleanup. No need to review again.

Reviewed-by: Kirti Wankhede <kwankhede@xxxxxxxxxx>

Thanks,
Kirti


> @@ -595,10 +573,8 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
>  		remote_vaddr = dma->vaddr + iova - dma->iova;
>  		ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i],
>  					     do_accounting);
> -		if (ret <= 0) {
> -			WARN_ON(!ret);
> +		if (ret)
>  			goto pin_unwind;
> -		}
>  
>  		ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
>  		if (ret) {
>