[PATCH 0/4] fs: btrfs: coverity fixes
Most of the coverity reports are at least a good hint to us to improve the code style. Although there are some false alerts, but it's really hard to teach coverity to be a real human (yet), thus fix the warning even it's just false alerts.
Qu Wenruo (4): fs: btrfs: inode: handle uninitialized type before returning it fs: btrfs: volumes: prevent overflow for multiplying fs: btrfs: initialize @ret to 0 to prevent uninitialized return value fs: btrfs: initialize @ii in show_dir() to make coverity happy
fs/btrfs/btrfs.c | 4 ++-- fs/btrfs/inode.c | 6 +++++- fs/btrfs/volumes.c | 4 ++-- 3 files changed, 9 insertions(+), 5 deletions(-)
In btrfs_lookup_path() the local variable @type should always be updated after we hit any file/dir.
But if @filename is NULL from the very beginning, then we don't initialize it and return it directly.
To prevent such problem from happening, we initialize @type to BTRFS_FT_UNKNOWN. For normal execution route, it will get updated for each filename we resolved. Buf if we didn't find any path, we check if the type is still FT_UNKNOWN and ret == 0. If true we know there is something wrong, just return -EUCLEAN to inform the caller.
Reported-by: Coverity CID 312958 Signed-off-by: Qu Wenruo wqu@suse.com --- fs/btrfs/inode.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index ff330280e025..019d532a1a4b 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -251,7 +251,7 @@ int btrfs_lookup_path(struct btrfs_root *root, u64 ino, const char *filename, const char *cur = filename; u64 next_ino; u8 next_type; - u8 type; + u8 type = BTRFS_FT_UNKNOWN; int len; int ret = 0;
@@ -335,6 +335,10 @@ next: cur += len; }
+ /* We haven't found anything, but still get no error? */ + if (type == BTRFS_FT_UNKNOWN && !ret) + ret = -EUCLEAN; + if (!ret) { *root_ret = root; *ino_ret = ino;
Reviewed-by: Marek Behún marek.behun@nic.cz
On Sat, Oct 31, 2020 at 09:07:49AM +0800, Qu Wenruo wrote:
In btrfs_lookup_path() the local variable @type should always be updated after we hit any file/dir.
But if @filename is NULL from the very beginning, then we don't initialize it and return it directly.
To prevent such problem from happening, we initialize @type to BTRFS_FT_UNKNOWN. For normal execution route, it will get updated for each filename we resolved. Buf if we didn't find any path, we check if the type is still FT_UNKNOWN and ret == 0. If true we know there is something wrong, just return -EUCLEAN to inform the caller.
Reported-by: Coverity CID 312958 Signed-off-by: Qu Wenruo wqu@suse.com Reviewed-by: Marek Behún marek.behun@nic.cz
Applied to u-boot/master, thanks!
In __btrfs_map_block() we do a int * int and assign it to u64. This is not safe as the result (int * int) is still evaluated as (int) thus it can overflow.
Convert one of the multiplier to u64 to prevent such problem.
In real world, this should not cause problem as we have device number limit thus it won't go beyond 4G for a single stripe.
But it's harder to teach coverity about all these hidden limits, so just fix the possible overflow.
Reported-by: Coverity CID 312957 Reported-by: Coverity CID 312948 Signed-off-by: Qu Wenruo wqu@suse.com --- fs/btrfs/volumes.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c index fcf52d4b0ff3..4aaaeab663f5 100644 --- a/fs/btrfs/volumes.c +++ b/fs/btrfs/volumes.c @@ -1030,7 +1030,7 @@ again: */ stripe_nr = stripe_nr / map->stripe_len;
- stripe_offset = stripe_nr * map->stripe_len; + stripe_offset = stripe_nr * (u64)map->stripe_len; BUG_ON(offset < stripe_offset);
/* stripe_offset is the offset of this block in its stripe*/ @@ -1103,7 +1103,7 @@ again: rot = stripe_nr % map->num_stripes;
/* Fill in the logical address of each stripe */ - tmp = stripe_nr * nr_data_stripes(map); + tmp = (u64)stripe_nr * nr_data_stripes(map);
for (i = 0; i < nr_data_stripes(map); i++) raid_map[(i+rot) % map->num_stripes] =
On Sat, 31 Oct 2020 09:07:50 +0800 Qu Wenruo wqu@suse.com wrote:
In real world, this should not cause problem as we have device number limit thus it won't go beyond 4G for a single stripe.
So we can't run into this overflow in U-Boot because only one device is supported? But in Linux we can run into this issue?
On 2020/11/2 上午7:02, Marek Behun wrote:
On Sat, 31 Oct 2020 09:07:50 +0800 Qu Wenruo wqu@suse.com wrote:
In real world, this should not cause problem as we have device number limit thus it won't go beyond 4G for a single stripe.
So we can't run into this overflow in U-Boot because only one device is supported? But in Linux we can run into this issue?
In kernel, we have device number check, IIRC for default nodesize is just several donzens of devices. And since each stripe is only 64K fixed in btrfs, you can see it won't go beyond 4G even in kernel.
Thanks, Qu
On 2020/11/2 上午8:20, Qu Wenruo wrote:
On 2020/11/2 上午7:02, Marek Behun wrote:
On Sat, 31 Oct 2020 09:07:50 +0800 Qu Wenruo wqu@suse.com wrote:
In real world, this should not cause problem as we have device number limit thus it won't go beyond 4G for a single stripe.
So we can't run into this overflow in U-Boot because only one device is supported? But in Linux we can run into this issue?
In kernel, we have device number check, IIRC for default nodesize is just several donzens of devices. And since each stripe is only 64K fixed in btrfs, you can see it won't go beyond 4G even in kernel.
Sorry, the 64K stripe_len is only for RAID56, and for kernel, we always use u64 for stripe_len, thus we won't hit the problem.
The problem is in btrfs-progs, where the code comes from, we use int, other than u64.
Thanks for the hint for the root cause, would related values to be u64 for both btrfs-progs and u-boot to follow the kernel scheme.
Thanks, Qu
Thanks, Qu
On Sat, Oct 31, 2020 at 09:07:50AM +0800, Qu Wenruo wrote:
In __btrfs_map_block() we do a int * int and assign it to u64. This is not safe as the result (int * int) is still evaluated as (int) thus it can overflow.
Convert one of the multiplier to u64 to prevent such problem.
In real world, this should not cause problem as we have device number limit thus it won't go beyond 4G for a single stripe.
But it's harder to teach coverity about all these hidden limits, so just fix the possible overflow.
Reported-by: Coverity CID 312957 Reported-by: Coverity CID 312948 Signed-off-by: Qu Wenruo wqu@suse.com
Applied to u-boot/master, thanks!
In show_dir() if we hit a ROOT_ITEM, we can exit with uninitialized @ret.
Fix it by initializing it to 0.
Reported-by: Coverity CID 312955 Signed-off-by: Qu Wenruo wqu@suse.com --- fs/btrfs/btrfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/btrfs/btrfs.c b/fs/btrfs/btrfs.c index e48972ffa21a..346b2c4341c2 100644 --- a/fs/btrfs/btrfs.c +++ b/fs/btrfs/btrfs.c @@ -36,7 +36,7 @@ static int show_dir(struct btrfs_root *root, struct extent_buffer *eb, char *target = NULL; char filetime[32]; time_t mtime; - int ret; + int ret = 0;
btrfs_dir_item_key_to_cpu(eb, di, &key);
Reviewed-by: Marek Behún marek.behun@nic.cz
On Sat, Oct 31, 2020 at 09:07:51AM +0800, Qu Wenruo wrote:
In show_dir() if we hit a ROOT_ITEM, we can exit with uninitialized @ret.
Fix it by initializing it to 0.
Reported-by: Coverity CID 312955 Signed-off-by: Qu Wenruo wqu@suse.com Reviewed-by: Marek Behún marek.behun@nic.cz
Applied to u-boot/master, thanks!
In show_dir() if we hit file type FT_CHRDEV or FT_BLKDEV, we expect an BTRFS_INODE_ITEM_KEY, and for that case, we should have @ii filled with data read from disk.
We even have ASSERT() for this purpose, but unfortunately coverity can't understand the ASSERT() nor if we get key type BTRFS_INODE_ITEM_KEY, previous if() branch must go to the read_extent_buffer() branch to fill the @ii.
So to make coverity happy, just initialize the variable @ii to all zero.
Reported-by: Coverity CID 312950 Signed-off-by: Qu Wenruo wqu@suse.com --- fs/btrfs/btrfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/fs/btrfs/btrfs.c b/fs/btrfs/btrfs.c index 346b2c4341c2..1abd2c291177 100644 --- a/fs/btrfs/btrfs.c +++ b/fs/btrfs/btrfs.c @@ -19,7 +19,7 @@ static int show_dir(struct btrfs_root *root, struct extent_buffer *eb, struct btrfs_dir_item *di) { struct btrfs_fs_info *fs_info = root->fs_info; - struct btrfs_inode_item ii; + struct btrfs_inode_item ii = { 0 }; struct btrfs_key key; static const char* dir_item_str[] = { [BTRFS_FT_REG_FILE] = "FILE",
On Sat, 31 Oct 2020 09:07:52 +0800 Qu Wenruo wqu@suse.com wrote:
In show_dir() if we hit file type FT_CHRDEV or FT_BLKDEV, we expect an BTRFS_INODE_ITEM_KEY, and for that case, we should have @ii filled with data read from disk.
We even have ASSERT() for this purpose, but unfortunately coverity can't understand the ASSERT() nor if we get key type BTRFS_INODE_ITEM_KEY, previous if() branch must go to the read_extent_buffer() branch to fill the @ii.
So to make coverity happy, just initialize the variable @ii to all zero.
WTF. If ASSERT excludes this from happening, coverity should understand this. We live in 2020. I don't much like the idea to do things just to get coverity happy if it can't understand and infer from things like ASSERT.
On 2020/11/2 上午7:06, Marek Behun wrote:
On Sat, 31 Oct 2020 09:07:52 +0800 Qu Wenruo wqu@suse.com wrote:
In show_dir() if we hit file type FT_CHRDEV or FT_BLKDEV, we expect an BTRFS_INODE_ITEM_KEY, and for that case, we should have @ii filled with data read from disk.
We even have ASSERT() for this purpose, but unfortunately coverity can't understand the ASSERT() nor if we get key type BTRFS_INODE_ITEM_KEY, previous if() branch must go to the read_extent_buffer() branch to fill the @ii.
So to make coverity happy, just initialize the variable @ii to all zero.
WTF. If ASSERT excludes this from happening, coverity should understand this. We live in 2020. I don't much like the idea to do things just to get coverity happy if it can't understand and infer from things like ASSERT.
It's not ASSERT(), it's the previous if () branch. The ASSERT() is just to ensure we didn't miss anything.
The previous if () looks like this: if (key.type == BTRFS_ROOT_ITEM_KEY) { /* @ii is not touched */ /* Further more, in this branch type should only be FT_DIR */ } else { /* This includes the key.type == INODE_ITEM_KEY case */ read_extent_buffer( &ii ); /* Here we fill &ii */ }
Then in the offending code we have: if (type == BTRFS_FT_CHRDEV || type == BTRFS_FT_BLKDEV) { ASSERT(key.type == BTRFS_INODE_ITEM_KEY); /* * If the type is above type, we must have key type INODE_ITEM * Thus the ii must be filled. */ printf("%4llu,%5llu ", btrfs_stack_inode_rdev(&ii) >> 20, btrfs_stack_inode_rdev(&ii) & 0xfffff); } else { if (key.type == BTRFS_INODE_ITEM_KEY) printf("%10llu ", btrfs_stack_inode_size(&ii)); else printf("%10llu ", 0ULL); }
Thus I really tend to believe it's just a bug in coverity. All locations accessing @ii all have its key.type checked to ensure it get filled in the first place.
Thanks, Qu
On Mon, 2 Nov 2020 08:27:16 +0800 Qu Wenruo wqu@suse.com wrote:
Thus I really tend to believe it's just a bug in coverity. All locations accessing @ii all have its key.type checked to ensure it get filled in the first place.
If this is a bug in coverity, we should fix coverity, not add extra code to U-Boot, even if it is just one instruction. That simply stinks the same way like when systemd crashed when "debug" parameter was present in /proc/cmdline, and they sent a patch to kernel which removed the "debug" parameter, instead of fixing systemd. IMO.
Marek
On 2020/11/2 下午3:24, Marek Behun wrote:
On Mon, 2 Nov 2020 08:27:16 +0800 Qu Wenruo wqu@suse.com wrote:
Thus I really tend to believe it's just a bug in coverity. All locations accessing @ii all have its key.type checked to ensure it get filled in the first place.
If this is a bug in coverity, we should fix coverity, not add extra code to U-Boot, even if it is just one instruction. That simply stinks the same way like when systemd crashed when "debug" parameter was present in /proc/cmdline, and they sent a patch to kernel which removed the "debug" parameter, instead of fixing systemd. IMO.
Yep, you're right.
Please discard this patch.
Thanks, Qu
Marek
On Mon, Nov 02, 2020 at 08:24:09AM +0100, Marek Behun wrote:
On Mon, 2 Nov 2020 08:27:16 +0800 Qu Wenruo wqu@suse.com wrote:
Thus I really tend to believe it's just a bug in coverity. All locations accessing @ii all have its key.type checked to ensure it get filled in the first place.
If this is a bug in coverity, we should fix coverity, not add extra code to U-Boot, even if it is just one instruction. That simply stinks the same way like when systemd crashed when "debug" parameter was present in /proc/cmdline, and they sent a patch to kernel which removed the "debug" parameter, instead of fixing systemd. IMO.
To be clear, I am quite happy to mark as intentional any issues that are flagged by Coverity but, well, intentional and not a problem. We may also benefit from adding a "modeling" file to help Coverity know when some cases of issues are handled by us already.
participants (3)
-
Marek Behun -
Qu Wenruo -
Tom Rini