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

Commit Message

Alex Williamson April 17, 2017, 2:32 p.m. UTC

On Mon, 17 Apr 2017 14:47:54 +0800
Peter Xu <[e-mail]> 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):

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;
> >  }  
>

Comments

Kirti Wankhede April 17, 2017, 7:05 p.m. UTC | #1

On 4/17/2017 8:02 PM, Alex Williamson wrote:
> On Mon, 17 Apr 2017 14:47:54 +0800
> Peter Xu <[e-mail]> 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 <[e-mail]>

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;
>>>  }  
>>
>

Patch

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);
 	}
 }