Re: [PATCH] xfsprogs: fix inode crash in xfs_repair

[Dave Chinner <david@fromorbit.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<tinguely@sgi.com>
> >>---
> >>  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