Messages in this thread | | | Date | Fri, 15 Jul 2016 10:22:04 +0800 | From | Liu Shuo <> | Subject | Re: [PATCH] KVM: release anon file in failure path of vm creation |
| |
On Thu 14.Jul'16 at 17:46:47 +0100, 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 <shuo.a.liu@intel.com> > >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 Could you please elaborate why we're not sure descriptor table's changing at the point? >like that. Moreover, this kvm_put_kvm() becomes a double-put, since >closing the damn file will drop that reference to kvm. You're right. Thanks for the correction on this point. > >> > 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 <viro@zeniv.linux.org.uk> Thanks. It's better. >--- >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; > } >
| |