From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay.sgi.com (relay3.corp.sgi.com [198.149.34.15]) by oss.sgi.com (Postfix) with ESMTP id DF88529DFC for ; Tue, 1 Oct 2013 17:40:29 -0500 (CDT) Date: Tue, 1 Oct 2013 17:40:25 -0500 From: Ben Myers Subject: Re: [PATCH v2] xfs: don't leak root inode reference Message-ID: <20131001224025.GV1935@sgi.com> References: <20130826204730.GZ7153@sgi.com> <20130826212423.GX6023@dastard> <20130827212558.GY5262@sgi.com> <20130827220833.GY6023@dastard> <20130910231122.GY1935@sgi.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20130910231122.GY1935@sgi.com> List-Id: XFS Filesystem from SGI List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: xfs-bounces@oss.sgi.com Sender: xfs-bounces@oss.sgi.com To: Ben Myers Cc: xfs@oss.sgi.com On Tue, Sep 10, 2013 at 06:11:22PM -0500, Ben Myers wrote: > Hi Dave, > > On Wed, Aug 28, 2013 at 08:08:33AM +1000, Dave Chinner wrote: > > On Tue, Aug 27, 2013 at 04:25:58PM -0500, Ben Myers wrote: > > > On Tue, Aug 27, 2013 at 07:24:23AM +1000, Dave Chinner wrote: > > > > On Mon, Aug 26, 2013 at 03:47:30PM -0500, Ben Myers wrote: > > > > > Looks like in 48fde701 we removed the iput of the root inode in > > > > > xfs_fs_fill_super for the error case. Add it back. > > > > > > > > > > Signed-off-by: Ben Myers > > > > > > > > > > --- > > > > > fs/xfs/xfs_super.c | 6 ++++-- > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > Index: b/fs/xfs/xfs_super.c > > > > > =================================================================== > > > > > --- a/fs/xfs/xfs_super.c 2013-08-26 15:36:09.170848579 -0500 > > > > > +++ b/fs/xfs/xfs_super.c 2013-08-26 15:40:19.450817933 -0500 > > > > > @@ -1493,12 +1493,12 @@ xfs_fs_fill_super( > > > > > } > > > > > if (is_bad_inode(root)) { > > > > > error = EINVAL; > > > > > - goto out_unmount; > > > > > + goto out_iput; > > > > > } > > > > > sb->s_root = d_make_root(root); > > > > > if (!sb->s_root) { > > > > > error = ENOMEM; > > > > > - goto out_unmount; > > > > > + goto out_iput; > > > > > } > > > > > > > > That's wrong. d_make_root() drops the reference to the inode on > > > > failure itself, and so the change in 48fde701 is correct and valid. > > > > > > > > The leak on bad inodes (which, AFAICT, can never happen on XFS) has > > > > been around a lot longer than Al's change - this commit introduced > > > > it: > > > > > > > > 2bcf6e9 xfs: start periodic workers later > > > > > > > > with this hunk: > > > > > > > > if (is_bad_inode(root)) { > > > > error = EINVAL; > > > > - goto fail_vnrele; > > > > + goto out_syncd_stop; > > > > } > > > > > > Thanks Gents. Here's another try: > > > > > > xfs: don't leak root inode reference > > > > > > Looks like in 2bcf6e9 we removed the iput of the root inode in > > > xfs_fs_fill_super for the is_bad_inode error case. Add it back. > > > > > > Signed-off-by: Ben Myers > > > > I don't think this is right, either. > > > > As I said in my previous reply, I don't think that XFS can ever see > > a bad inode. The fact is that we're grabbing mp->m_rootip, which is > > we already have a reference to and is in cache and validated thanks > > to an xfs_iget() call in xfs_mountfs(). If we fail validation when > > reading the root inode into cache then xfs_mountfs() will fail and > > we won't ever get to this check. > > > > Further, XFS never marks inodes bad - even on a failed lookup or a > > shut down filesystem - and so AFAICT we cannot ever see the root > > inode (or any other XFS inode) as a bad inode. > > > > Hence I think that the is_bad_inode(root) check should just go away. > > Ok. Lets try this. ;) > > xfs: remove usage of is_bad_inode > > XFS never calls mark_inode_bad or iget_failed, so it will never see a > bad inode. Remove all checks for is_bad_inode because they are > unnecessary. > > Signed-off-by: Ben Myers Applied this one. _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs