From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751242AbdH1F2X (ORCPT ); Mon, 28 Aug 2017 01:28:23 -0400 Received: from zeniv.linux.org.uk ([195.92.253.2]:52696 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750708AbdH1F2V (ORCPT ); Mon, 28 Aug 2017 01:28:21 -0400 Date: Mon, 28 Aug 2017 06:28:08 +0100 From: Al Viro To: Paul Mackerras Cc: Nixiaoming , David Hildenbrand , "agraf@suse.com" , "pbonzini@redhat.com" , "rkrcmar@redhat.com" , "benh@kernel.crashing.org" , "mpe@ellerman.id.au" , "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 Message-ID: <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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170828043837.GA12629@fergus.ozlabs.ibm.com> User-Agent: Mutt/1.8.3 (2017-05-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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... From mboxrd@z Thu Jan 1 00:00:00 1970 From: Al Viro Date: Mon, 28 Aug 2017 05:28:08 +0000 Subject: Re: Re: [PATCH] fix memory leak on kvm_vm_ioctl_create_spapr_tce Message-Id: <20170828052808.GH5426@ZenIV.linux.org.uk> List-Id: 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> In-Reply-To: <20170828043837.GA12629@fergus.ozlabs.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Paul Mackerras Cc: Nixiaoming , David Hildenbrand , "agraf@suse.com" , "pbonzini@redhat.com" , "rkrcmar@redhat.com" , "benh@kernel.crashing.org" , "mpe@ellerman.id.au" , "kvm-ppc@vger.kernel.org" , "kvm@vger.kernel.org" , "linuxppc-dev@lists.ozlabs.org" , "linux-kernel@vger.kernel.org" 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...