xfs
[Top] [All Lists]

Re: [PATCH V2] xfs_repair: test for bad level in dir2 node

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH V2] xfs_repair: test for bad level in dir2 node
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Tue, 10 Sep 2013 12:24:09 -0500
Cc: "'linux-xfs@xxxxxxxxxxx'" <linux-xfs@xxxxxxxxxxx>
Delivered-to: linux-xfs@xxxxxxxxxxx
In-reply-to: <522F4C26.2080106@xxxxxxx>
References: <52274F96.2010702@xxxxxxxxxxx> <522F4001.8010104@xxxxxxxxxxx> <522F4C26.2080106@xxxxxxx>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:17.0) Gecko/20130801 Thunderbird/17.0.8
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<jyb@xxxxxxx>
>> Signed-off-by: Eric Sandeen<sandeen@xxxxxxxxxx>
>> ---
>>
>> 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

<Prev in Thread] Current Thread [Next in Thread>