From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay.sgi.com (relay2.corp.sgi.com [137.38.102.29]) by oss.sgi.com (Postfix) with ESMTP id 584897F54 for ; Tue, 10 Sep 2013 12:24:16 -0500 (CDT) Received: from cuda.sgi.com (cuda3.sgi.com [192.48.176.15]) by relay2.corp.sgi.com (Postfix) with ESMTP id 486BE304051 for ; Tue, 10 Sep 2013 10:24:16 -0700 (PDT) Received: from sandeen.net (sandeen.net [63.231.237.45]) by cuda.sgi.com with ESMTP id r2CgFcZE21ZTTBJ7 for ; Tue, 10 Sep 2013 10:24:10 -0700 (PDT) Message-ID: <522F55B9.3030509@sandeen.net> Date: Tue, 10 Sep 2013 12:24:09 -0500 From: Eric Sandeen MIME-Version: 1.0 Subject: Re: [PATCH V2] xfs_repair: test for bad level in dir2 node References: <52274F96.2010702@sandeen.net> <522F4001.8010104@sandeen.net> <522F4C26.2080106@sgi.com> In-Reply-To: <522F4C26.2080106@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: "'linux-xfs@oss.sgi.com'" On 9/10/13 11:43 AM, Mark Tinguely wrote: > On 09/10/13 10:51, Eric Sandeen wrote: >> In traverse_int_dir2block(), the variable 'i' is the level in >> the tree, with 0 being a leaf node. In the "do" loop we >> start at the root, and work our way down to a leaf. >> >> If the first node we read is an interior node with NODE_MAGIC, >> but it tells us that its level is 0 (a leaf), this is clearly >> an inconsistency. >> >> Worse, we'd return with success, bno set, and only level[0] >> in the cursor initialized. Then down this path we'll >> segfault when accessing an uninitialized (and zeroed) member >> of the cursor's level array: >> >> process_node_dir2 >> traverse_int_dir2block // returns 0 w/ bno set, only level[0] init'd >> process_leaf_level_dir2 >> verify_dir2_path(mp, da_cursor, 0) // p_level == 0 >> this_level = p_level + 1; >> node = cursor->level[this_level].bp->b_addr; // level[1] uninit& 0'd >> >> Fix this by recognizing that an interior node w/ level 0 is invalid, and >> error out as for other inconsistencies. >> >> By the time the level 0 test is done, we have already ensured that >> this block has XFS_DA[3]_NODE_MAGIC. >> >> Reported-by: Jan Yves Brueckner >> Signed-off-by: Eric Sandeen >> --- >> >> V2: Drop re-test of hdr magic which is guaranteed to be NODE at this point. >> fix "interior inode" - s/b "interior node" >> >> My only testcase for this is Jan Yves Brueckner's badly corrupted >> filesystem image. With this change, we get i.e. : >> >> +bad level in interior inode for directory inode 39869938 >> +corrupt block 6 in directory inode 39869957 >> + will junk block >> >> diff --git a/repair/dir2.c b/repair/dir2.c >> index 05bd4b7..24db351 100644 >> --- a/repair/dir2.c >> +++ b/repair/dir2.c >> @@ -220,6 +220,15 @@ _("bad record count in inode %" PRIu64 ", count = %d, max = %d\n"), >> */ >> if (i == -1) { >> i = da_cursor->active = nodehdr.level; >> + /* Tests above ensure that we have NODE_MAGIC here */ >> + if (i == 0) { >> + do_warn( >> +_("bad level 0 in interior node for directory inode %" PRIu64 "\n"), >> + da_cursor->ino); >> + libxfs_putbuf(bp); >> + i = -1; >> + goto error_out; >> + } >> if (i>= XFS_DA_NODE_MAXDEPTH) { >> do_warn( >> _("bad header depth for directory inode %" PRIu64 "\n"), >> > > But moving the check out of the (i == -1) block, then the loop can check all the intermediate nodes along the way and also the ending leaf. > > --Mark. > Let me think about this. There is already some level consistency checking at each level: if (nodehdr.level == i - 1) { i--; } else { do_warn( _("bad directory btree for directory inode %" PRIu64 "\n"), ... goto error_out; but I guess maybe we could check _magic_ more carefully on other levels. Is that what you mean? Hm, but as I cited above, we *already* check that either: 1) The block magc is LEAFN. If so, we stop. We warn if it's not root level (but don't fix? Maybe that's a bug for another patch?) 2) The block magic is NODE. If not, we error out. and as I showed above: 3) The level matches each level we're at in the loop. So: Any block which isnt' LEAFN or NODE is caught prior to the (i == -1) block. Any block which has a level that doesn't match is caught on the else of the (i == -1) block. And those are the only 2 valid types here. What case is missing? -eric _______________________________________________ xfs mailing list xfs@oss.sgi.com http://oss.sgi.com/mailman/listinfo/xfs