[PATCH] btrfs-progs: fix eb leakage caused by missing btrfs_release_path() call.

[PATCH] btrfs-progs: fix eb leakage caused by missing btrfs_release_path() call.

[Qu Wenruo]

[BUG]
Commit 06b6ad5e017e ("btrfs-progs: check: check for invalid free space
tree entries") makes btrfs check to report eb leakage even on newly
created btrfs:

 # mkfs.btrfs -f test.img
 # btrfs check test.img
  Opening filesystem to check...
  Checking filesystem on test.img
  UUID: 13c26b6a-3b2c-49b3-94c7-80bcfa4e494b
  [1/7] checking root items
  [2/7] checking extents
  [3/7] checking free space tree
  [4/7] checking fs roots
  [5/7] checking only csums items (without verifying data)
  [6/7] checking root refs
  [7/7] checking quota groups skipped (not enabled on this FS)
  found 147456 bytes used, no error found
  total csum bytes: 0
  total tree bytes: 147456
  total fs tree bytes: 32768
  total extent tree bytes: 16384
  btree space waste bytes: 140595
  file data blocks allocated: 0
   referenced 0
  extent buffer leak: start 30572544 len 16384 <<< Extent buffer leakage

[CAUSE]
Surprisingly the patch submitted to the mailing list is correct:

+	path = btrfs_alloc_path();
+	if (!path)
+		return -ENOMEM;
+
+	while (1) {
...
+		if (ret < 0)
+			goto out;
+		if (path->slots[0] >= btrfs_header_nritems(path->nodes[0]))
+			break;
...
+	}
+	ret = 0;
+out:
+	btrfs_free_path(path);
+	return ret;
+}

So no matter if it's an error or we exhausted the free space tree, the
path will be released and freed eventually.

But the commit merged into btrfs-progs goes on-stack path method:

+       btrfs_init_path(&path);
+
+       while (1) {
...
+               if (ret < 0)
+                       goto out;
+               if (path.slots[0] >= btrfs_header_nritems(path.nodes[0]))
+                       break;
+
+               btrfs_release_path(&path);
...
+       }
+       ret = 0;
+out:
+       return ret;
+}
+

Now we only release the path inside the while loop, no at out tag.
This means, if we hit error or even just exhausted free space tree as
expected, we will leak the path to free space tree root.

Thus leading to the above leakage report.

[FIX]
Fix the bug by calling btrfs_release_path() at out: tag too.

This should make the code behave the same as the patch submitted to the
mailing list.

Fixes: 06b6ad5e017e ("btrfs-progs: check: check for invalid free space tree entries")
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/check/main.c b/check/main.c
index 0ba38f73c0a4..0c5716a51ad1 100644
--- a/check/main.c
+++ b/check/main.c
@@ -5776,6 +5776,7 @@ static int check_free_space_tree(struct btrfs_root *root)
 	}
 	ret = 0;
 out:
+	btrfs_release_path(&path);
 	return ret;
 }
 
-- 
2.37.2

Re: [PATCH] btrfs-progs: fix eb leakage caused by missing btrfs_release_path() call.

[David Sterba]

On Tue, Aug 30, 2022 at 12:57:16PM +0800, Qu Wenruo wrote:
> [BUG]
> Commit 06b6ad5e017e ("btrfs-progs: check: check for invalid free space
> tree entries") makes btrfs check to report eb leakage even on newly
> created btrfs:
> 
>  # mkfs.btrfs -f test.img
>  # btrfs check test.img
>   Opening filesystem to check...
>   Checking filesystem on test.img
>   UUID: 13c26b6a-3b2c-49b3-94c7-80bcfa4e494b
>   [1/7] checking root items
>   [2/7] checking extents
>   [3/7] checking free space tree
>   [4/7] checking fs roots
>   [5/7] checking only csums items (without verifying data)
>   [6/7] checking root refs
>   [7/7] checking quota groups skipped (not enabled on this FS)
>   found 147456 bytes used, no error found
>   total csum bytes: 0
>   total tree bytes: 147456
>   total fs tree bytes: 32768
>   total extent tree bytes: 16384
>   btree space waste bytes: 140595
>   file data blocks allocated: 0
>    referenced 0
>   extent buffer leak: start 30572544 len 16384 <<< Extent buffer leakage
> 
> [CAUSE]
> Surprisingly the patch submitted to the mailing list is correct:
> 
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return -ENOMEM;
> +
> +	while (1) {
> ...
> +		if (ret < 0)
> +			goto out;
> +		if (path->slots[0] >= btrfs_header_nritems(path->nodes[0]))
> +			break;
> ...
> +	}
> +	ret = 0;
> +out:
> +	btrfs_free_path(path);
> +	return ret;
> +}

