On Fri, 14 Apr 2017 14:58:14 -0600 Alex Williamson <[email protected]> wrote:
> On Sat, 15 Apr 2017 00:51:28 +0530 > Kirti Wankhede <[email protected]> wrote: > > > On 4/12/2017 12:58 AM, Alex Williamson wrote: > > > If the mmap_sem is contented then the vfio type1 IOMMU backend will > > > defer locked page accounting updates to a workqueue task. This has a > > > few problems and depending on which side the user tries to play, they > > > might be over-penalized for unmaps that haven't yet been accounted or > > > race the workqueue to enter more mappings than they're allowed. The > > > original intent of this workqueue mechanism seems to be focused on > > > reducing latency through the ioctl, but we cannot do so at the cost > > > of correctness. Remove this workqueue mechanism and update the > > > callers to allow for failure. We can also now recheck the limit under > > > write lock to make sure we don't exceed it. > > > > > > vfio_pin_pages_remote() also now necessarily includes an unwind path > > > which we can jump to directly if the consecutive page pinning finds > > > that we're exceeding the user's memory limits. This avoids the > > > current lazy approach which does accounting and mapping up to the > > > fault, only to return an error on the next iteration to unwind the > > > entire vfio_dma. > > > > > > Cc: [email protected] > > > Signed-off-by: Alex Williamson <[email protected]> > > > --- > > > > > > v3: Update for comments from Peter > > > - Use task_rlimit() exclusively > > > - Discuss vfio_pin_pages_remote() exit branch in commitlog > > > > > > drivers/vfio/vfio_iommu_type1.c | 99 > > > +++++++++++++++++---------------------- > > > 1 file changed, 44 insertions(+), 55 deletions(-) > > > > > > diff --git a/drivers/vfio/vfio_iommu_type1.c > > > b/drivers/vfio/vfio_iommu_type1.c > > > index 32d2633092a3..176ebcc0ffa2 100644 > > > --- a/drivers/vfio/vfio_iommu_type1.c > > > +++ b/drivers/vfio/vfio_iommu_type1.c > > > @@ -246,69 +246,43 @@ static int vfio_iova_put_vfio_pfn(struct vfio_dma > > > *dma, struct vfio_pfn *vpfn) > > > return ret; > > > } > > > > > > -struct vwork { > > > - struct mm_struct *mm; > > > - long npage; > > > - struct work_struct work; > > > -}; > > > - > > > -/* delayed decrement/increment for locked_vm */ > > > -static void vfio_lock_acct_bg(struct work_struct *work) > > > -{ > > > - struct vwork *vwork = container_of(work, struct vwork, work); > > > - struct mm_struct *mm; > > > - > > > - mm = vwork->mm; > > > - down_write(&mm->mmap_sem); > > > - mm->locked_vm += vwork->npage; > > > - up_write(&mm->mmap_sem); > > > - mmput(mm); > > > - kfree(vwork); > > > -} > > > - > > > -static void vfio_lock_acct(struct task_struct *task, long npage) > > > +static int vfio_lock_acct(struct task_struct *task, long npage) > > > { > > > - 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) { > > > + mm->locked_vm += npage; > > > + } else { > > > + unsigned long limit; > > > > > > - if (is_current) { > > > - mm = get_task_mm(task); > > > - if (!mm) > > > - return; > > > + limit = task_rlimit(task, RLIMIT_MEMLOCK) >> PAGE_SHIFT; > > > + > > > + if (mm->locked_vm + npage <= limit) > > > + mm->locked_vm += npage; > > > + else > > > + ret = -ENOMEM; > > > + } > > > + > > > > Sorry if I'm late here on my review. > > > > There are rlimit checks before calling vfio_lock_acct() while pinning > > pages. I agree this is checked holding locks, so this check is more > > robust, but still it feels redundant. I think you can remove checks from > > vfio_pin_page_external() and vfio_pin_pages_remote(). > > If we removed those pre-checks then a user/mdev vendor driver would be > able to pin massive amounts of memory, potentially causing a DoS on the > host (ex. trigger OOM), before we bother to test whether they really > have permission to do so. I think redundancy is better. Perhaps _external is overly redundant since it's only doing one page increments, _remote could go well into the above scenario w/o the redundancy. Thanks, Alex > > Also while checking the limit, !lock_cap checks is not considered here. > > That would mean that there code would impose limit check even without > > lock capability? > > That's a bug! Thanks, > > Alex > > > > + up_write(&mm->mmap_sem); > > > } > > > > > > - /* > > > - * Couldn't get mmap_sem lock, so must setup to update > > > - * mm->locked_vm later. If locked_vm were atomic, we > > > - * wouldn't need this silliness > > > - */ > > > - vwork = kmalloc(sizeof(struct vwork), GFP_KERNEL); > > > - if (WARN_ON(!vwork)) { > > > + if (!is_current) > > > mmput(mm); > > > - return; > > > - } > > > - INIT_WORK(&vwork->work, vfio_lock_acct_bg); > > > - vwork->mm = mm; > > > - vwork->npage = npage; > > > - schedule_work(&vwork->work); > > > + > > > + return ret; > > > } > > > > > > /* > > > @@ -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); > > > + > > > +unpin_out: > > > + if (ret) { > > > + if (!rsvd) { > > > + for (pfn = *pfn_base ; pinned ; pfn++, pinned--) > > > + put_pfn(pfn, dma->prot); > > > + } > > > + > > > + return ret; > > > + } > > > > > > return pinned; > > > } > > > @@ -522,8 +505,14 @@ static int vfio_pin_page_external(struct vfio_dma > > > *dma, unsigned long vaddr, > > > goto pin_page_exit; > > > } > > > > > > - if (!rsvd && do_accounting) > > > - vfio_lock_acct(dma->task, 1); > > > + if (!rsvd && do_accounting) { > > > + ret = vfio_lock_acct(dma->task, 1); > > > + if (ret) { > > > + put_pfn(*pfn_base, dma->prot); > > > + goto pin_page_exit; > > > + } > > > + } > > > + > > > ret = 1; > > > > > > pin_page_exit: > > > >
