[{"id":1756312,"web_url":"http://patchwork.ozlabs.org/comment/1756312/","msgid":"<20170824111513.GI5379@umbus.fritz.box>","date":"2017-08-24T11:15:13","subject":"Re: [PATCH really v2] KVM: PPC: Book3S: Fix race and leak in\n\tkvm_vm_ioctl_create_spapr_tce()","submitter":{"id":47,"url":"http://patchwork.ozlabs.org/api/people/47/","name":"David Gibson","email":"david@gibson.dropbear.id.au"},"content":"On Thu, Aug 24, 2017 at 07:14:47PM +1000, Paul Mackerras wrote:\n> Nixiaoming pointed out that there is a memory leak in\n> kvm_vm_ioctl_create_spapr_tce() if the call to anon_inode_getfd()\n> fails; the memory allocated for the kvmppc_spapr_tce_table struct\n> is not freed, and nor are the pages allocated for the iommu\n> tables. In addition, we have already incremented the process's\n> count of locked memory pages, and this doesn't get restored on\n> error.\n> \n> David Hildenbrand pointed out that there is a race in that the\n> function checks early on that there is not already an entry in the\n> stt->iommu_tables list with the same LIOBN, but an entry with the\n> same LIOBN could get added between then and when the new entry is\n> added to the list.\n> \n> This fixes all three problems. To simplify things, we now call\n> anon_inode_getfd() before placing the new entry in the list. The\n> check for an existing entry is done while holding the kvm->lock\n> mutex, immediately before adding the new entry to the list.\n> Finally, on failure we now call kvmppc_account_memlimit to\n> decrement the process's count of locked memory pages.\n> \n> Reported-by: Nixiaoming \n> Reported-by: David Hildenbrand \n> Signed-off-by: Paul Mackerras \n\nReviewed-by: David Gibson \n\n> ---\n> v2: Don't overwrite stt in loop over spapr_tce_tables\n> \n> arch/powerpc/kvm/book3s_64_vio.c | 56 ++++++++++++++++++++++++----------------\n> 1 file changed, 34 insertions(+), 22 deletions(-)\n> \n> diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c\n> index a160c14..53766e2 100644\n> --- a/arch/powerpc/kvm/book3s_64_vio.c\n> +++ b/arch/powerpc/kvm/book3s_64_vio.c\n> @@ -294,32 +294,26 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,\n> \t\t\t\t struct kvm_create_spapr_tce_64 *args)\n> {\n> \tstruct kvmppc_spapr_tce_table *stt = NULL;\n> +\tstruct kvmppc_spapr_tce_table *siter;\n> \tunsigned long npages, size;\n> \tint ret = -ENOMEM;\n> \tint i;\n> +\tint fd = -1;\n> \n> \tif (!args->size)\n> \t\treturn -EINVAL;\n> \n> -\t/* Check this LIOBN hasn't been previously allocated */\n> -\tlist_for_each_entry(stt, &kvm->arch.spapr_tce_tables, list) {\n> -\t\tif (stt->liobn == args->liobn)\n> -\t\t\treturn -EBUSY;\n> -\t}\n> -\n> \tsize = _ALIGN_UP(args->size, PAGE_SIZE >> 3);\n> \tnpages = kvmppc_tce_pages(size);\n> \tret = kvmppc_account_memlimit(kvmppc_stt_pages(npages), true);\n> -\tif (ret) {\n> -\t\tstt = NULL;\n> -\t\tgoto fail;\n> -\t}\n> +\tif (ret)\n> +\t\treturn ret;\n> \n> \tret = -ENOMEM;\n> \tstt = kzalloc(sizeof(*stt) + npages * sizeof(struct page *),\n> \t\t GFP_KERNEL);\n> \tif (!stt)\n> -\t\tgoto fail;\n> +\t\tgoto fail_acct;\n> \n> \tstt->liobn = args->liobn;\n> \tstt->page_shift = args->page_shift;\n> @@ -334,24 +328,42 @@ long kvm_vm_ioctl_create_spapr_tce(struct kvm *kvm,\n> \t\t\tgoto fail;\n> \t}\n> \n> -\tkvm_get_kvm(kvm);\n> +\tret = fd = anon_inode_getfd(\"kvm-spapr-tce\", &kvm_spapr_tce_fops,\n> +\t\t\t\t stt, O_RDWR | O_CLOEXEC);\n> +\tif (ret < 0)\n> +\t\tgoto fail;\n> \n> \tmutex_lock(&kvm->lock);\n> -\tlist_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables);\n> +\n> +\t/* Check this LIOBN hasn't been previously allocated */\n> +\tret = 0;\n> +\tlist_for_each_entry(siter, &kvm->arch.spapr_tce_tables, list) {\n> +\t\tif (siter->liobn == args->liobn) {\n> +\t\t\tret = -EBUSY;\n> +\t\t\tbreak;\n> +\t\t}\n> +\t}\n> +\n> +\tif (!ret) {\n> +\t\tlist_add_rcu(&stt->list, &kvm->arch.spapr_tce_tables);\n> +\t\tkvm_get_kvm(kvm);\n> +\t}\n> \n> \tmutex_unlock(&kvm->lock);\n> \n> -\treturn anon_inode_getfd(\"kvm-spapr-tce\", &kvm_spapr_tce_fops,\n> -\t\t\t\tstt, O_RDWR | O_CLOEXEC);\n> +\tif (!ret)\n> +\t\treturn fd;\n> \n> -fail:\n> -\tif (stt) {\n> -\t\tfor (i = 0; i < npages; i++)\n> -\t\t\tif (stt->pages[i])\n> -\t\t\t\t__free_page(stt->pages[i]);\n> +\tput_unused_fd(fd);\n> \n> -\t\tkfree(stt);\n> -\t}\n> + fail:\n> +\tfor (i = 0; i < npages; i++)\n> +\t\tif (stt->pages[i])\n> +\t\t\t__free_page(stt->pages[i]);\n> +\n> +\tkfree(stt);\n> + fail_acct:\n> +\tkvmppc_account_memlimit(kvmppc_stt_pages(npages), false);\n> \treturn ret;\n> }\n>","headers":{"Return-Path":"","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xdNvq4WHrz9sR9\n\tfor ;\n\tThu, 24 Aug 2017 22:31:51 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xdNvq3N8DzDqkc\n\tfor ;\n\tThu, 24 Aug 2017 22:31:51 +1000 (AEST)","from ozlabs.org (ozlabs.org [IPv6:2401:3900:2:1::2])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xdNsv2pmDzDqhL\n\tfor ;\n\tThu, 24 Aug 2017 22:30:11 +1000 (AEST)","by ozlabs.org (Postfix, from userid 1007)\n\tid 3xdNsv117Cz9sR9; Thu, 24 Aug 2017 22:30:11 +1000 (AEST)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=gibson.dropbear.id.au\n\theader.i=@gibson.dropbear.id.au header.b=\"NikERs9Q\"; \n\tdkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=gibson.dropbear.id.au\n\theader.i=@gibson.dropbear.id.au header.b=\"NikERs9Q\"; \n\tdkim-atps=neutral","lists.ozlabs.org; dkim=pass (1024-bit key;\n\tunprotected) header.d=gibson.dropbear.id.au\n\theader.i=@gibson.dropbear.id.au\n\theader.b=\"NikERs9Q\"; dkim-atps=neutral"],"DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple;\n\td=gibson.dropbear.id.au; s=201602; t=1503577811;\n\tbh=MeTvBKTmOMABtGnrdt8cGo+b5JRyXkp18PO5iqutYsc=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=NikERs9QeSjXlgH5Unu8DfDvRiFHERY/XcTJEE75IyVlNFAE0lMMDjqaGjx44yVH3\n\ti2tlTndan9rjGIzcfq3Lcr8VsgF8c4ZOaq02KwzvyBOm3HvBSMmMuKI2iKLIzUNIQz\n\tAj4sLLh6YHHx8mxlA5X6L0Yf8fgKLHqnSwCtLIgw=","Date":"Thu, 24 Aug 2017 21:15:13 +1000","From":"David Gibson ","To":"Paul Mackerras ","Subject":"Re: [PATCH really v2] KVM: PPC: Book3S: Fix race and leak in\n\tkvm_vm_ioctl_create_spapr_tce()","Message-ID":"<20170824111513.GI5379@umbus.fritz.box>","References":"<20170824091447.GG27401@fergus.ozlabs.ibm.com>","MIME-Version":"1.0","Content-Type":"multipart/signed; micalg=pgp-sha256;\n\tprotocol=\"application/pgp-signature\"; boundary=\"qih7n4MdZQ4fb9uN\"","Content-Disposition":"inline","In-Reply-To":"<20170824091447.GG27401@fergus.ozlabs.ibm.com>","User-Agent":"Mutt/1.8.3 (2017-05-23)","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List\n\t","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"nixiaoming , linuxppc-dev@lists.ozlabs.org,\n\tkvm@vger.kernel.org, kvm-ppc@vger.kernel.org,\n\tDavid Hildenbrand ","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n\t"}},{"id":1756620,"web_url":"http://patchwork.ozlabs.org/comment/1756620/","msgid":"<88b11c61-08e7-f1a3-6930-578f539ce546@redhat.com>","date":"2017-08-24T17:25:22","subject":"Re: [PATCH really v2] KVM: PPC: Book3S: Fix race and leak in\n\tkvm_vm_ioctl_create_spapr_tce()","submitter":{"id":70402,"url":"http://patchwork.ozlabs.org/api/people/70402/","name":"David Hildenbrand","email":"david@redhat.com"},"content":"On 24.08.2017 11:14, Paul Mackerras wrote:\n> Nixiaoming pointed out that there is a memory leak in\n> kvm_vm_ioctl_create_spapr_tce() if the call to anon_inode_getfd()\n> fails; the memory allocated for the kvmppc_spapr_tce_table struct\n> is not freed, and nor are the pages allocated for the iommu\n> tables. In addition, we have already incremented the process's\n> count of locked memory pages, and this doesn't get restored on\n> error.\n> \n> David Hildenbrand pointed out that there is a race in that the\n> function checks early on that there is not already an entry in the\n> stt->iommu_tables list with the same LIOBN, but an entry with the\n> same LIOBN could get added between then and when the new entry is\n> added to the list.\n> \n> This fixes all three problems. To simplify things, we now call\n> anon_inode_getfd() before placing the new entry in the list. The\n> check for an existing entry is done while holding the kvm->lock\n> mutex, immediately before adding the new entry to the list.\n> Finally, on failure we now call kvmppc_account_memlimit to\n> decrement the process's count of locked memory pages.\n> \n> Reported-by: Nixiaoming \n> Reported-by: David Hildenbrand \n> Signed-off-by: Paul Mackerras \n> ---\n> v2: Don't overwrite stt in loop over spapr_tce_tables\n> \n\nReviewed-by: David Hildenbrand ","headers":{"Return-Path":"","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xdWS80XDfz9sNV\n\tfor ;\n\tFri, 25 Aug 2017 03:26:48 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xdWS75pQbzDrJS\n\tfor ;\n\tFri, 25 Aug 2017 03:26:47 +1000 (AEST)","from mx1.redhat.com (mx1.redhat.com [209.132.183.28])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xdWQZ6H70zDrJN\n\tfor ;\n\tFri, 25 Aug 2017 03:25:26 +1000 (AEST)","from smtp.corp.redhat.com\n\t(int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby mx1.redhat.com (Postfix) with ESMTPS id BA87AC04D2AB;\n\tThu, 24 Aug 2017 17:25:24 +0000 (UTC)","from [10.36.116.68] (ovpn-116-68.ams2.redhat.com [10.36.116.68])\n\tby smtp.corp.redhat.com (Postfix) with ESMTP id 5DA4F729C9;\n\tThu, 24 Aug 2017 17:25:23 +0000 (UTC)"],"DMARC-Filter":"OpenDMARC Filter v1.3.2 mx1.redhat.com BA87AC04D2AB","Authentication-Results":["ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tdmarc=none (p=none dis=none) header.from=redhat.com","ext-mx07.extmail.prod.ext.phx2.redhat.com;\n\tspf=fail smtp.mailfrom=david@redhat.com"],"Subject":"Re: [PATCH really v2] KVM: PPC: Book3S: Fix race and leak in\n\tkvm_vm_ioctl_create_spapr_tce()","To":"Paul Mackerras , kvm-ppc@vger.kernel.org,\n\tkvm@vger.kernel.org, linuxppc-dev@lists.ozlabs.org","References":"<20170824091447.GG27401@fergus.ozlabs.ibm.com>","From":"David Hildenbrand ","Organization":"Red Hat GmbH","Message-ID":"<88b11c61-08e7-f1a3-6930-578f539ce546@redhat.com>","Date":"Thu, 24 Aug 2017 19:25:22 +0200","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101\n\tThunderbird/52.2.1","MIME-Version":"1.0","In-Reply-To":"<20170824091447.GG27401@fergus.ozlabs.ibm.com>","Content-Type":"text/plain; charset=utf-8","Content-Language":"en-US","Content-Transfer-Encoding":"7bit","X-Scanned-By":"MIMEDefang 2.79 on 10.5.11.16","X-Greylist":"Sender IP whitelisted, not delayed by milter-greylist-4.5.16\n\t(mx1.redhat.com [10.5.110.31]); Thu, 24 Aug 2017 17:25:25 +0000 (UTC)","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List\n\t","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"nixiaoming ,\n\tDavid Gibson ","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n\t"}},{"id":1756959,"web_url":"http://patchwork.ozlabs.org/comment/1756959/","msgid":"","date":"2017-08-25T01:43:39","subject":"RE: [PATCH really v2] KVM: PPC: Book3S: Fix race and leak in\n\tkvm_vm_ioctl_create_spapr_tce()","submitter":{"id":72208,"url":"http://patchwork.ozlabs.org/api/people/72208/","name":"Xiaoming Ni","email":"nixiaoming@huawei.com"},"content":"On 24.08.2017 11:14, Paul Mackerras wrote:\n> Nixiaoming pointed out that there is a memory leak in\n> kvm_vm_ioctl_create_spapr_tce() if the call to anon_inode_getfd() \n> fails; the memory allocated for the kvmppc_spapr_tce_table struct is \n> not freed, and nor are the pages allocated for the iommu tables. In \n> addition, we have already incremented the process's count of locked \n> memory pages, and this doesn't get restored on error.\n> \n> David Hildenbrand pointed out that there is a race in that the \n> function checks early on that there is not already an entry in the\n> stt->iommu_tables list with the same LIOBN, but an entry with the\n> same LIOBN could get added between then and when the new entry is \n> added to the list.\n> \n> This fixes all three problems. To simplify things, we now call\n> anon_inode_getfd() before placing the new entry in the list. The \n> check for an existing entry is done while holding the kvm->lock mutex, \n> immediately before adding the new entry to the list.\n> Finally, on failure we now call kvmppc_account_memlimit to decrement \n> the process's count of locked memory pages.\n> \n> Reported-by: Nixiaoming \n> Reported-by: David Hildenbrand \n> Signed-off-by: Paul Mackerras \n> ---\n> v2: Don't overwrite stt in loop over spapr_tce_tables\n> \n\nReviewed-by: nixiaoming ","headers":{"Return-Path":"","X-Original-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@ozlabs.org","linuxppc-dev@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xdkWG5vsZz9t3m\n\tfor ;\n\tFri, 25 Aug 2017 11:45:14 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xdkWG55rVzDrL7\n\tfor ;\n\tFri, 25 Aug 2017 11:45:14 +1000 (AEST)","from szxga01-in.huawei.com (szxga01-in.huawei.com [45.249.212.187])\n\t(using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xdkTt0fhQzDrFM\n\tfor ;\n\tFri, 25 Aug 2017 11:44:01 +1000 (AEST)","from 172.30.72.57 (EHLO DGGEMA406-HUB.china.huawei.com)\n\t([172.30.72.57])\n\tby dggrg01-dlp.huawei.com (MOS 4.4.6-GA FastPath queued)\n\twith ESMTP id AVF07068; Fri, 25 Aug 2017 09:43:45 +0800 (CST)","from DGGEMA505-MBX.china.huawei.com ([169.254.1.46]) by\n\tDGGEMA406-HUB.china.huawei.com ([10.3.20.47]) with mapi id\n\t14.03.0301.000; Fri, 25 Aug 2017 09:43:39 +0800"],"From":"Nixiaoming ","To":"Paul Mackerras ","Subject":"RE: [PATCH really v2] KVM: PPC: Book3S: Fix race and leak in\n\tkvm_vm_ioctl_create_spapr_tce()","Thread-Topic":"[PATCH really v2] KVM: PPC: Book3S: Fix race and leak in\n\tkvm_vm_ioctl_create_spapr_tce()","Thread-Index":"AQHTHLl25R+MCNJjhku7usrlOD0nvaKUTUYw","Date":"Fri, 25 Aug 2017 01:43:39 +0000","Message-ID":"","References":"<20170824091447.GG27401@fergus.ozlabs.ibm.com>","In-Reply-To":"<20170824091447.GG27401@fergus.ozlabs.ibm.com>","Accept-Language":"zh-CN, en-US","Content-Language":"zh-CN","X-MS-Has-Attach":"","X-MS-TNEF-Correlator":"","x-originating-ip":"[10.57.88.168]","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"quoted-printable","MIME-Version":"1.0","X-CFilter-Loop":"Reflected","X-Mirapoint-Virus-RAPID-Raw":"score=unknown(0),\n\trefid=str=0001.0A090204.599F80D2.00E9, ss=1, re=0.000, recu=0.000,\n\treip=0.000, \n\tcl=1, cld=1, fgs=0, ip=169.254.1.46,\n\tso=2014-11-16 11:51:01, dmn=2013-03-21 17:37:32","X-Mirapoint-Loop-Id":"c3a089018fb0277b1181c23594f9d81b","X-BeenThere":"linuxppc-dev@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Linux on PowerPC Developers Mail List\n\t","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"David Gibson ,\n\t\"linuxppc-dev@lists.ozlabs.org\" ,\n\t\"kvm@vger.kernel.org\" ,\n\t\"kvm-ppc@vger.kernel.org\" ,\n\tDavid Hildenbrand ","Errors-To":"linuxppc-dev-bounces+patchwork-incoming=ozlabs.org@lists.ozlabs.org","Sender":"\"Linuxppc-dev\"\n\t"}}]