From patchwork Thu Jul 14 16:46:47 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Al Viro X-Patchwork-Id: 9230263 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 000B660572 for ; Thu, 14 Jul 2016 16:47:15 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id E5154281A7 for ; Thu, 14 Jul 2016 16:47:15 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id D982C28249; Thu, 14 Jul 2016 16:47:15 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=unavailable version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 671CD281A7 for ; Thu, 14 Jul 2016 16:47:15 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751633AbcGNQq4 (ORCPT ); Thu, 14 Jul 2016 12:46:56 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:58056 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751284AbcGNQqz (ORCPT ); Thu, 14 Jul 2016 12:46:55 -0400 Received: from viro by ZenIV.linux.org.uk with local (Exim 4.86_2 #1 (Red Hat Linux)) id 1bNjmh-0008SQ-Uw; Thu, 14 Jul 2016 16:46:48 +0000 Date: Thu, 14 Jul 2016 17:46:47 +0100 From: Al Viro To: Paolo Bonzini Cc: Liu Shuo , linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Zhang Yanmin , He Bo , Liu Shuo , Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH] KVM: release anon file in failure path of vm creation Message-ID: <20160714164647.GD14480@ZenIV.linux.org.uk> References: <1468316323-23835-1-git-send-email-shuo.a.liu@intel.com> <28049c2a-1b49-1909-52ce-105859e14e33@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <28049c2a-1b49-1909-52ce-105859e14e33@redhat.com> User-Agent: Mutt/1.6.0 (2016-04-01) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Tue, Jul 12, 2016 at 12:24:39PM +0200, Paolo Bonzini wrote: > On 12/07/2016 11:38, Liu Shuo wrote: > > The failure of create debugfs of VM will return directly without release > > the anon file. It will leak memory and file descriptors, even through > > be not serious. > > > > Signed-off-by: Liu Shuo This is broken. > > @@ -3067,6 +3068,7 @@ static int kvm_dev_ioctl_create_vm(unsigned long type) > > > > if (kvm_create_vm_debugfs(kvm, r) < 0) { > > kvm_put_kvm(kvm); > > + sys_close(r); You have no warranty whatsoever that descriptor table has not been changed by that point. You should *NEVER* use sys_close() on failure exit paths like that. Moreover, this kvm_put_kvm() becomes a double-put, since closing the damn file will drop that reference to kvm. > > return -ENOMEM; > > } > > > > > > Thanks, applied to kvm/master. Please, revert. anon_inode_getfd() should be used only when there's no possible failures past its call. What you need in this case (due to fucking ugly API - decriptor number used as part of debugfs pathname) is something like this (against mainline): [kvm] don't use anon_inode_getfd() before possible failures Once anon_inode_getfd() has succeeded, it's impossible to undo in a clean way and no, sys_close() is not usable in such cases. Use anon_inode_getfile() and get_unused_fd_flags() to get struct file and descriptor and do *not* install the file into descriptor table until after the last possible failure exit. Signed-off-by: Al Viro --- -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 48bd520..7f82c6c 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -3048,6 +3048,7 @@ static int kvm_dev_ioctl_create_vm(unsigned long type) { int r; struct kvm *kvm; + struct file *file; kvm = kvm_create_vm(type); if (IS_ERR(kvm)) @@ -3059,17 +3060,25 @@ static int kvm_dev_ioctl_create_vm(unsigned long type) return r; } #endif - r = anon_inode_getfd("kvm-vm", &kvm_vm_fops, kvm, O_RDWR | O_CLOEXEC); + r = get_unused_fd_flags(O_CLOEXEC); if (r < 0) { kvm_put_kvm(kvm); return r; } + file = anon_inode_getfile("kvm-vm", &kvm_vm_fops, kvm, O_RDWR); + if (IS_ERR(file)) { + put_unused_fd(r); + kvm_put_kvm(kvm); + return PTR_ERR(file); + } if (kvm_create_vm_debugfs(kvm, r) < 0) { - kvm_put_kvm(kvm); + put_unused_fd(r); + fput(file); return -ENOMEM; } + fd_install(r, file); return r; }