From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751727AbcGNQ4a (ORCPT ); Thu, 14 Jul 2016 12:56:30 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50926 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751284AbcGNQ40 (ORCPT ); Thu, 14 Jul 2016 12:56:26 -0400 Subject: Re: [PATCH] KVM: release anon file in failure path of vm creation To: Al Viro References: <1468316323-23835-1-git-send-email-shuo.a.liu@intel.com> <28049c2a-1b49-1909-52ce-105859e14e33@redhat.com> <20160714164647.GD14480@ZenIV.linux.org.uk> Cc: Liu Shuo , linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Zhang Yanmin , He Bo , Liu Shuo , =?UTF-8?B?UmFkaW0gS3LEjW3DocWZ?= From: Paolo Bonzini Message-ID: Date: Thu, 14 Jul 2016 18:56:21 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20160714164647.GD14480@ZenIV.linux.org.uk> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 14 Jul 2016 16:56:25 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 14/07/2016 18:46, Al Viro wrote: > 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): Al, thanks very much for the explanation and the patch. I've reverted the original patch and installed yours instead. Paolo > [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 > --- > 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;git > } > >