btrfs: qgroup: add missing extent changeset release

[PATCH] btrfs: qgroup: add missing extent changeset release

Posted by Fedor Pchelkin 1 year, 3 months ago

The extent changeset may have some additional memory dynamically allocated
for ulist in result of clear_record_extent_bits() execution.

Release it after the local changeset is no longer needed in
BTRFS_QGROUP_MODE_DISABLED case.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

Reported-by: syzbot+81670362c283f3dd889c@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/lkml/000000000000aa8c0c060ade165e@google.com
Fixes: af0e2aab3b70 ("btrfs: qgroup: flush reservations during quota disable")
Cc: stable@vger.kernel.org # 6.10+
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
 fs/btrfs/qgroup.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 5d57a285d59b..4f1fa5d427e1 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -4345,9 +4345,10 @@ static int __btrfs_qgroup_release_data(struct btrfs_inode *inode,
 
 	if (btrfs_qgroup_mode(inode->root->fs_info) == BTRFS_QGROUP_MODE_DISABLED) {
 		extent_changeset_init(&changeset);
-		return clear_record_extent_bits(&inode->io_tree, start,
-						start + len - 1,
-						EXTENT_QGROUP_RESERVED, &changeset);
+		ret = clear_record_extent_bits(&inode->io_tree, start,
+					       start + len - 1,
+					       EXTENT_QGROUP_RESERVED, &changeset);
+		goto out;
 	}
 
 	/* In release case, we shouldn't have @reserved */
-- 
2.39.2

Re: [PATCH] btrfs: qgroup: add missing extent changeset release

Posted by Qu Wenruo 1 year, 3 months ago


在 2024/8/28 00:42, Fedor Pchelkin 写道:
> The extent changeset may have some additional memory dynamically allocated
> for ulist in result of clear_record_extent_bits() execution.
>
> Release it after the local changeset is no longer needed in
> BTRFS_QGROUP_MODE_DISABLED case.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Reported-by: syzbot+81670362c283f3dd889c@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/lkml/000000000000aa8c0c060ade165e@google.com
> Fixes: af0e2aab3b70 ("btrfs: qgroup: flush reservations during quota disable")
> Cc: stable@vger.kernel.org # 6.10+
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>

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

In this particular case, the changeset is really only locally utilized,
thus should always be released.

Thanks,
Qu
> ---
>   fs/btrfs/qgroup.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 5d57a285d59b..4f1fa5d427e1 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -4345,9 +4345,10 @@ static int __btrfs_qgroup_release_data(struct btrfs_inode *inode,
>
>   	if (btrfs_qgroup_mode(inode->root->fs_info) == BTRFS_QGROUP_MODE_DISABLED) {
>   		extent_changeset_init(&changeset);
> -		return clear_record_extent_bits(&inode->io_tree, start,
> -						start + len - 1,
> -						EXTENT_QGROUP_RESERVED, &changeset);
> +		ret = clear_record_extent_bits(&inode->io_tree, start,
> +					       start + len - 1,
> +					       EXTENT_QGROUP_RESERVED, &changeset);
> +		goto out;
>   	}
>
>   	/* In release case, we shouldn't have @reserved */

Re: [PATCH] btrfs: qgroup: add missing extent changeset release

Posted by Qu Wenruo 1 year, 3 months ago


在 2024/8/28 18:54, Qu Wenruo 写道:
>
>
> 在 2024/8/28 00:42, Fedor Pchelkin 写道:
>> The extent changeset may have some additional memory dynamically
>> allocated
>> for ulist in result of clear_record_extent_bits() execution.
>>
>> Release it after the local changeset is no longer needed in
>> BTRFS_QGROUP_MODE_DISABLED case.
>>
>> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>>
>> Reported-by: syzbot+81670362c283f3dd889c@syzkaller.appspotmail.com
>> Closes:
>> https://lore.kernel.org/lkml/000000000000aa8c0c060ade165e@google.com
>> Fixes: af0e2aab3b70 ("btrfs: qgroup: flush reservations during quota
>> disable")
>> Cc: stable@vger.kernel.org # 6.10+
>> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
>
> Reviewed-by: Qu Wenruo <wqu@suse.com>
>
> In this particular case, the changeset is really only locally utilized,
> thus should always be released.

My bad, after checking your latest reply to David, I think we can go one
step further.

Just do not pass changeset to clear_record_extent_bits().

A changeset is utilized for two reasons:

- To let the caller know how many bytes are changed
   Just like what we did for the qgroup enabled case.

- Allow the caller to revert its change
   This happens for qgroup_unreserve_range() when we hit an error and
   needs to free what we just reserved.

In this particular case, since qgroup is already disabled, we just want
to clear the extent io tree bits, not really bother how many bytes are
released nor keep the info for reverting.

So just pass NULL and everything should be fine.

Thanks,
Qu
>
> Thanks,
> Qu
>> ---
>>   fs/btrfs/qgroup.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
>> index 5d57a285d59b..4f1fa5d427e1 100644
>> --- a/fs/btrfs/qgroup.c
>> +++ b/fs/btrfs/qgroup.c
>> @@ -4345,9 +4345,10 @@ static int __btrfs_qgroup_release_data(struct
>> btrfs_inode *inode,
>>
>>       if (btrfs_qgroup_mode(inode->root->fs_info) ==
>> BTRFS_QGROUP_MODE_DISABLED) {
>>           extent_changeset_init(&changeset);
>> -        return clear_record_extent_bits(&inode->io_tree, start,
>> -                        start + len - 1,
>> -                        EXTENT_QGROUP_RESERVED, &changeset);
>> +        ret = clear_record_extent_bits(&inode->io_tree, start,
>> +                           start + len - 1,
>> +                           EXTENT_QGROUP_RESERVED, &changeset);
>> +        goto out;
>>       }
>>
>>       /* In release case, we shouldn't have @reserved */
>

[PATCH v2] btrfs: qgroup: don't use extent changeset when not needed

Posted by Fedor Pchelkin 1 year, 3 months ago

The local extent changeset is passed to clear_record_extent_bits() where
it may have some additional memory dynamically allocated for ulist. When
qgroup is disabled, the memory is leaked because in this case the
changeset is not released upon __btrfs_qgroup_release_data() return.

Since the recorded contents of the changeset are not used thereafter, just
don't pass it.

Found by Linux Verification Center (linuxtesting.org) with Syzkaller.

Reported-by: syzbot+81670362c283f3dd889c@syzkaller.appspotmail.com
Closes: https://lore.kernel.org/lkml/000000000000aa8c0c060ade165e@google.com
Fixes: af0e2aab3b70 ("btrfs: qgroup: flush reservations during quota disable")
Cc: stable@vger.kernel.org # 6.10+
Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
---
v2: rework the fix as Qu Wenruo suggested - just don't pass unneeded
    changeset. Update the commit title and description accordingly.

 fs/btrfs/qgroup.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 5d57a285d59b..f6118c5f3c9f 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -4344,10 +4344,9 @@ static int __btrfs_qgroup_release_data(struct btrfs_inode *inode,
 	int ret;
 
 	if (btrfs_qgroup_mode(inode->root->fs_info) == BTRFS_QGROUP_MODE_DISABLED) {
-		extent_changeset_init(&changeset);
 		return clear_record_extent_bits(&inode->io_tree, start,
 						start + len - 1,
-						EXTENT_QGROUP_RESERVED, &changeset);
+						EXTENT_QGROUP_RESERVED, NULL);
 	}
 
 	/* In release case, we shouldn't have @reserved */
-- 
2.39.2

Re: [PATCH v2] btrfs: qgroup: don't use extent changeset when not needed

Posted by David Sterba 1 year, 3 months ago

On Wed, Aug 28, 2024 at 07:14:11PM +0300, Fedor Pchelkin wrote:
> The local extent changeset is passed to clear_record_extent_bits() where
> it may have some additional memory dynamically allocated for ulist. When
> qgroup is disabled, the memory is leaked because in this case the
> changeset is not released upon __btrfs_qgroup_release_data() return.
> 
> Since the recorded contents of the changeset are not used thereafter, just
> don't pass it.
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Reported-by: syzbot+81670362c283f3dd889c@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/lkml/000000000000aa8c0c060ade165e@google.com
> Fixes: af0e2aab3b70 ("btrfs: qgroup: flush reservations during quota disable")
> Cc: stable@vger.kernel.org # 6.10+
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>
> ---
> v2: rework the fix as Qu Wenruo suggested - just don't pass unneeded
>     changeset. Update the commit title and description accordingly.

Thanks, added to for-next.

Re: [PATCH v2] btrfs: qgroup: don't use extent changeset when not needed

Posted by Qu Wenruo 1 year, 3 months ago


在 2024/8/29 01:44, Fedor Pchelkin 写道:
> The local extent changeset is passed to clear_record_extent_bits() where
> it may have some additional memory dynamically allocated for ulist. When
> qgroup is disabled, the memory is leaked because in this case the
> changeset is not released upon __btrfs_qgroup_release_data() return.
>
> Since the recorded contents of the changeset are not used thereafter, just
> don't pass it.
>
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
>
> Reported-by: syzbot+81670362c283f3dd889c@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/lkml/000000000000aa8c0c060ade165e@google.com
> Fixes: af0e2aab3b70 ("btrfs: qgroup: flush reservations during quota disable")
> Cc: stable@vger.kernel.org # 6.10+
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>

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

Thanks,
Qu

> ---
> v2: rework the fix as Qu Wenruo suggested - just don't pass unneeded
>      changeset. Update the commit title and description accordingly.
>
>   fs/btrfs/qgroup.c | 3 +--
>   1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 5d57a285d59b..f6118c5f3c9f 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -4344,10 +4344,9 @@ static int __btrfs_qgroup_release_data(struct btrfs_inode *inode,
>   	int ret;
>
>   	if (btrfs_qgroup_mode(inode->root->fs_info) == BTRFS_QGROUP_MODE_DISABLED) {
> -		extent_changeset_init(&changeset);
>   		return clear_record_extent_bits(&inode->io_tree, start,
>   						start + len - 1,
> -						EXTENT_QGROUP_RESERVED, &changeset);
> +						EXTENT_QGROUP_RESERVED, NULL);
>   	}
>
>   	/* In release case, we shouldn't have @reserved */

Re: [PATCH v2] btrfs: qgroup: don't use extent changeset when not needed

Posted by Boris Burkov 1 year, 3 months ago

On Wed, Aug 28, 2024 at 07:14:11PM +0300, Fedor Pchelkin wrote:
> The local extent changeset is passed to clear_record_extent_bits() where
> it may have some additional memory dynamically allocated for ulist. When
> qgroup is disabled, the memory is leaked because in this case the
> changeset is not released upon __btrfs_qgroup_release_data() return.
> 
> Since the recorded contents of the changeset are not used thereafter, just
> don't pass it.
> 
> Found by Linux Verification Center (linuxtesting.org) with Syzkaller.
> 
> Reported-by: syzbot+81670362c283f3dd889c@syzkaller.appspotmail.com
> Closes: https://lore.kernel.org/lkml/000000000000aa8c0c060ade165e@google.com
> Fixes: af0e2aab3b70 ("btrfs: qgroup: flush reservations during quota disable")
> Cc: stable@vger.kernel.org # 6.10+
> Signed-off-by: Fedor Pchelkin <pchelkin@ispras.ru>

This version looks even better, to me. Thanks for the catch and fix!

Reviewed-by: Boris Burkov <boris@bur.io>

> ---
> v2: rework the fix as Qu Wenruo suggested - just don't pass unneeded
>     changeset. Update the commit title and description accordingly.
> 
>  fs/btrfs/qgroup.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 5d57a285d59b..f6118c5f3c9f 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -4344,10 +4344,9 @@ static int __btrfs_qgroup_release_data(struct btrfs_inode *inode,
>  	int ret;
>  
>  	if (btrfs_qgroup_mode(inode->root->fs_info) == BTRFS_QGROUP_MODE_DISABLED) {
> -		extent_changeset_init(&changeset);
>  		return clear_record_extent_bits(&inode->io_tree, start,
>  						start + len - 1,
> -						EXTENT_QGROUP_RESERVED, &changeset);
> +						EXTENT_QGROUP_RESERVED, NULL);
>  	}
>  
>  	/* In release case, we shouldn't have @reserved */
> -- 
> 2.39.2
>

Re: [PATCH] btrfs: qgroup: add missing extent changeset release

Posted by David Sterba 1 year, 3 months ago

On Tue, Aug 27, 2024 at 06:12:43PM +0300, Fedor Pchelkin wrote:
> The extent changeset may have some additional memory dynamically allocated
> for ulist in result of clear_record_extent_bits() execution.

This can happen, as clear_record_extent_bits adds more data to the
changeset in some cases. What I don't see yet how it happens. An extent
range must be split so that a new entry is added with different bits
set. This is usual thing, but why does this happen with the quotas
disabled.

Re: [PATCH] btrfs: qgroup: add missing extent changeset release

Posted by Fedor Pchelkin 1 year, 3 months ago

On Tue, 27. Aug 18:03, David Sterba wrote:
> On Tue, Aug 27, 2024 at 06:12:43PM +0300, Fedor Pchelkin wrote:
> > The extent changeset may have some additional memory dynamically allocated
> > for ulist in result of clear_record_extent_bits() execution.
> 
> This can happen, as clear_record_extent_bits adds more data to the
> changeset in some cases. What I don't see yet how it happens. An extent
> range must be split so that a new entry is added with different bits
> set. This is usual thing, but why does this happen with the quotas
> disabled.

In the reproducer case, qgroup_reserve_data() which sets the bits happens
just before disabling the quotas via ioctl.

Commit af0e2aab3b70 ("btrfs: qgroup: flush reservations during quota disable")
added a call to clear_record_extent_bits() inside __btrfs_qgroup_release_data().
The changeset being passed is freshly initialized and empty. So the first call
to clear_state_bit() there will definitely create a new entry and add it to
the ulist.

If for some reason clear_state_bit() shouldn't be eventually called then,
to be honest, I don't quite understand why a call to clear_record_extent_bits()
was added in the first place without expecting it to do the real work with
clear_state_bit().