Please don't put diffs into changelogs, it confuses some tools that
parse the mails or patches.

> 
> So no matter if it's an error or we exhausted the free space tree, the
> path will be released and freed eventually.
> 
> But the commit merged into btrfs-progs goes on-stack path method:

I do that because that's the preferred style, but not all people respond
to mailing list comments so we're left with unfixed bug with patch or a
unclean version committed if there's no followup. Or something in
between that could introduce bugs.

> 
> +       btrfs_init_path(&path);
> +
> +       while (1) {
> ...
> +               if (ret < 0)
> +                       goto out;
> +               if (path.slots[0] >= btrfs_header_nritems(path.nodes[0]))
> +                       break;
> +
> +               btrfs_release_path(&path);
> ...
> +       }
> +       ret = 0;
> +out:
> +       return ret;
> +}
> +
> 
> Now we only release the path inside the while loop, no at out tag.
> This means, if we hit error or even just exhausted free space tree as
> expected, we will leak the path to free space tree root.
> 
> Thus leading to the above leakage report.
> 
> [FIX]
> Fix the bug by calling btrfs_release_path() at out: tag too.
> 
> This should make the code behave the same as the patch submitted to the
> mailing list.
> 
> Fixes: 06b6ad5e017e ("btrfs-progs: check: check for invalid free space tree entries")
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Added to devel, thanks.

Re: [PATCH] btrfs-progs: fix eb leakage caused by missing btrfs_release_path() call.

[David Sterba]

On Wed, Aug 31, 2022 at 05:49:13AM +0800, Qu Wenruo wrote:
> > I do that because that's the preferred style, but not all people respond
> > to mailing list comments so we're left with unfixed bug with patch or a
> > unclean version committed if there's no followup. Or something in
> > between that could introduce bugs.
> 
> Another thing related to this on-stack path usage is, do we need the
> same change in kernel space?

No, in kernel the stack space is a limited resource.

> And do we prefer on-stack path initialized to all 0, or use
> btrfs_init_path()?

It's initialized by btrfs_init_path everywhere else so for consistency
this should be used, though the memset 0 is also possible, there are
only int types or pointers in the structure.

Re: [PATCH] btrfs-progs: fix eb leakage caused by missing btrfs_release_path() call.

[Qu Wenruo]

On 2022/8/31 01:17, David Sterba wrote:
> On Tue, Aug 30, 2022 at 12:57:16PM +0800, Qu Wenruo wrote:
>> [BUG]
>> Commit 06b6ad5e017e ("btrfs-progs: check: check for invalid free space
>> tree entries") makes btrfs check to report eb leakage even on newly
>> created btrfs:
>>
>>   # mkfs.btrfs -f test.img
>>   # btrfs check test.img
>>    Opening filesystem to check...
>>    Checking filesystem on test.img
>>    UUID: 13c26b6a-3b2c-49b3-94c7-80bcfa4e494b
>>    [1/7] checking root items
>>    [2/7] checking extents
>>    [3/7] checking free space tree
>>    [4/7] checking fs roots
>>    [5/7] checking only csums items (without verifying data)
>>    [6/7] checking root refs
>>    [7/7] checking quota groups skipped (not enabled on this FS)
>>    found 147456 bytes used, no error found
>>    total csum bytes: 0
>>    total tree bytes: 147456
>>    total fs tree bytes: 32768
>>    total extent tree bytes: 16384
>>    btree space waste bytes: 140595
>>    file data blocks allocated: 0
>>     referenced 0
>>    extent buffer leak: start 30572544 len 16384 <<< Extent buffer leakage
>>
>> [CAUSE]
>> Surprisingly the patch submitted to the mailing list is correct:
>>
>> +	path = btrfs_alloc_path();
>> +	if (!path)
>> +		return -ENOMEM;
>> +
>> +	while (1) {
>> ...
>> +		if (ret < 0)
>> +			goto out;
>> +		if (path->slots[0] >= btrfs_header_nritems(path->nodes[0]))
>> +			break;
>> ...
>> +	}
>> +	ret = 0;
>> +out:
>> +	btrfs_free_path(path);
>> +	return ret;
>> +}
>
> Please don't put diffs into changelogs, it confuses some tools that
> parse the mails or patches.
>
>>
>> So no matter if it's an error or we exhausted the free space tree, the
>> path will be released and freed eventually.
>>
>> But the commit merged into btrfs-progs goes on-stack path method:
>
> I do that because that's the preferred style, but not all people respond
> to mailing list comments so we're left with unfixed bug with patch or a
> unclean version committed if there's no followup. Or something in
> between that could introduce bugs.

