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

[Alex Williamson]

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):

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