[PATCH 0/3] Couple of coverity fixes

[PATCH 0/3] Couple of coverity fixes

[Nikolay Borisov]

Here are 3 patches based on the latest coverity defect scans. Mostly minor, 
quality-of-life type of fixes. One redundant assignment removal, 1 bounds 
checking in qgroup and 1 possible overflow in qgroups please merge.  

Nikolay Borisov (3):
  btrfs: Remove unused -EIO assignment in end_bio_extent_readpage
  btrfs: Fix bound checking in qgroup_trace_new_subtree_blocks
  btrfs: Avoid possible qgroup_rsv_size overflow in
    btrfs_calculate_inode_block_rsv_size

 fs/btrfs/extent-tree.c | 2 +-
 fs/btrfs/extent_io.c   | 2 --
 fs/btrfs/qgroup.c      | 6 +++---
 3 files changed, 4 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH 1/3] btrfs: Remove unused -EIO assignment in end_bio_extent_readpage

[Nikolay Borisov]

In case we hit the error case for a metadata buffer in
end_bio_extent_readpage then 'ret' won't really be checked before it's
written again to. This means the -EIO in this case will never be
checked, just remove it.

Fixes-coverity-id: 1442513
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent_io.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index a04e0328f1bb..49af6d2a2a11 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2705,8 +2705,6 @@ static void end_bio_extent_readpage(struct bio *bio)
 			if (test_and_clear_bit(EXTENT_BUFFER_READAHEAD,
 					       &eb->bflags))
 				btree_readahead_hook(eb, -EIO);
