On Thu, May 2, 2013 at 12:32 AM, Eric Sandeen <sandeen@xxxxxxxxxx> wrote:
> On 5/1/13 9:46 PM, Lin Ming wrote:
>> walk_down_tree will fault when read_tree_block fails with NULL returned.
>>
>> Signed-off-by: Lin Ming <mlin@xxxxxxxxxx>
>> ---
>> cmds-check.c | 2 ++
>> 1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/cmds-check.c b/cmds-check.c
>> index 12192fa..e4435d5 100644
>> --- a/cmds-check.c
>> +++ b/cmds-check.c
>> @@ -1256,6 +1256,8 @@ static int walk_down_tree(struct btrfs_root *root, struct btrfs_path *path,
>> reada_walk_down(root, cur, path->slots[*level]);
>> next = read_tree_block(root, bytenr, blocksize,
>> ptr_gen);
>> + if (!next)
>> + goto out;
>> }
>>
>> *level = *level - 1;
>>
>
> I suppose that could fix a segfault . . . although, details
> would be nice. next is only used as:
>
> path->nodes[*level] = next;
>
> before it gets reassigned in the loop. So I guess someone
> uses the path array and doesn't handle a NULL?
walk_down_tree():
while (*level >= 0) {
.....
cur = path->nodes[*level];
if (btrfs_header_level(cur) != *level) <---- segfault here
.....
path->nodes[*level] = next;
>
> Ok, but if read_tree_block fails(), doesn't that mean some error
> occurred? And if so, walk_down_tree would still return 0,
> which looks like success to the caller. Then what?
>
> I guess that's what the other error returns in this function
> do as well. :(
>
> But this seems like suboptimal behavior for a filesystem checker,
> doesn't it? Just silently ignoring errors?
>
> And while this might fix the segfault I'm afraid it does so without
> really handling the problem, and it might never be noticed again.
>
> The caller looks like it might handle an error, if one were ever
> passed up (which it's not right now):
>
> wret = walk_down_tree(root, &path, wc, &level);
> if (wret < 0)
> ret = wret;
> if (wret != 0)
> break;
>
> Over on the kernel side, this commit at least catches the error
> and passes it up:
>
> commit 97d9a8a420444eb5b5c071d4b3b9c4100a7ae015
> Author: Tsutomu Itoh <t-itoh@xxxxxxxxxxxxxx>
> Date: Thu Mar 24 06:33:21 2011 +0000
>
> Btrfs: check return value of read_tree_block()
>
> This patch is checking return value of read_tree_block(),
> and if it is NULL, error processing.
>
> Signed-off-by: Tsutomu Itoh <t-itoh@xxxxxxxxxxxxxx>
> Signed-off-by: Chris Mason <chris.mason@xxxxxxxxxx>
>
> ...
> next = read_tree_block(root, bytenr, blocksize, generation);
> + if (!next)
> + return -EIO;
> ...
>
> so that's probably a better way to go, though it might require some testing.
Agree, we should pass up the return value of walk_down_tree().
Lin Ming
>
> Another reason to get userspace caught up w/ kernelspace, argh.
>
> -Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
