Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751462AbdH1LbT (ORCPT ); Mon, 28 Aug 2017 07:31:19 -0400 Received: from ozlabs.org ([103.22.144.67]:50933 "EHLO ozlabs.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750873AbdH1LbR (ORCPT ); Mon, 28 Aug 2017 07:31:17 -0400 From: Michael Ellerman To: Al Viro , Paul Mackerras Cc: Nixiaoming , David Hildenbrand , "agraf\@suse.com" , "pbonzini\@redhat.com" , "rkrcmar\@redhat.com" , "benh\@kernel.crashing.org" , "kvm-ppc\@vger.kernel.org" , "kvm\@vger.kernel.org" , "linuxppc-dev\@lists.ozlabs.org" , "linux-kernel\@vger.kernel.org" Subject: Re: Re: [PATCH] fix memory leak on kvm_vm_ioctl_create_spapr_tce In-Reply-To: <20170828052808.GH5426@ZenIV.linux.org.uk> References: <20170822142823.69425-1-nixiaoming@huawei.com> <95fe182a-fd21-77a1-33df-0e609c2845fd@redhat.com> <8fd9a878-a8ce-5576-9a5c-1c221ff6ded7@redhat.com> <20170823060624.GA13958@fergus.ozlabs.ibm.com> <20170827210220.GG5426@ZenIV.linux.org.uk> <20170828043837.GA12629@fergus.ozlabs.ibm.com> <20170828052808.GH5426@ZenIV.linux.org.uk> User-Agent: Notmuch/0.21 (https://notmuchmail.org) Date: Mon, 28 Aug 2017 21:31:15 +1000 Message-ID: <87h8wsudx8.fsf@concordia.ellerman.id.au> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 1130 Lines: 28 Al Viro writes: > On Mon, Aug 28, 2017 at 02:38:37PM +1000, Paul Mackerras wrote: >> On Sun, Aug 27, 2017 at 10:02:20PM +0100, Al Viro wrote: >> > On Wed, Aug 23, 2017 at 04:06:24PM +1000, Paul Mackerras wrote: >> > >> > > It seems to me that it would be better to do the anon_inode_getfd() >> > > call before the kvm_get_kvm() call, and go to the fail label if it >> > > fails. >> > >> > And what happens if another thread does close() on the (guessed) fd? >> >> Chaos ensues, but mostly because we don't have proper mutual exclusion >> on the modifications to the list. I'll add a mutex_lock/unlock to >> kvm_spapr_tce_release() and move the anon_inode_getfd() call inside >> the mutex. >> >> It looks like the other possible uses of the fd (mmap, and passing it >> as a parameter to the KVM_DEV_VFIO_GROUP_SET_SPAPR_TCE ioctl on a KVM >> device fd) are safe. > > Frankly, it's a lot saner to have "no failure points past anon_inode_getfd()" > policy... Actually I thought that was a hard rule. But I don't see it documented or mentioned anywhere so I'm not sure now why I thought that. cheers