| Message ID | 20210812081617.20811-1-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: Improve error handling while loading log root | expand |
On 2021/8/12 下午4:16, Nikolay Borisov wrote: > read_tree_block can return an error due to a variety of reason, > currently its return value is not being checked when loading the > log root's node but is directly used in a call to > extent_buffer_uptodate. This can lead to a crash if read_tree_block > errored out, since the node won't be a pointer but an error value cast > to a pointer. > > Fix this by properly checking the return value of read_tree_block before > utilising the value for anything else. > --- > kernel-shared/disk-io.c | 9 ++++----- > 1 file changed, 4 insertions(+), 5 deletions(-) > > diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c > index cc635152c46d..4b5436254671 100644 > --- a/kernel-shared/disk-io.c > +++ b/kernel-shared/disk-io.c > @@ -629,15 +629,14 @@ static int find_and_setup_log_root(struct btrfs_root *tree_root, > > log_root->node = read_tree_block(fs_info, blocknr, > btrfs_super_generation(disk_super) + 1); > - > - fs_info->log_root_tree = log_root; > - > - if (!extent_buffer_uptodate(log_root->node)) { > - free_extent_buffer(log_root->node); > + if (IS_ERR(log_root->node) || !extent_buffer_uptodate(log_root->node)) { extent_buffer_uptodate() already has check for IS_ERROR(). Thus the existing check is already good. > + if (!IS_ERR(log_root->node)) > + free_extent_buffer(log_root->node); The same for free_extent_buffer(); Thanks, Qu > free(log_root); > fs_info->log_root_tree = NULL; > return -EIO; > } > + fs_info->log_root_tree = log_root; > > return 0; > } >
On 12.08.21 г. 11:51, Qu Wenruo wrote: > > > On 2021/8/12 下午4:16, Nikolay Borisov wrote: >> read_tree_block can return an error due to a variety of reason, >> currently its return value is not being checked when loading the >> log root's node but is directly used in a call to >> extent_buffer_uptodate. This can lead to a crash if read_tree_block >> errored out, since the node won't be a pointer but an error value cast >> to a pointer. >> >> Fix this by properly checking the return value of read_tree_block before >> utilising the value for anything else. >> --- >> kernel-shared/disk-io.c | 9 ++++----- >> 1 file changed, 4 insertions(+), 5 deletions(-) >> >> diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c >> index cc635152c46d..4b5436254671 100644 >> --- a/kernel-shared/disk-io.c >> +++ b/kernel-shared/disk-io.c >> @@ -629,15 +629,14 @@ static int find_and_setup_log_root(struct >> btrfs_root *tree_root, >> >> log_root->node = read_tree_block(fs_info, blocknr, >> btrfs_super_generation(disk_super) + 1); >> - >> - fs_info->log_root_tree = log_root; >> - >> - if (!extent_buffer_uptodate(log_root->node)) { >> - free_extent_buffer(log_root->node); >> + if (IS_ERR(log_root->node) || >> !extent_buffer_uptodate(log_root->node)) { > > extent_buffer_uptodate() already has check for IS_ERROR(). > > Thus the existing check is already good. > >> + if (!IS_ERR(log_root->node)) >> + free_extent_buffer(log_root->node); > > The same for free_extent_buffer(); You are right, so indeed it's not an issue but this is somewhat hidden. So this patch can be ignored. > > Thanks, > Qu >> free(log_root); >> fs_info->log_root_tree = NULL; >> return -EIO; >> } >> + fs_info->log_root_tree = log_root; >> >> return 0; >> } >> >
diff --git a/kernel-shared/disk-io.c b/kernel-shared/disk-io.c index cc635152c46d..4b5436254671 100644 --- a/kernel-shared/disk-io.c +++ b/kernel-shared/disk-io.c @@ -629,15 +629,14 @@ static int find_and_setup_log_root(struct btrfs_root *tree_root, log_root->node = read_tree_block(fs_info, blocknr, btrfs_super_generation(disk_super) + 1); - - fs_info->log_root_tree = log_root; - - if (!extent_buffer_uptodate(log_root->node)) { - free_extent_buffer(log_root->node); + if (IS_ERR(log_root->node) || !extent_buffer_uptodate(log_root->node)) { + if (!IS_ERR(log_root->node)) + free_extent_buffer(log_root->node); free(log_root); fs_info->log_root_tree = NULL; return -EIO; } + fs_info->log_root_tree = log_root; return 0; }