| Message ID | 043f1db2c7548723eaff302ebba4183afb910830.1661835430.git.wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs-progs: fix eb leakage caused by missing btrfs_release_path() call. | expand |
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.
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.
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.
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.
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.
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; }
[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(+)