| Message ID | 20200803093506.GA19472@amd (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix error value in btrfs_get_extent | expand |
On 3.08.20 г. 12:35 ч., Pavel Machek wrote: > btrfs_get_extent() sets variable ret, but out: error path expect error > to be in variable err. Fix that. > > Signed-off-by: Pavel Machek (CIP) <pavel@denx.de> Good catch, this also needs: Fixes: 6bf9e4bd6a27 ("btrfs: inode: Verify inode mode to avoid NULL pointer dereference") Reviewed-by: Nikolay Borisov <nborisov@suse.com> > > --- > > Notice that patch introducing this problem is on its way to 4.19.137-stable. > > diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c > index 7befb7c12bd3..4aaa01540f89 100644 > --- a/fs/btrfs/inode.c > +++ b/fs/btrfs/inode.c > @@ -7012,7 +7012,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode, > found_type == BTRFS_FILE_EXTENT_PREALLOC) { > /* Only regular file could have regular/prealloc extent */ > if (!S_ISREG(inode->vfs_inode.i_mode)) { > - ret = -EUCLEAN; > + err = -EUCLEAN; > btrfs_crit(fs_info, > "regular/prealloc extent found for non-regular inode %llu", > btrfs_ino(inode)); >
On 3.08.20 г. 12:39 ч., Nikolay Borisov wrote: > > > On 3.08.20 г. 12:35 ч., Pavel Machek wrote: >> btrfs_get_extent() sets variable ret, but out: error path expect error >> to be in variable err. Fix that. >> >> Signed-off-by: Pavel Machek (CIP) <pavel@denx.de> > > Good catch, this also needs: > > Fixes: 6bf9e4bd6a27 ("btrfs: inode: Verify inode mode to avoid NULL > pointer dereference") > > Reviewed-by: Nikolay Borisov <nborisov@suse.com> Actually the reason this error got introduced in the first place and I missed it during the review is that the function is doing something rather counter-intuitive - it's using 'err' variable as a synonym for 'ret'. A better approach would be to simply remove 'err' from that function. I'm now authoring such a patch, nevertheless the issue still stands. > > >> >> --- >> >> Notice that patch introducing this problem is on its way to 4.19.137-stable. >> >> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c >> index 7befb7c12bd3..4aaa01540f89 100644 >> --- a/fs/btrfs/inode.c >> +++ b/fs/btrfs/inode.c >> @@ -7012,7 +7012,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode, >> found_type == BTRFS_FILE_EXTENT_PREALLOC) { >> /* Only regular file could have regular/prealloc extent */ >> if (!S_ISREG(inode->vfs_inode.i_mode)) { >> - ret = -EUCLEAN; >> + err = -EUCLEAN; >> btrfs_crit(fs_info, >> "regular/prealloc extent found for non-regular inode %llu", >> btrfs_ino(inode)); >> >
On Mon, Aug 03, 2020 at 11:35:06AM +0200, Pavel Machek wrote: > btrfs_get_extent() sets variable ret, but out: error path expect error > to be in variable err. Fix that. > > Signed-off-by: Pavel Machek (CIP) <pavel@denx.de> Thanks, patch queued for 5.9. > > --- > > Notice that patch introducing this problem is on its way to 4.19.137-stable. Yeah, I'll let stable team know once this patch gets merged that the relevant patches can be picked.
On Mon, Aug 03, 2020 at 12:50:31PM +0300, Nikolay Borisov wrote: > On 3.08.20 г. 12:39 ч., Nikolay Borisov wrote: > > On 3.08.20 г. 12:35 ч., Pavel Machek wrote: > >> btrfs_get_extent() sets variable ret, but out: error path expect error > >> to be in variable err. Fix that. > >> > >> Signed-off-by: Pavel Machek (CIP) <pavel@denx.de> > > > > Good catch, this also needs: > > > > Fixes: 6bf9e4bd6a27 ("btrfs: inode: Verify inode mode to avoid NULL > > pointer dereference") > > > > Reviewed-by: Nikolay Borisov <nborisov@suse.com> > > Actually the reason this error got introduced in the first place and I > missed it during the review is that the function is doing something > rather counter-intuitive - it's using 'err' variable as a synonym for > 'ret'. A better approach would be to simply remove 'err' from that > function. I'm now authoring such a patch, nevertheless the issue still > stands. The expected pattern is to use 'ret' for function return value and add other temporary variables instead of the err/ret switching, which can be found in the oldish code still. So the cleanup is going to do the right thing, thanks.
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 7befb7c12bd3..4aaa01540f89 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -7012,7 +7012,7 @@ struct extent_map *btrfs_get_extent(struct btrfs_inode *inode, found_type == BTRFS_FILE_EXTENT_PREALLOC) { /* Only regular file could have regular/prealloc extent */ if (!S_ISREG(inode->vfs_inode.i_mode)) { - ret = -EUCLEAN; + err = -EUCLEAN; btrfs_crit(fs_info, "regular/prealloc extent found for non-regular inode %llu", btrfs_ino(inode));
btrfs_get_extent() sets variable ret, but out: error path expect error to be in variable err. Fix that. Signed-off-by: Pavel Machek (CIP) <pavel@denx.de> --- Notice that patch introducing this problem is on its way to 4.19.137-stable.