[PATCH v2] btrfs-progs: common: make sure that qgroup id is in range

[PATCH v2] btrfs-progs: common: make sure that qgroup id is in range

[Sidong Yang]

When user assign qgroup with qgroup id that is too big to exceeds
range and invade level value, and it works without any error. but
this action would be make undefined error. this code make sure that
qgroup id doesn't exceed range(0 ~ 2^48-1).

Signed-off-by: Sidong Yang <realwakka@gmail.com>
---
v2:
  Use btrfs_qgroup_level() for checking
---
 common/utils.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/common/utils.c b/common/utils.c
index 57e41432..ba0bcb24 100644
--- a/common/utils.c
+++ b/common/utils.c
@@ -727,6 +727,8 @@ u64 parse_qgroupid(const char *p)
 		id = strtoull(p, &ptr_parse_end, 10);
 		if (ptr_parse_end != ptr_src_end)
 			goto path;
+		if (btrfs_qgroup_level(id))
+			goto err;
 		return id;
 	}
 	level = strtoull(p, &ptr_parse_end, 10);
@@ -734,6 +736,9 @@ u64 parse_qgroupid(const char *p)
 		goto path;
 
 	id = strtoull(s + 1, &ptr_parse_end, 10);
+	if (btrfs_qgroup_level(id))
+		goto err;
+
 	if (ptr_parse_end != ptr_src_end)
 		goto  path;
 
-- 
2.25.1

Re: [PATCH v2] btrfs-progs: common: make sure that qgroup id is in range

[Qu Wenruo]

On 2021/3/16 下午9:27, Sidong Yang wrote:
> When user assign qgroup with qgroup id that is too big to exceeds
> range and invade level value, and it works without any error. but
> this action would be make undefined error. this code make sure that
> qgroup id doesn't exceed range(0 ~ 2^48-1).
>
> Signed-off-by: Sidong Yang <realwakka@gmail.com>

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

Thanks,
Qu
> ---
> v2:
>    Use btrfs_qgroup_level() for checking
> ---
>   common/utils.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/common/utils.c b/common/utils.c
> index 57e41432..ba0bcb24 100644
> --- a/common/utils.c
> +++ b/common/utils.c
> @@ -727,6 +727,8 @@ u64 parse_qgroupid(const char *p)
>   		id = strtoull(p, &ptr_parse_end, 10);
>   		if (ptr_parse_end != ptr_src_end)
>   			goto path;
> +		if (btrfs_qgroup_level(id))
> +			goto err;
>   		return id;
>   	}
>   	level = strtoull(p, &ptr_parse_end, 10);
> @@ -734,6 +736,9 @@ u64 parse_qgroupid(const char *p)
>   		goto path;
>
>   	id = strtoull(s + 1, &ptr_parse_end, 10);
> +	if (btrfs_qgroup_level(id))
> +		goto err;
> +
>   	if (ptr_parse_end != ptr_src_end)
>   		goto  path;
>
>

Re: [PATCH v2] btrfs-progs: common: make sure that qgroup id is in range

[David Sterba]

On Tue, Mar 16, 2021 at 01:27:46PM +0000, Sidong Yang wrote:
> When user assign qgroup with qgroup id that is too big to exceeds
> range and invade level value, and it works without any error. but
> this action would be make undefined error. this code make sure that
> qgroup id doesn't exceed range(0 ~ 2^48-1).

Should the level be also validate? The function parse_qgroupid does not
do full validation, so eg 0//0 would be parsed as a path and not as a
typo, level larger than 64K will be silently clamped.

Re: [PATCH v2] btrfs-progs: common: make sure that qgroup id is in range

[Sidong Yang]

On Wed, Mar 17, 2021 at 07:36:47PM +0100, David Sterba wrote:
> On Tue, Mar 16, 2021 at 01:27:46PM +0000, Sidong Yang wrote:
> > When user assign qgroup with qgroup id that is too big to exceeds
> > range and invade level value, and it works without any error. but
> > this action would be make undefined error. this code make sure that
> > qgroup id doesn't exceed range(0 ~ 2^48-1).
> 
> Should the level be also validate? The function parse_qgroupid does not
> do full validation, so eg 0//0 would be parsed as a path and not as a
> typo, level larger than 64K will be silently clamped.

I agree. 0//0 would be parsed as path but it failed in
btrfs_util_is_subvolume() and goes to err. I understand that upper 16
bits of qgroupid is for level. so, The valid llevel range is [0~2^16-1].
But I can't get it that level larger than 64K will be clampled. 

one more question about that, I see that the ioctl calls just store the
qgroupid without any opeartion with level. is the level meaningless in
kernel?

Re: [PATCH v2] btrfs-progs: common: make sure that qgroup id is in range

[Qu Wenruo]

