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

[Eric Sandeen <sandeen@xxxxxxxxxxx>]

On 9/9/13 7:45 PM, Dave Chinner wrote:
> On Wed, Sep 04, 2013 at 10:19:50AM -0500, 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.
>>
>> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
>> ---
>>
>> 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..20c6e1a 100644
>> --- a/repair/dir2.c
>> +++ b/repair/dir2.c
>> @@ -220,6 +220,16 @@ _("bad record count in inode %" PRIu64 ", count = %d, 
>> max = %d\n"),
>>               */
>>              if (i == -1) {
>>                      i = da_cursor->active = nodehdr.level;
>> +                    if (i == 0 &&
>> +                        (nodehdr.magic == XFS_DA_NODE_MAGIC ||
>> +                         nodehdr.magic == XFS_DA3_NODE_MAGIC)) {
>> +                            do_warn(
>> +_("bad level 0 in interior inode 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"),
> 
> Looks sane, though wouldn't it be better to check for the correct
> header magic number (i.e LEAF1/LEAFN) here? i.e. if we are at level
> zero and we don't have a leaf, then there's something wrong. This
> will only catch the case of a node replacing a leaf, not a free
> space block or data block being at the wrong place...

Hm, well, above my new test we have (slightly snipped down):

                if (nodehdr.magic == XFS_DIR2_LEAFN_MAGIC ||
                    nodehdr.magic == XFS_DIR3_LEAFN_MAGIC)  {
                        ...
                        *rbno = 0;
                        libxfs_putbuf(bp);
                        return(1);
                } else if (!(nodehdr.magic == XFS_DA_NODE_MAGIC ||
                             nodehdr.magic == XFS_DA3_NODE_MAGIC))  {
                        ...
_("bad dir magic number 0x%x in inode %" PRIu64 " bno = %u\n"),
                        goto error_out;
                }

so by this point, we actually MUST be either XFS_DA_NODE_MAGIC or 
XFS_DA3_NODE_MAGIC

and then I added:

                if (i == -1) {
                        i = da_cursor->active = nodehdr.level;
                        if (i == 0 &&
                            (nodehdr.magic == XFS_DA_NODE_MAGIC ||
                             nodehdr.magic == XFS_DA3_NODE_MAGIC)) {
                                do_warn(
_("bad level 0 in interior inode for directory inode %" PRIu64 "\n"),
                                        da_cursor->ino);
                                libxfs_putbuf(bp);
                                i = -1;
                                goto error_out;
                        }

So if anything, I should probably just drop the magic test, because it's 
already ensured.
(along with a comment ...)

-Eric