On 4/17/2017 8:02 PM, Alex Williamson wrote:
> On Mon, 17 Apr 2017 14:47:54 +0800
> Peter Xu <pet...@redhat.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):
>
In my opinion, passing 'bool lock_cap' looks much clean and simple.
Reviewed-by: Kirti Wankhede <kwankh...@nvidia.com>
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;
>>> }
>>
>