On 2021/3/18 上午10:22, Sidong Yang wrote:
> On Wed, Mar 17, 2021 at 07:36:47PM +0100, David Sterba wrote:
>> On Tue, Mar 16, 2021 at 01:27:46PM +0000, Sidong Yang wrote:
>>> When user assign qgroup with qgroup id that is too big to exceeds
>>> range and invade level value, and it works without any error. but
>>> this action would be make undefined error. this code make sure that
>>> qgroup id doesn't exceed range(0 ~ 2^48-1).
>>
>> Should the level be also validate? The function parse_qgroupid does not
>> do full validation, so eg 0//0 would be parsed as a path and not as a
>> typo, level larger than 64K will be silently clamped.
>
> I agree. 0//0 would be parsed as path but it failed in
> btrfs_util_is_subvolume() and goes to err. I understand that upper 16
> bits of qgroupid is for level. so, The valid llevel range is [0~2^16-1].
> But I can't get it that level larger than 64K will be clampled.
>
> one more question about that, I see that the ioctl calls just store the
> qgroupid without any opeartion with level. is the level meaningless in
> kernel?
>
No, kernel uses the qgroup level, but it's deep in qgroup code.

In fact, kernel treats qgroupid just as an u64, and sometimes checks the
qgroup level for relationship, but under most cases, it's just a u64,
level/id doesn't really matter that much.

Thanks,
Qu

Re: [PATCH v2] btrfs-progs: common: make sure that qgroup id is in range

[David Sterba]

On Thu, Mar 18, 2021 at 02:22:20AM +0000, Sidong Yang wrote:
> On Wed, Mar 17, 2021 at 07:36:47PM +0100, David Sterba wrote:
> > On Tue, Mar 16, 2021 at 01:27:46PM +0000, Sidong Yang wrote:
> > > When user assign qgroup with qgroup id that is too big to exceeds
> > > range and invade level value, and it works without any error. but
> > > this action would be make undefined error. this code make sure that
> > > qgroup id doesn't exceed range(0 ~ 2^48-1).
> > 
> > Should the level be also validate? The function parse_qgroupid does not
> > do full validation, so eg 0//0 would be parsed as a path and not as a
> > typo, level larger than 64K will be silently clamped.
> 
> I agree. 0//0 would be parsed as path but it failed in
> btrfs_util_is_subvolume() and goes to err. I understand that upper 16
> bits of qgroupid is for level. so, The valid llevel range is [0~2^16-1].
> But I can't get it that level larger than 64K will be clampled. 

The way the level gets stored into the final qgroup id is level << 48.
For example invalid values 70000/281474976710779 would be stored
as subvol id 123 and level 4465, where the u64 is 0x117100000000007b

> one more question about that, I see that the ioctl calls just store the
> qgroupid without any opeartion with level. is the level meaningless in
> kernel?

The quota groups are hierarchical and the level denotes the level, where
0/subvolid is the lowest always attached to a subvolume and the higher
levels are artificial and may contain any qgroups and do the whole
accounting on the subtree. The original design doc .pdf can be found in
https://git.kernel.org/pub/scm/linux/kernel/git/arne/qgroups-doc.git/tree/

Re: [PATCH v2] btrfs-progs: common: make sure that qgroup id is in range

[Sidong Yang]

On Thu, Mar 18, 2021 at 09:34:14PM +0100, David Sterba wrote:
> On Thu, Mar 18, 2021 at 02:22:20AM +0000, Sidong Yang wrote:
> > On Wed, Mar 17, 2021 at 07:36:47PM +0100, David Sterba wrote:
> > > On Tue, Mar 16, 2021 at 01:27:46PM +0000, Sidong Yang wrote:
> > > > When user assign qgroup with qgroup id that is too big to exceeds
> > > > range and invade level value, and it works without any error. but
> > > > this action would be make undefined error. this code make sure that
> > > > qgroup id doesn't exceed range(0 ~ 2^48-1).
> > > 
> > > Should the level be also validate? The function parse_qgroupid does not
> > > do full validation, so eg 0//0 would be parsed as a path and not as a
> > > typo, level larger than 64K will be silently clamped.
> > 
> > I agree. 0//0 would be parsed as path but it failed in
> > btrfs_util_is_subvolume() and goes to err. I understand that upper 16
> > bits of qgroupid is for level. so, The valid llevel range is [0~2^16-1].
> > But I can't get it that level larger than 64K will be clampled. 
> 
> The way the level gets stored into the final qgroup id is level << 48.
> For example invalid values 70000/281474976710779 would be stored
> as subvol id 123 and level 4465, where the u64 is 0x117100000000007b
> 
> > one more question about that, I see that the ioctl calls just store the
> > qgroupid without any opeartion with level. is the level meaningless in
> > kernel?
> 
> The quota groups are hierarchical and the level denotes the level, where
> 0/subvolid is the lowest always attached to a subvolume and the higher
> levels are artificial and may contain any qgroups and do the whole
> accounting on the subtree. The original design doc .pdf can be found in
> https://git.kernel.org/pub/scm/linux/kernel/git/arne/qgroups-doc.git/tree/

Thanks for detailed description.
I'll write the new patch for checking level also.

Thanks,
Sidong