在 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 */

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