walk_down_tree will fault when read_tree_block fails with NULL returned. Signed-off-by: Lin Ming <mlin@kernel.org> --- 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; -- 1.7.2.5 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Eric Sandeen
2013-May-02 04:32 UTC
Re: [PATCH] btrfs-progs: fix segfault in walk_down_tree
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@kernel.org> > --- > 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? 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@jp.fujitsu.com> 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@jp.fujitsu.com> Signed-off-by: Chris Mason <chris.mason@oracle.com> ... 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. 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 2, 2013 at 12:32 AM, Eric Sandeen <sandeen@redhat.com> 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@kernel.org> >> --- >> 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@jp.fujitsu.com> > 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@jp.fujitsu.com> > Signed-off-by: Chris Mason <chris.mason@oracle.com> > > ... > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html