diff mbox

btrfs: qgroups: Fix BUG_ON condition

Message ID 1499841739-18549-1-git-send-email-nborisov@suse.com (mailing list archive)
State New, archived
Headers show

Commit Message

Nikolay Borisov July 12, 2017, 6:42 a.m. UTC 
The current code was erroneously checking for root_level > BTRFS_MAX_LEVEL. If
we had a root_level of 8 then the check won't trigger and we could
potentially hit a buffer overflow. The correct check should be
root_level >= BTRFS_MAX_LEVEL

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/qgroup.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Qu Wenruo July 12, 2017, 7:09 a.m. UTC | #1 
在 2017年07月12日 14:42, Nikolay Borisov 写道:
> The current code was erroneously checking for root_level > BTRFS_MAX_LEVEL. If
> we had a root_level of 8 then the check won't trigger and we could
> potentially hit a buffer overflow. The correct check should be
> root_level >= BTRFS_MAX_LEVEL

Thanks for catching this.

Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>

> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/qgroup.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 4ce351efe281..3b787915ef31 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -1603,7 +1603,7 @@ int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans,
>   	struct extent_buffer *eb = root_eb;
>   	struct btrfs_path *path = NULL;
>   
> -	BUG_ON(root_level < 0 || root_level > BTRFS_MAX_LEVEL);
> +	BUG_ON(root_level < 0 || root_level >= BTRFS_MAX_LEVEL);
>   	BUG_ON(root_eb == NULL);
>   
>   	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
> @@ -2959,7 +2959,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);

I didn't recongize it's a tailing white space at first.
Nice catch.

Thanks,
Qu

>   	if (ret < 0)
>   		goto out;
> 
--
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
David Sterba July 12, 2017, 1:42 p.m. UTC | #2 
On Wed, Jul 12, 2017 at 03:09:42PM +0800, Qu Wenruo wrote:
> 
> 
> 在 2017年07月12日 14:42, Nikolay Borisov 写道:
> > The current code was erroneously checking for root_level > BTRFS_MAX_LEVEL. If
> > we had a root_level of 8 then the check won't trigger and we could
> > potentially hit a buffer overflow. The correct check should be
> > root_level >= BTRFS_MAX_LEVEL
> 
> Thanks for catching this.
> 
> Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
> 
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > ---
> >   fs/btrfs/qgroup.c | 4 ++--
> >   1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> > index 4ce351efe281..3b787915ef31 100644
> > --- a/fs/btrfs/qgroup.c
> > +++ b/fs/btrfs/qgroup.c
> > @@ -1603,7 +1603,7 @@ int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans,
> >   	struct extent_buffer *eb = root_eb;
> >   	struct btrfs_path *path = NULL;
> >   
> > -	BUG_ON(root_level < 0 || root_level > BTRFS_MAX_LEVEL);
> > +	BUG_ON(root_level < 0 || root_level >= BTRFS_MAX_LEVEL);
> >   	BUG_ON(root_eb == NULL);
> >   
> >   	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
> > @@ -2959,7 +2959,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);
> 
> I didn't recongize it's a tailing white space at first.

The original code is from you, so please configure your editor to
hilight trailing whitespace. Whitespace damage happens, git am warns
about tha but git cherry-pick does not.

> Nice catch.

So before we start seeing patches that fix random whitespace in
unrelated code: please don't do that.

As you wrote, it was not obvious that there was no change on the line,
this just slowed down reading the patch.
--
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
Nikolay Borisov July 12, 2017, 1:50 p.m. UTC | #3 
On 12.07.2017 16:42, David Sterba wrote:
> On Wed, Jul 12, 2017 at 03:09:42PM +0800, Qu Wenruo wrote:
>>
>>
>> 在 2017年07月12日 14:42, Nikolay Borisov 写道:
>>> The current code was erroneously checking for root_level > BTRFS_MAX_LEVEL. If
>>> we had a root_level of 8 then the check won't trigger and we could
>>> potentially hit a buffer overflow. The correct check should be
>>> root_level >= BTRFS_MAX_LEVEL
>>
>> Thanks for catching this.
>>
>> Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
>>
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>> ---
>>>   fs/btrfs/qgroup.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index 4ce351efe281..3b787915ef31 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -1603,7 +1603,7 @@ int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans,
>>>   	struct extent_buffer *eb = root_eb;
>>>   	struct btrfs_path *path = NULL;
>>>   
>>> -	BUG_ON(root_level < 0 || root_level > BTRFS_MAX_LEVEL);
>>> +	BUG_ON(root_level < 0 || root_level >= BTRFS_MAX_LEVEL);
>>>   	BUG_ON(root_eb == NULL);
>>>   
>>>   	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
>>> @@ -2959,7 +2959,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);
>>
>> I didn't recongize it's a tailing white space at first.
> 
> The original code is from you, so please configure your editor to
> hilight trailing whitespace. Whitespace damage happens, git am warns
> about tha but git cherry-pick does not.
> 
>> Nice catch.
> 
> So before we start seeing patches that fix random whitespace in
> unrelated code: please don't do that.
> 
> As you wrote, it was not obvious that there was no change on the line,
> this just slowed down reading the patch.

