Re: [PATCH v4 1/2] vfio/type1: Remove locked page accounting workqueue

[Kirti Wankhede]

On 4/17/2017 8:02 PM, Alex Williamson wrote:
> On Mon, 17 Apr 2017 14:47:54 +0800
> Peter Xu <peterx@xxxxxxxxxx> wrote:
> 
>> On Sun, Apr 16, 2017 at 07:42:27PM -0600, Alex Williamson wrote:
>>
>> [...]
>>
>>> -static void vfio_lock_acct(struct task_struct *task, long npage)
>>> +static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap)
>>>  {
>>> -	struct vwork *vwork;
>>>  	struct mm_struct *mm;
>>>  	bool is_current;
>>> +	int ret;
>>>  
>>>  	if (!npage)
>>> -		return;
>>> +		return 0;
>>>  
>>>  	is_current = (task->mm == current->mm);
>>>  
>>>  	mm = is_current ? task->mm : get_task_mm(task);
>>>  	if (!mm)
>>> -		return; /* process exited */
>>> +		return -ESRCH; /* process exited */
>>>  
>>> -	if (down_write_trylock(&mm->mmap_sem)) {
>>> -		mm->locked_vm += npage;
>>> -		up_write(&mm->mmap_sem);
>>> -		if (!is_current)
>>> -			mmput(mm);
>>> -		return;
>>> -	}
>>> +	ret = down_write_killable(&mm->mmap_sem);
>>> +	if (!ret) {
>>> +		if (npage < 0 || lock_cap) {  
>>
>> Nit: maybe we can avoid passing in lock_cap in all the callers of
>> vfio_lock_acct() and fetch it via has_capability() only if npage < 0?
>> IMHO that'll keep the vfio_lock_acct() interface cleaner, and we won't
>> need to pass in "false" any time when doing unpins.
> 
> Unfortunately vfio_pin_pages_remote() needs to know about lock_cap
> since it tests whether the user is exceeding their locked memory
> limit.  The other callers could certainly get away with
> vfio_lock_acct() testing the capability itself but that would add a
> redundant call for the most common user.  I'm not a big fan of passing
> a lock_cap bool either, but it seemed the best fix for now.  The
> cleanest alternative I can up with is this (untested):
> 

In my opinion, passing 'bool lock_cap' looks much clean and simple.

Reviewed-by: Kirti Wankhede <kwankhede@xxxxxxxxxx>

Thanks,
Kirti.

> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index 07e0e58f22e9..0dbcf950fef9 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -246,7 +246,7 @@ static int vfio_iova_put_vfio_pfn(struct vfio_dma *dma, struct vfio_pfn *vpfn)
>  	return ret;
>  }
>  
> -static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap)
> +static int vfio_lock_acct(struct task_struct *task, long npage, bool *lock_cap)
>  {
>  	struct mm_struct *mm;
>  	bool is_current;
> @@ -263,19 +263,24 @@ static int vfio_lock_acct(struct task_struct *task, long npage, bool lock_cap)
>  
>  	ret = down_write_killable(&mm->mmap_sem);
>  	if (!ret) {
> -		if (npage < 0 || lock_cap) {
> +		if (npage < 0 || (lock_cap && *lock_cap)) {
>  			mm->locked_vm += npage;
>  		} else {
> -			unsigned long limit;
> +			if (lock_cap || !has_capability(task, CAP_IPC_LOCK)) {
> +				unsigned long limit;
>  
> -			limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> +				limit = task_rlimit(task, RLIMIT_MEMLOCK)
> +								>> PAGE_SHIFT;
>  
> -			if (mm->locked_vm + npage <= limit)
> -				mm->locked_vm += npage;
> -			else
> -				ret = -ENOMEM;
> -		}
> +				if (mm->locked_vm + npage > limit) {
> +					ret = -ENOMEM;
> +					goto upwrite;
> +				}
> +			}
>  
> +			mm->locked_vm += npage;
> +		}
> +upwrite:
>  		up_write(&mm->mmap_sem);
>  	}
>  
> @@ -440,7 +445,7 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
>  	}
>  
>  out:
> -	ret = vfio_lock_acct(current, lock_acct, lock_cap);
> +	ret = vfio_lock_acct(current, lock_acct, &lock_cap);
>  
>  unpin_out:
>  	if (ret) {
> @@ -471,7 +476,7 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova,
>  	}
>  
>  	if (do_accounting)
> -		vfio_lock_acct(dma->task, locked - unlocked, false);
> +		vfio_lock_acct(dma->task, locked - unlocked, NULL);
>  
>  	return unlocked;
>  }
> @@ -488,8 +493,7 @@ static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr,
>  
>  	ret = vaddr_get_pfn(mm, vaddr, dma->prot, pfn_base);
>  	if (!ret && do_accounting && !is_invalid_reserved_pfn(*pfn_base)) {
> -		ret = vfio_lock_acct(dma->task, 1,
> -				     has_capability(dma->task, CAP_IPC_LOCK));
> +		ret = vfio_lock_acct(dma->task, 1, NULL);
>  		if (ret)
>  			put_pfn(*pfn_base, dma->prot);
>  	}
> @@ -510,7 +514,7 @@ static int vfio_unpin_page_external(struct vfio_dma *dma, dma_addr_t iova,
>  	unlocked = vfio_iova_put_vfio_pfn(dma, vpfn);
>  
>  	if (do_accounting)
> -		vfio_lock_acct(dma->task, -unlocked, false);
> +		vfio_lock_acct(dma->task, -unlocked, NULL);
>  
>  	return unlocked;
>  }
> @@ -705,7 +709,7 @@ static long vfio_unmap_unpin(struct vfio_iommu *iommu, struct vfio_dma *dma,
>  
>  	dma->iommu_mapped = false;
>  	if (do_accounting) {
> -		vfio_lock_acct(dma->task, -unlocked, false);
> +		vfio_lock_acct(dma->task, -unlocked, NULL);
>  		return 0;
>  	}
>  	return unlocked;
> @@ -1347,7 +1351,7 @@ static void vfio_iommu_unmap_unpin_reaccount(struct vfio_iommu *iommu)
>  			if (!is_invalid_reserved_pfn(vpfn->pfn))
>  				locked++;
>  		}
> -		vfio_lock_acct(dma->task, locked - unlocked, false);
> +		vfio_lock_acct(dma->task, locked - unlocked, NULL);
>  	}
>  }
>  
> ie. we keep that third arg to vfio_lock_acct(), but it's effectively
> optional.  Thoughts?
> 
> 
>> [...]
>>
>>> @@ -405,7 +379,7 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned long vaddr,
>>>  static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
>>>  				  long npage, unsigned long *pfn_base)
>>>  {
>>> -	unsigned long limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>>> +	unsigned long pfn = 0, limit = rlimit(RLIMIT_MEMLOCK) >> PAGE_SHIFT;
>>>  	bool lock_cap = capable(CAP_IPC_LOCK);
>>>  	long ret, pinned = 0, lock_acct = 0;
>>>  	bool rsvd;
>>> @@ -442,8 +416,6 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
>>>  	/* Lock all the consecutive pages from pfn_base */
>>>  	for (vaddr += PAGE_SIZE, iova += PAGE_SIZE; pinned < npage;
>>>  	     pinned++, vaddr += PAGE_SIZE, iova += PAGE_SIZE) {
>>> -		unsigned long pfn = 0;
>>> -
>>>  		ret = vaddr_get_pfn(current->mm, vaddr, dma->prot, &pfn);
>>>  		if (ret)
>>>  			break;
>>> @@ -460,14 +432,25 @@ static long vfio_pin_pages_remote(struct vfio_dma *dma, unsigned long vaddr,
>>>  				put_pfn(pfn, dma->prot);
>>>  				pr_warn("%s: RLIMIT_MEMLOCK (%ld) exceeded\n",
>>>  					__func__, limit << PAGE_SHIFT);
>>> -				break;
>>> +				ret = -ENOMEM;
>>> +				goto unpin_out;
>>>  			}
>>>  			lock_acct++;
>>>  		}
>>>  	}
>>>  
>>>  out:
>>> -	vfio_lock_acct(current, lock_acct);
>>> +	ret = vfio_lock_acct(current, lock_acct, lock_cap);  
>>
>> I just didn't notice this in previous review, but... do we need to
>> check against !rsvd as well here before doing the accounting?
> 
> rsvd is taken care of above, lock_acct is only incremented for
> non-reserved pages, so a block of rsvd pages would call vfio_lock_acct
> with 0 pages, which will immediately return.  Thanks,
> 
> Alex
> 
>>> +
>>> +unpin_out:
>>> +	if (ret) {
>>> +		if (!rsvd) {
>>> +			for (pfn = *pfn_base ; pinned ; pfn++, pinned--)
>>> +				put_pfn(pfn, dma->prot);
>>> +		}
>>> +
>>> +		return ret;
>>> +	}
>>>  
>>>  	return pinned;
>>>  }  
>>
>