Another thing related to this on-stack path usage is, do we need the
same change in kernel space?

And do we prefer on-stack path initialized to all 0, or use
btrfs_init_path()?

Thanks,
Qu
>
>>
>> +       btrfs_init_path(&path);
>> +
>> +       while (1) {
>> ...
>> +               if (ret < 0)
>> +                       goto out;
>> +               if (path.slots[0] >= btrfs_header_nritems(path.nodes[0]))
>> +                       break;
>> +
>> +               btrfs_release_path(&path);
>> ...
>> +       }
>> +       ret = 0;
>> +out:
>> +       return ret;
>> +}
>> +
>>
>> Now we only release the path inside the while loop, no at out tag.
>> This means, if we hit error or even just exhausted free space tree as
>> expected, we will leak the path to free space tree root.
>>
>> Thus leading to the above leakage report.
>>
>> [FIX]
>> Fix the bug by calling btrfs_release_path() at out: tag too.
>>
>> This should make the code behave the same as the patch submitted to the
>> mailing list.
>>
>> Fixes: 06b6ad5e017e ("btrfs-progs: check: check for invalid free space tree entries")
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>
> Added to devel, thanks.

Re: [PATCH] btrfs-progs: fix eb leakage caused by missing btrfs_release_path() call.

[Qu Wenruo]

On 2022/8/31 05:49, David Sterba wrote:
> On Wed, Aug 31, 2022 at 05:49:13AM +0800, Qu Wenruo wrote:
>>> I do that because that's the preferred style, but not all people respond
>>> to mailing list comments so we're left with unfixed bug with patch or a
>>> unclean version committed if there's no followup. Or something in
>>> between that could introduce bugs.
>>
>> Another thing related to this on-stack path usage is, do we need the
>> same change in kernel space?
>
> No, in kernel the stack space is a limited resource.

Should it be a case by case check or a general recommendation?

As for some call sites which is known to have very shallow stack height,
can we use on-stack path or just avoid it for all cases?

Thanks,
Qu
>
>> And do we prefer on-stack path initialized to all 0, or use
>> btrfs_init_path()?
>
> It's initialized by btrfs_init_path everywhere else so for consistency
> this should be used, though the memset 0 is also possible, there are
> only int types or pointers in the structure.

Re: [PATCH] btrfs-progs: fix eb leakage caused by missing btrfs_release_path() call.

[David Sterba]

On Wed, Aug 31, 2022 at 06:15:15AM +0800, Qu Wenruo wrote:
> On 2022/8/31 05:49, David Sterba wrote:
> > On Wed, Aug 31, 2022 at 05:49:13AM +0800, Qu Wenruo wrote:
> >>> I do that because that's the preferred style, but not all people respond
> >>> to mailing list comments so we're left with unfixed bug with patch or a
> >>> unclean version committed if there's no followup. Or something in
> >>> between that could introduce bugs.
> >>
> >> Another thing related to this on-stack path usage is, do we need the
> >> same change in kernel space?
> >
> > No, in kernel the stack space is a limited resource.
> 
> Should it be a case by case check or a general recommendation?

Path is not suitable for on-stack storage, it's 112 bytes and used in
may functions called deep from call chains. The safe case for on-stack
storage are typically the ioctl callbacks for the ioctl arguments where
the ioclt is also lightweight (not doing IO).

> As for some call sites which is known to have very shallow stack height,
> can we use on-stack path or just avoid it for all cases?

The on-stack can be considered an optimization so it should be done in
cases where we gain something, eg. removing a potential error case,
otherwise for consistency it can be allocated by kmalloc or from the
slab caches. Using the on-stack storage should be justified and proven
that it's safe and not a first thought trying to be clever.

The stack depth on function entry could be estimated in some cases but
when IO is involved it's best to reduce the consumption when the block
layer or other layers are stacked, eg. MD/LVM/NFS/iSCSI/... Temporary
local variables should be free to use, compiler will know if it's safe
to reuse the memory and we can have readable code, so it's namely for
arrays or large structures.