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 A24C629DFB for ; Wed, 14 Aug 2013 19:33:46 -0500 (CDT) Received: from cuda.sgi.com (cuda2.sgi.com [192.48.176.25]) by relay3.corp.sgi.com (Postfix) with ESMTP id 21DB9AC002 for ; Wed, 14 Aug 2013 17:33:43 -0700 (PDT) Received: from ipmail05.adl6.internode.on.net (ipmail05.adl6.internode.on.net [150.101.137.143]) by cuda.sgi.com with ESMTP id DnfABjcqGkqWzioE for ; Wed, 14 Aug 2013 17:33:41 -0700 (PDT) Date: Thu, 15 Aug 2013 10:33:13 +1000 From: Dave Chinner Subject: Re: [PATCH] xfsprogs: fix inode crash in xfs_repair Message-ID: <20130815003313.GJ6023@dastard> References: <20130813221739.031858865@sgi.com> <20130814064013.GC12779@dastard> <520B870F.8040808@sgi.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <520B870F.8040808@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: Mark Tinguely Cc: xfs@oss.sgi.com On Wed, Aug 14, 2013 at 08:33:03AM -0500, Mark Tinguely wrote: > On 08/14/13 01:40, Dave Chinner wrote: > >On Tue, Aug 13, 2013 at 05:13:31PM -0500, Mark Tinguely wrote: > >>Adding the lost+found in phase 6 could allocate an inode from > >>a new inode chunk. That newly created chunk was not around in > >>the scan phase, and is not in the avl tree which will result > >>in a NULL dereference. > >> > >>This patch adds the newly created inode chunk and inodes as if > >>found in the scan phase. > >> > >>Metadata dump available for future tests. > >> > >>Signed-off-by: Mark Tinguely > >>--- > >> repair/incore_ino.c | 2 +- > >> repair/phase6.c | 15 +++++++++++++++ > >> 2 files changed, 16 insertions(+), 1 deletion(-) > >> > >>Index: b/repair/incore_ino.c > >>=================================================================== > >>--- a/repair/incore_ino.c > >>+++ b/repair/incore_ino.c > >>@@ -700,7 +700,7 @@ get_inode_parent(ino_tree_node_t *irec, > >> return(0LL); > >> } > >> > >>-static void > >>+void > >> alloc_ex_data(ino_tree_node_t *irec) > >> { > >> parent_list_t *ptbl; > >>Index: b/repair/phase6.c > >>=================================================================== > >>--- a/repair/phase6.c > >>+++ b/repair/phase6.c > >>@@ -930,6 +930,21 @@ mk_orphanage(xfs_mount_t *mp) > >> irec = find_inode_rec(mp, > >> XFS_INO_TO_AGNO(mp, ino), > >> XFS_INO_TO_AGINO(mp, ino)); > >>+ > >>+ if (irec == NULL&& XFS_INO_TO_AGNO(mp, ino)< mp->m_sb.sb_agcount&& > >>+ ip != NULL&& ip->i_d.di_magic == XFS_DINODE_MAGIC) { BTW, Mark, you're mailer is doing weird things to whitespace in code when it's quoting quoted code. > >I don't understand this check. > > > >We've already dereferenced ip several lines above to increment the > >link count and get the inode number stored in ino, so the ip != NULL > >is unnecessary. > > > >We've just allocated the inode, so why would the magic number be > >wrong? And why would the inode number lie in a non-existent > >allocation group? > > > > just being being paranoid. It's the same code as in the kernel - if is that broken that it can't tell it didn't allocate a real inode, then we've got bigger problems. We design the code to return an error when it fails so we don't have to robustly check every possible error condition at every call site. So really the only check needed is "if (!irec) {...}" Cheers, Dave. -- Dave Chinner david@fromorbit.com _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs