xfs
[Top] [All Lists]

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

To: Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH] xfs_repair: test for bad level in dir2 node
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 10 Sep 2013 10:45:40 +1000
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <52274F96.2010702@xxxxxxxxxxx>
References: <52274F96.2010702@xxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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...

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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