I didn't intentionally fix this, I've configured vi so as to
automatically do this. There is also whitespace damage on a particular
line in extent-tree.c and every time I submit a patch that touches this
file I explicitly have to omit that particular hunk.

How would you feel about me sending a patch fixing those 2 whitespace
damages?

> 
--
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
David Sterba July 12, 2017, 1:51 p.m. UTC | #4 
On Wed, Jul 12, 2017 at 04:50:20PM +0300, Nikolay Borisov wrote:
> > As you wrote, it was not obvious that there was no change on the line,
> > this just slowed down reading the patch.
> 
> I didn't intentionally fix this, I've configured vi so as to
> automatically do this. There is also whitespace damage on a particular
> line in extent-tree.c and every time I submit a patch that touches this
> file I explicitly have to omit that particular hunk.
> 
> How would you feel about me sending a patch fixing those 2 whitespace
> damages?

Can't you fix your editor not to auto-correct? :)
--
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
Qu Wenruo July 12, 2017, 2 p.m. UTC | #5 
On 2017年07月12日 21:42, David Sterba wrote:
> On Wed, Jul 12, 2017 at 03:09:42PM +0800, Qu Wenruo wrote:
>>
>>
>> 在 2017年07月12日 14:42, Nikolay Borisov 写道:
>>> The current code was erroneously checking for root_level > BTRFS_MAX_LEVEL. If
>>> we had a root_level of 8 then the check won't trigger and we could
>>> potentially hit a buffer overflow. The correct check should be
>>> root_level >= BTRFS_MAX_LEVEL
>>
>> Thanks for catching this.
>>
>> Reviewed-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
>>
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>> ---
>>>    fs/btrfs/qgroup.c | 4 ++--
>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>>> index 4ce351efe281..3b787915ef31 100644
>>> --- a/fs/btrfs/qgroup.c
>>> +++ b/fs/btrfs/qgroup.c
>>> @@ -1603,7 +1603,7 @@ int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans,
>>>    	struct extent_buffer *eb = root_eb;
>>>    	struct btrfs_path *path = NULL;
>>>    
>>> -	BUG_ON(root_level < 0 || root_level > BTRFS_MAX_LEVEL);
>>> +	BUG_ON(root_level < 0 || root_level >= BTRFS_MAX_LEVEL);
>>>    	BUG_ON(root_eb == NULL);
>>>    
>>>    	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
>>> @@ -2959,7 +2959,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);
>>
>> I didn't recongize it's a tailing white space at first.
> 
> The original code is from you, so please configure your editor to
> hilight trailing whitespace. Whitespace damage happens, git am warns
> about tha but git cherry-pick does not.

Well, I should make send-email to automatically to run checkpatch.

Sometimes I forgot to run checkpatch manually and will cause such damage.
Really sorry for that.

Thanks,
Qu

> 
>> Nice catch.
> 
> So before we start seeing patches that fix random whitespace in
> unrelated code: please don't do that.
> 
> As you wrote, it was not obvious that there was no change on the line,
> this just slowed down reading the patch.
> 
--
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
Qu Wenruo July 12, 2017, 2:11 p.m. UTC | #6 
On 2017年07月12日 21:51, David Sterba wrote:
> On Wed, Jul 12, 2017 at 04:50:20PM +0300, Nikolay Borisov wrote:
>>> As you wrote, it was not obvious that there was no change on the line,
>>> this just slowed down reading the patch.
>>
>> I didn't intentionally fix this, I've configured vi so as to
>> automatically do this. There is also whitespace damage on a particular
>> line in extent-tree.c and every time I submit a patch that touches this
>> file I explicitly have to omit that particular hunk.
>>
>> How would you feel about me sending a patch fixing those 2 whitespace
>> damages?
> 
> Can't you fix your editor not to auto-correct? :)

At least for tailing white space, there are only 4  in v4.12, including 
this one.

Why not fixing it in one patch so we don't need to bother them any longer?

Thanks,
Qu
--
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
diff mbox

Patch

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 4ce351efe281..3b787915ef31 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -1603,7 +1603,7 @@  int btrfs_qgroup_trace_subtree(struct btrfs_trans_handle *trans,
 	struct extent_buffer *eb = root_eb;
 	struct btrfs_path *path = NULL;
 
-	BUG_ON(root_level < 0 || root_level > BTRFS_MAX_LEVEL);
+	BUG_ON(root_level < 0 || root_level >= BTRFS_MAX_LEVEL);
 	BUG_ON(root_eb == NULL);
 
 	if (!test_bit(BTRFS_FS_QUOTA_ENABLED, &fs_info->flags))
@@ -2959,7 +2959,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;