-
-			ret = -EIO;
 		}
 readpage_ok:
 		if (likely(uptodate)) {
-- 
2.17.1

[PATCH 2/3] btrfs: Fix bound checking in qgroup_trace_new_subtree_blocks

[Nikolay Borisov]

If 'cur_level' is 7  then the bound checking at the top of the function
will actually pass. Later on, it's possible to dereference
ds_path->nodes[cur_level+1] which will be an out of bounds.

The correct check will be cur_level >= BTRFS_MAX_LEVEL - 1 .

Fixes-coverty-id: 1440918
Fixes-coverty-id: 1440911
Fixes: ea49f3e73c4b ("btrfs: qgroup: Introduce function to find all new tree blocks of reloc tree")
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/qgroup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index eb680b715dd6..7019edf5625c 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1922,8 +1922,8 @@ static int qgroup_trace_new_subtree_blocks(struct btrfs_trans_handle* trans,
 	int i;
 
 	/* Level sanity check */
-	if (cur_level < 0 || cur_level >= BTRFS_MAX_LEVEL ||
-	    root_level < 0 || root_level >= BTRFS_MAX_LEVEL ||
+	if (cur_level < 0 || cur_level >= BTRFS_MAX_LEVEL - 1 ||
+	    root_level < 0 || root_level >= BTRFS_MAX_LEVEL - 1 ||
 	    root_level < cur_level) {
 		btrfs_err_rl(fs_info,
 			"%s: bad levels, cur_level=%d root_level=%d",
@@ -3482,7 +3482,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
 	if (free && reserved)
 		return qgroup_free_reserved_data(inode, reserved, start, len);
 	extent_changeset_init(&changeset);
-	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start, 
+	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start,
 			start + len -1, EXTENT_QGROUP_RESERVED, &changeset);
 	if (ret < 0)
 		goto out;
-- 
2.17.1

[PATCH 3/3] btrfs: Avoid possible qgroup_rsv_size overflow in btrfs_calculate_inode_block_rsv_size

[Nikolay Borisov]

qgroup_rsv_size is calculated as the product of
outstanding_extent * fs_info->nodesize. The product is calculated with
32 bith precision since both variables are defined as u32. Yet
qgroup_rsv_size expects a 64 bit result.

Avoid possible multiplication overflow by casting outstanding_extent to
u64.

Fixes-coverity-id: 1435101
ff6bc37eb7f6 ("btrfs: qgroup: Use independent and accurate per inode qgroup rsv")

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index b085d8215f0e..beddf9eef4a2 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -6173,7 +6173,7 @@ static void btrfs_calculate_inode_block_rsv_size(struct btrfs_fs_info *fs_info,
 	 *
 	 * This is overestimating in most cases.
 	 */
-	qgroup_rsv_size = outstanding_extents * fs_info->nodesize;
+	qgroup_rsv_size = (u64) outstanding_extents * fs_info->nodesize;
 
 	spin_lock(&block_rsv->lock);
 	block_rsv->size = reserve_size;
-- 
2.17.1

Re: [PATCH 2/3] btrfs: Fix bound checking in qgroup_trace_new_subtree_blocks

[Qu Wenruo]

On 2019/3/18 下午11:45, Nikolay Borisov wrote:
> If 'cur_level' is 7  then the bound checking at the top of the function
> will actually pass. Later on, it's possible to dereference
> ds_path->nodes[cur_level+1] which will be an out of bounds.
> 
> The correct check will be cur_level >= BTRFS_MAX_LEVEL - 1 .
> 
> Fixes-coverty-id: 1440918
> Fixes-coverty-id: 1440911
> Fixes: ea49f3e73c4b ("btrfs: qgroup: Introduce function to find all new tree blocks of reloc tree")
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  fs/btrfs/qgroup.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index eb680b715dd6..7019edf5625c 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1922,8 +1922,8 @@ static int qgroup_trace_new_subtree_blocks(struct btrfs_trans_handle* trans,
>  	int i;
>  
>  	/* Level sanity check */
> -	if (cur_level < 0 || cur_level >= BTRFS_MAX_LEVEL ||
> -	    root_level < 0 || root_level >= BTRFS_MAX_LEVEL ||
> +	if (cur_level < 0 || cur_level >= BTRFS_MAX_LEVEL - 1 ||
> +	    root_level < 0 || root_level >= BTRFS_MAX_LEVEL - 1 ||
>  	    root_level < cur_level) {
>  		btrfs_err_rl(fs_info,
>  			"%s: bad levels, cur_level=%d root_level=%d",
> @@ -3482,7 +3482,7 @@ static int __btrfs_qgroup_release_data(struct inode *inode,
>  	if (free && reserved)
>  		return qgroup_free_reserved_data(inode, reserved, start, len);
>  	extent_changeset_init(&changeset);
> -	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start, 
> +	ret = clear_record_extent_bits(&BTRFS_I(inode)->io_tree, start,
>  			start + len -1, EXTENT_QGROUP_RESERVED, &changeset);
>  	if (ret < 0)
>  		goto out;
> 

Re: [PATCH 3/3] btrfs: Avoid possible qgroup_rsv_size overflow in btrfs_calculate_inode_block_rsv_size

[Qu Wenruo]

On 2019/3/18 下午11:45, Nikolay Borisov wrote:
> qgroup_rsv_size is calculated as the product of
> outstanding_extent * fs_info->nodesize. The product is calculated with
> 32 bith precision since both variables are defined as u32. Yet
> qgroup_rsv_size expects a 64 bit result.
> 
> Avoid possible multiplication overflow by casting outstanding_extent to
> u64.
> 
> Fixes-coverity-id: 1435101
> ff6bc37eb7f6 ("btrfs: qgroup: Use independent and accurate per inode qgroup rsv")
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/extent-tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index b085d8215f0e..beddf9eef4a2 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -6173,7 +6173,7 @@ static void btrfs_calculate_inode_block_rsv_size(struct btrfs_fs_info *fs_info,
>  	 *
>  	 * This is overestimating in most cases.
>  	 */
> -	qgroup_rsv_size = outstanding_extents * fs_info->nodesize;
> +	qgroup_rsv_size = (u64) outstanding_extents * fs_info->nodesize;

I'm a little uncertain about what's the proper way to do a u32 * u32 and
get a u64 in C.

For division we have DIV macro but not for multiple.

Thanks,
Qu

>  
>  	spin_lock(&block_rsv->lock);
>  	block_rsv->size = reserve_size;
> 

Re: [PATCH 3/3] btrfs: Avoid possible qgroup_rsv_size overflow in btrfs_calculate_inode_block_rsv_size

[Nikolay Borisov]

On 19.03.19 г. 6:46 ч., Qu Wenruo wrote:
> 
> 
> On 2019/3/18 下午11:45, Nikolay Borisov wrote:
>> qgroup_rsv_size is calculated as the product of
>> outstanding_extent * fs_info->nodesize. The product is calculated with
>> 32 bith precision since both variables are defined as u32. Yet
>> qgroup_rsv_size expects a 64 bit result.
>>
>> Avoid possible multiplication overflow by casting outstanding_extent to
>> u64.
>>
>> Fixes-coverity-id: 1435101
>> ff6bc37eb7f6 ("btrfs: qgroup: Use independent and accurate per inode qgroup rsv")
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/extent-tree.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index b085d8215f0e..beddf9eef4a2 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -6173,7 +6173,7 @@ static void btrfs_calculate_inode_block_rsv_size(struct btrfs_fs_info *fs_info,
>>  	 *
>>  	 * This is overestimating in most cases.
>>  	 */
>> -	qgroup_rsv_size = outstanding_extents * fs_info->nodesize;
>> +	qgroup_rsv_size = (u64) outstanding_extents * fs_info->nodesize;
> 
> I'm a little uncertain about what's the proper way to do a u32 * u32 and
> get a u64 in C.
> 
> For division we have DIV macro but not for multiple.

You should definitely read this:

https://wiki.sei.cmu.edu/confluence/display/c/INT18-C.+Evaluate+integer+expressions+in+a+larger+size+before+comparing+or+assigning+to+that+size

In particular the 2nd "Noncompliant Code Example
" described there is exactly the case you have in this code.

> 
> Thanks,
> Qu
> 
>>  
>>  	spin_lock(&block_rsv->lock);
>>  	block_rsv->size = reserve_size;
>>
> 

Re: [PATCH 3/3] btrfs: Avoid possible qgroup_rsv_size overflow in btrfs_calculate_inode_block_rsv_size

[Qu Wenruo]

On 2019/3/19 下午2:48, Nikolay Borisov wrote:
> 
> 
> On 19.03.19 г. 6:46 ч., Qu Wenruo wrote:
>>
>>
>> On 2019/3/18 下午11:45, Nikolay Borisov wrote:
>>> qgroup_rsv_size is calculated as the product of
>>> outstanding_extent * fs_info->nodesize. The product is calculated with
>>> 32 bith precision since both variables are defined as u32. Yet
>>> qgroup_rsv_size expects a 64 bit result.
>>>
>>> Avoid possible multiplication overflow by casting outstanding_extent to
>>> u64.
>>>
>>> Fixes-coverity-id: 1435101
>>> ff6bc37eb7f6 ("btrfs: qgroup: Use independent and accurate per inode qgroup rsv")
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>> ---
>>>  fs/btrfs/extent-tree.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>>> index b085d8215f0e..beddf9eef4a2 100644
>>> --- a/fs/btrfs/extent-tree.c
>>> +++ b/fs/btrfs/extent-tree.c
>>> @@ -6173,7 +6173,7 @@ static void btrfs_calculate_inode_block_rsv_size(struct btrfs_fs_info *fs_info,
>>>  	 *
>>>  	 * This is overestimating in most cases.
>>>  	 */
>>> -	qgroup_rsv_size = outstanding_extents * fs_info->nodesize;
>>> +	qgroup_rsv_size = (u64) outstanding_extents * fs_info->nodesize;
>>
>> I'm a little uncertain about what's the proper way to do a u32 * u32 and
>> get a u64 in C.
>>
>> For division we have DIV macro but not for multiple.
> 
> You should definitely read this:
> 
> https://wiki.sei.cmu.edu/confluence/display/c/INT18-C.+Evaluate+integer+expressions+in+a+larger+size+before+comparing+or+assigning+to+that+size
> 
> In particular the 2nd "Noncompliant Code Example
> " described there is exactly the case you have in this code.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> 
>>
>> Thanks,
>> Qu
>>
>>>  
>>>  	spin_lock(&block_rsv->lock);
>>>  	block_rsv->size = reserve_size;
>>>
>>

Re: [PATCH 1/3] btrfs: Remove unused -EIO assignment in end_bio_extent_readpage

[David Sterba]

On Mon, Mar 18, 2019 at 05:45:18PM +0200, Nikolay Borisov wrote:
> In case we hit the error case for a metadata buffer in
> end_bio_extent_readpage then 'ret' won't really be checked before it's
> written again to. This means the -EIO in this case will never be
> checked, just remove it.
> 
> Fixes-coverity-id: 1442513
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: David Sterba <dsterba@suse.com>

Re: [PATCH 3/3] btrfs: Avoid possible qgroup_rsv_size overflow in btrfs_calculate_inode_block_rsv_size

[David Sterba]

On Mon, Mar 18, 2019 at 05:45:20PM +0200, Nikolay Borisov wrote:
> qgroup_rsv_size is calculated as the product of
> outstanding_extent * fs_info->nodesize. The product is calculated with
> 32 bith precision since both variables are defined as u32. Yet
> qgroup_rsv_size expects a 64 bit result.
> 
> Avoid possible multiplication overflow by casting outstanding_extent to
> u64.
> 
> Fixes-coverity-id: 1435101
> ff6bc37eb7f6 ("btrfs: qgroup: Use independent and accurate per inode qgroup rsv")

Fixes: hash ("subject")

I've added a note about the worst case when the overflow would happen,
which is 65536 outstanding extents with 64K nodesize. Unlikely to
happen.

Re: [PATCH 0/3] Couple of coverity fixes

[David Sterba]

On Mon, Mar 18, 2019 at 05:45:17PM +0200, Nikolay Borisov wrote:
> Here are 3 patches based on the latest coverity defect scans. Mostly minor, 
> quality-of-life type of fixes. One redundant assignment removal, 1 bounds 
> checking in qgroup and 1 possible overflow in qgroups please merge.  
> 
> Nikolay Borisov (3):
>   btrfs: Remove unused -EIO assignment in end_bio_extent_readpage
>   btrfs: Fix bound checking in qgroup_trace_new_subtree_blocks
>   btrfs: Avoid possible qgroup_rsv_size overflow in
>     btrfs_calculate_inode_block_rsv_size

Added to misc-next, thanks.