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

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

[Eric Sandeen]

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@redhat.com>
---

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"),

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Dave Chinner]

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@redhat.com>
> ---
> 
> 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@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Eric Sandeen]

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@redhat.com>
>> ---
>>
>> 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

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Eric Sandeen]

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@gmx.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

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"),


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Mark Tinguely]

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@gmx.com>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> ---
>
> 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.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Eric Sandeen]

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@gmx.com>
>> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
>> ---
>>
>> 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

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

[Mark Tinguely]

On 09/10/13 12:24, Eric Sandeen wrote:
> 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@gmx.com>
>>> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
>>> ---
>>>
>>> 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?)

Yes. We do not loop if "i == 1", so another LEAF should not be found.

> 2) The block magic is NODE.  If not, we error out.

Yes.

> 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.

Yes must be a NODE.

> Any block which has a level that doesn't match is caught on the else of the (i == -1) block.

Yes, and "i" has to be larger than 1 because of the loop. Which I did 
not catch before.
>
> And those are the only 2 valid types here.
>
> What case is missing?
>
> -eric
>

With loop condition of "i > 1" then it cannot miss what I first thought 
was being missed, but the level of 1 being a leaf is not checked.

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Eric Sandeen]

On 9/10/13 1:03 PM, Mark Tinguely wrote:
>> 1) The block magic 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?)
> 
> Yes. We do not loop if "i == 1", so another LEAF should not be found.



>> 2) The block magic is NODE.  If not, we error out.
> 
> Yes.
> 
>> and as I showed above:
>> 3) The level matches each level we're at in the loop.
>>
>> So:
>>
>> Any block which isn't LEAFN or NODE is caught prior to the (i == -1) block.
> 
> Yes must be a NODE.
> 
>> Any block which has a level that doesn't match is caught on the else of the (i == -1) block.
> 
> Yes, and "i" has to be larger than 1 because of the loop. Which I did not catch before.
>>
>> And those are the only 2 valid types here.
>>
>> What case is missing?
>>
>> -eric
>>
> 
> With loop condition of "i > 1" then it cannot miss what I first thought was being missed, but the level of 1 being a leaf is not checked.

But I don't think that's right, is it?  level[0] is leaf; level[1] is a node, right?

Argh.  Now I'm more confused; xfs_check has:

        case XFS_DA_NODE_MAGIC:
                node = iocur_top->data;
                xfs_da3_node_hdr_from_disk(&nodehdr, node);
                if (nodehdr.level < 1 || nodehdr.level > XFS_DA_NODE_MAXDEPTH) {
                        if (!sflag || v)
                                dbprintf(_("bad node block level %d for dir ino "
                                         "%lld block %d\n"),
                                        nodehdr.level, id->ino,
                                        dabno);
                        error++;

so nodehdr.level == XFS_DA_NODE_MAXDEPTH is valid there (and level == 1 is a valid
node), but repair says:

                        if (i >= XFS_DA_NODE_MAXDEPTH) {
                                do_warn(
_("bad header depth for directory inode %" PRIu64 "\n"),
                                        da_cursor->ino);

so nodehdr.level == XFS_DA_NODE_MAXDEPTH is *not* valid here.

indices and counters and depths, oh my.  I need to back up and remember what's what.  :(

... Still not sure any of this invalidates my targeted fix - although I should just 
make it a one-liner and do:

                if (i == -1) {
                        i = da_cursor->active = nodehdr.level;
-                       if (i >= XFS_DA_NODE_MAXDEPTH) {
+                       if (i < 1 || i >= XFS_DA_NODE_MAXDEPTH) {
                                do_warn(
 _("bad header depth for directory inode %" PRIu64 "\n"),

-Eric

> --Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Eric Sandeen]

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@gmx.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

V3: Simplify the test.

Mark, Dave, I know you had some concerns about other conditions being
tested, but I think those are separate from this fix, which simply ensures
that the level we find for this _NODE block is within the valid range
for a node.  (It also matches the test currently present in xfs_check).

If we've got other missing conditions, those can be other patches,
I think.

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..e82ca7d 100644
--- a/repair/dir2.c
+++ b/repair/dir2.c
@@ -220,7 +220,7 @@ _("bad record count in inode %" PRIu64 ", count = %d, max = %d\n"),
 		 */
 		if (i == -1) {
 			i = da_cursor->active = nodehdr.level;
-			if (i >= XFS_DA_NODE_MAXDEPTH) {
+			if (i < 1 || i >= XFS_DA_NODE_MAXDEPTH) {
 				do_warn(
 _("bad header depth for directory inode %" PRIu64 "\n"),
 					da_cursor->ino);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Mark Tinguely]

On 09/12/13 15:56, 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@gmx.com>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> ---
>
> V3: Simplify the test.
>
> Mark, Dave, I know you had some concerns about other conditions being
> tested, but I think those are separate from this fix, which simply ensures
> that the level we find for this _NODE block is within the valid range
> for a node.  (It also matches the test currently present in xfs_check).
>

Nod.

--Mark.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Mark Tinguely]

On 09/12/13 15:56, 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@gmx.com>
> Signed-off-by: Eric Sandeen<sandeen@redhat.com>
> ---
>
> V3: Simplify the test.
>
> Mark, Dave, I know you had some concerns about other conditions being
> tested, but I think those are separate from this fix, which simply ensures
> that the level we find for this _NODE block is within the valid range
> for a node.  (It also matches the test currently present in xfs_check).
>
> If we've got other missing conditions, those can be other patches,
> I think.
>
> 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

I okay with this to fix the bug. I will make a note to think more on the 
level == 1 case, but that is not related to the bug.

Reviewed-by: Mark Tinguely <tinguely@sgi.com>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

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

[Rich Johnston]

This has been committed.

Thanks
--Rich

commit 44dae5e6804408b4123a916a2738b73e21d8c61e
Author: Eric Sandeen <sandeen@sandeen.net>
Date:   Thu Sep 12 20:56:36 2013 +0000

     xfs_repair: test for bad level in dir2 node

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs