| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20170417083201.043cedbf@t450s.home>
Date: Mon, 17 Apr 2017 08:32:01 -0600
From: Alex Williamson <alex.williamson@...hat.com>
To: Peter Xu <peterx@...hat.com>
Cc: kvm@...r.kernel.org, eric.auger@...hat.com, kwankhede@...dia.com,
linux-kernel@...r.kernel.org, slp@...hat.com
Subject: Re: [PATCH v4 1/2] vfio/type1: Remove locked page accounting
workqueue
On Mon, 17 Apr 2017 14:47:54 +0800
Peter Xu <peterx@...hat.com> 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;
> > }
>
Powered by blists - more mailing lists