On 4/17/2017 7:12 AM, Alex Williamson wrote:
> With vfio_lock_acct() testing the locked memory limit under mmap_sem,
> it's redundant to do it here for a single page. We can also reorder
> our tests such that we can avoid testing for reserved pages if we're
> not doing accounting, and test the process CAP_IPC_LOCK only if we
> are doing accounting. Finally, this function oddly returns 1 on
> success. Update to return zero on success, -errno on error. Since
> the function only pins a single page, there's no need to return the
> number of pages pinned.
>
> N.B. vfio_pin_pages_remote() can pin a large contiguous range of pages
> before calling vfio_lock_acct(). If we were to similarly remove the
> extra test there, a user could temporarily pin far more pages than
> they're allowed.
>
> Suggested-by: Kirti Wankhede <kwankhede@xxxxxxxxxx>
> Suggested-by: Peter Xu <peterx@xxxxxxxxxx>
> Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
> ---
> drivers/vfio/vfio_iommu_type1.c | 34 +++++-----------------------------
> 1 file changed, 5 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index fb18e4a5df62..07e0e58f22e9 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -479,43 +479,21 @@ static long vfio_unpin_pages_remote(struct vfio_dma *dma, dma_addr_t iova,
> static int vfio_pin_page_external(struct vfio_dma *dma, unsigned long vaddr,
> unsigned long *pfn_base, bool do_accounting)
> {
> - unsigned long limit;
> - bool lock_cap = has_capability(dma->task, CAP_IPC_LOCK);
> struct mm_struct *mm;
> int ret;
> - bool rsvd;
>
> mm = get_task_mm(dma->task);
> if (!mm)
> return -ENODEV;
>
> ret = vaddr_get_pfn(mm, vaddr, dma->prot, pfn_base);
> - if (ret)
> - goto pin_page_exit;
> -
> - rsvd = is_invalid_reserved_pfn(*pfn_base);
> - limit = task_rlimit(dma->task, RLIMIT_MEMLOCK) >> PAGE_SHIFT;
> -
> - if (!rsvd && !lock_cap && mm->locked_vm + 1 > limit) {
> - put_pfn(*pfn_base, dma->prot);
> - pr_warn("%s: Task %s (%d) RLIMIT_MEMLOCK (%ld) exceeded\n",
> - __func__, dma->task->comm, task_pid_nr(dma->task),
> - limit << PAGE_SHIFT);
> - ret = -ENOMEM;
> - goto pin_page_exit;
> - }
> -
> - if (!rsvd && do_accounting) {
> - ret = vfio_lock_acct(dma->task, 1, lock_cap);
> - if (ret) {
> + if (!ret && do_accounting && !is_invalid_reserved_pfn(*pfn_base)) {
> + ret = vfio_lock_acct(dma->task, 1,
> + has_capability(dma->task, CAP_IPC_LOCK));
> + if (ret)
> put_pfn(*pfn_base, dma->prot);
> - goto pin_page_exit;
> - }
> }
>
> - ret = 1;
> -
> -pin_page_exit:
> mmput(mm);
> return ret;
> }
Thanks. This looks clean.
Just a nit pick, if vfio_lock_acct() returns -ENOMEM, its better to have
warning about task's mlock limit exceeded, which got removed in the
cleanup. No need to review again.
Reviewed-by: Kirti Wankhede <kwankhede@xxxxxxxxxx>
Thanks,
Kirti
> @@ -595,10 +573,8 @@ static int vfio_iommu_type1_pin_pages(void *iommu_data,
> remote_vaddr = dma->vaddr + iova - dma->iova;
> ret = vfio_pin_page_external(dma, remote_vaddr, &phys_pfn[i],
> do_accounting);
> - if (ret <= 0) {
> - WARN_ON(!ret);
> + if (ret)
> goto pin_unwind;
> - }
>
> ret = vfio_add_to_pfn_list(dma, iova, phys_pfn[i]);
> if (ret) {
>
