Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757378AbdDQTRP (ORCPT ); Mon, 17 Apr 2017 15:17:15 -0400 Received: from nat-hk.nvidia.com ([203.18.50.4]:5742 "EHLO nat-hk.nvidia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753883AbdDQTRJ (ORCPT ); Mon, 17 Apr 2017 15:17:09 -0400 X-PGP-Universal: processed; by hkpgpgate102.nvidia.com on Mon, 17 Apr 2017 12:17:01 -0700 Subject: Re: [PATCH v4 2/2] vfio/type1: Prune vfio_pin_page_external() To: Alex Williamson , References: <20170417014142.25866.16852.stgit@gimli.home> <20170417014239.25866.42333.stgit@gimli.home> CC: , , , X-Nvconfidentiality: public From: Kirti Wankhede Message-ID: <16f06e1b-ea0f-0b50-d083-aba77a496c5f@nvidia.com> Date: Tue, 18 Apr 2017 00:46:51 +0530 MIME-Version: 1.0 In-Reply-To: <20170417014239.25866.42333.stgit@gimli.home> X-Originating-IP: [10.24.71.218] X-ClientProxiedBy: BGMAIL101.nvidia.com (10.25.59.10) To DRBGMAIL102.nvidia.com (10.18.16.21) Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3367 Lines: 101 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 > Suggested-by: Peter Xu > Signed-off-by: Alex Williamson > --- > 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 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) { >