* [bug report] btrfs: turn fs_roots_radix in btrfs_fs_info into an XArray
@ 2022-05-04 15:29 Dan Carpenter
2022-05-11 13:10 ` David Sterba
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2022-05-04 15:29 UTC (permalink / raw)
To: gniebler; +Cc: linux-btrfs
Hello Gabriel Niebler,
The patch eb8da5bf4831: "btrfs: turn fs_roots_radix in btrfs_fs_info
into an XArray" from Apr 26, 2022, leads to the following Smatch
static checker warning:
fs/btrfs/disk-io.c:4453 btrfs_cleanup_fs_roots() error: uninitialized symbol 'i'.
fs/btrfs/disk-io.c:4453 btrfs_cleanup_fs_roots() error: uninitialized symbol 'grabbed'.
fs/btrfs/disk-io.c
4408 int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
4409 {
4410 struct btrfs_root *roots[8];
4411 unsigned long index = 0;
4412 int i;
4413 int err = 0;
4414 int grabbed;
4415
4416 while (1) {
4417 struct btrfs_root *root;
4418
4419 spin_lock(&fs_info->fs_roots_lock);
4420 if (!xa_find(&fs_info->fs_roots, &index, ULONG_MAX, XA_PRESENT)) {
4421 spin_unlock(&fs_info->fs_roots_lock);
4422 break;
"i" and "grabbed" are uninitialized if we hit this break statement on
the first iteration through the loop.
roots is also uninitialized. This error handling is badly broken.
If we hit it on the second iteration then we are also toasted. Double
frees. I think. (Trying to send emails quickly and then head out the
door).
4423 }
4424
4425 grabbed = 0;
4426 xa_for_each_start(&fs_info->fs_roots, index, root, index) {
4427 /* Avoid grabbing roots in dead_roots */
4428 if (btrfs_root_refs(&root->root_item) == 0) {
4429 roots[grabbed] = NULL;
4430 } else {
4431 /* Grab all the search results for later use */
4432 roots[grabbed] = btrfs_grab_root(root);
4433 }
4434 grabbed++;
4435 if (grabbed >= ARRAY_SIZE(roots))
4436 break;
4437 }
4438 spin_unlock(&fs_info->fs_roots_lock);
4439
4440 for (i = 0; i < grabbed; i++) {
4441 if (!roots[i])
4442 continue;
4443 index = roots[i]->root_key.objectid;
4444 err = btrfs_orphan_cleanup(roots[i]);
4445 if (err)
4446 break;
Imagine we hit this break. Double frees as well.
4447 btrfs_put_root(roots[i]);
4448 }
4449 index++;
4450 }
4451
4452 /* Release the roots that remain uncleaned due to error */
--> 4453 for (; i < grabbed; i++) {
^^^^^^^^^^^
4454 if (roots[i])
4455 btrfs_put_root(roots[i]);
4456 }
4457 return err;
4458 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] btrfs: turn fs_roots_radix in btrfs_fs_info into an XArray
2022-05-04 15:29 Dan Carpenter
@ 2022-05-11 13:10 ` David Sterba
0 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2022-05-11 13:10 UTC (permalink / raw)
To: Dan Carpenter; +Cc: gniebler, linux-btrfs
On Wed, May 04, 2022 at 06:29:18PM +0300, Dan Carpenter wrote:
> Hello Gabriel Niebler,
>
> The patch eb8da5bf4831: "btrfs: turn fs_roots_radix in btrfs_fs_info
> into an XArray" from Apr 26, 2022, leads to the following Smatch
> static checker warning:
>
> fs/btrfs/disk-io.c:4453 btrfs_cleanup_fs_roots() error: uninitialized symbol 'i'.
> fs/btrfs/disk-io.c:4453 btrfs_cleanup_fs_roots() error: uninitialized symbol 'grabbed'.
>
> fs/btrfs/disk-io.c
> 4408 int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
> 4409 {
> 4410 struct btrfs_root *roots[8];
> 4411 unsigned long index = 0;
> 4412 int i;
> 4413 int err = 0;
> 4414 int grabbed;
> 4415
> 4416 while (1) {
> 4417 struct btrfs_root *root;
> 4418
> 4419 spin_lock(&fs_info->fs_roots_lock);
> 4420 if (!xa_find(&fs_info->fs_roots, &index, ULONG_MAX, XA_PRESENT)) {
> 4421 spin_unlock(&fs_info->fs_roots_lock);
> 4422 break;
>
> "i" and "grabbed" are uninitialized if we hit this break statement on
> the first iteration through the loop.
>
> roots is also uninitialized. This error handling is badly broken.
>
> If we hit it on the second iteration then we are also toasted. Double
> frees. I think. (Trying to send emails quickly and then head out the
> door).
Thanks for the report, this has been meanwhile fixed in the tree,
Nikolay sent a fixup that replaced break with return.
^ permalink raw reply [flat|nested] 7+ messages in thread
* [bug report] btrfs: turn fs_roots_radix in btrfs_fs_info into an XArray
@ 2022-05-16 8:04 Dan Carpenter
2022-05-16 10:39 ` Nikolay Borisov
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2022-05-16 8:04 UTC (permalink / raw)
To: gniebler; +Cc: linux-btrfs
Hello Gabriel Niebler,
The patch 06a79e50ff00: "btrfs: turn fs_roots_radix in btrfs_fs_info
into an XArray" from May 3, 2022, leads to the following Smatch
static checker warning:
fs/btrfs/disk-io.c:4560 btrfs_cleanup_fs_roots()
warn: ignoring unreachable code.
fs/btrfs/disk-io.c
4520 int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
4521 {
4522 struct btrfs_root *roots[8];
4523 unsigned long index = 0;
4524 int i;
4525 int err = 0;
4526 int grabbed;
4527
4528 while (1) {
4529 struct btrfs_root *root;
4530
4531 spin_lock(&fs_info->fs_roots_lock);
4532 if (!xa_find(&fs_info->fs_roots, &index, ULONG_MAX, XA_PRESENT)) {
4533 spin_unlock(&fs_info->fs_roots_lock);
4534 return err;
4535 }
4536
4537 grabbed = 0;
4538 xa_for_each_start(&fs_info->fs_roots, index, root, index) {
4539 /* Avoid grabbing roots in dead_roots */
4540 if (btrfs_root_refs(&root->root_item) > 0)
4541 roots[grabbed++] = btrfs_grab_root(root);
4542 if (grabbed >= ARRAY_SIZE(roots))
4543 break;
4544 }
4545 spin_unlock(&fs_info->fs_roots_lock);
4546
4547 for (i = 0; i < grabbed; i++) {
4548 if (!roots[i])
4549 continue;
4550 index = roots[i]->root_key.objectid;
4551 err = btrfs_orphan_cleanup(roots[i]);
4552 if (err)
4553 break;
4554 btrfs_put_root(roots[i]);
4555 }
4556 index++;
4557 }
4558
4559 /* Release the roots that remain uncleaned due to error */
--> 4560 for (; i < grabbed; i++) {
This code is unreachable now.
4561 if (roots[i])
4562 btrfs_put_root(roots[i]);
4563 }
4564 return err;
4565 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] btrfs: turn fs_roots_radix in btrfs_fs_info into an XArray
2022-05-16 8:04 [bug report] btrfs: turn fs_roots_radix in btrfs_fs_info into an XArray Dan Carpenter
@ 2022-05-16 10:39 ` Nikolay Borisov
2022-05-16 11:02 ` Dan Carpenter
0 siblings, 1 reply; 7+ messages in thread
From: Nikolay Borisov @ 2022-05-16 10:39 UTC (permalink / raw)
To: Dan Carpenter, gniebler; +Cc: linux-btrfs
On 16.05.22 г. 11:04 ч., Dan Carpenter wrote:
> Hello Gabriel Niebler,
>
> The patch 06a79e50ff00: "btrfs: turn fs_roots_radix in btrfs_fs_info
> into an XArray" from May 3, 2022, leads to the following Smatch
> static checker warning:
>
> fs/btrfs/disk-io.c:4560 btrfs_cleanup_fs_roots()
> warn: ignoring unreachable code.
>
> fs/btrfs/disk-io.c
> 4520 int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
> 4521 {
> 4522 struct btrfs_root *roots[8];
> 4523 unsigned long index = 0;
> 4524 int i;
> 4525 int err = 0;
> 4526 int grabbed;
> 4527
> 4528 while (1) {
> 4529 struct btrfs_root *root;
> 4530
> 4531 spin_lock(&fs_info->fs_roots_lock);
> 4532 if (!xa_find(&fs_info->fs_roots, &index, ULONG_MAX, XA_PRESENT)) {
> 4533 spin_unlock(&fs_info->fs_roots_lock);
> 4534 return err;
> 4535 }
> 4536
> 4537 grabbed = 0;
> 4538 xa_for_each_start(&fs_info->fs_roots, index, root, index) {
> 4539 /* Avoid grabbing roots in dead_roots */
> 4540 if (btrfs_root_refs(&root->root_item) > 0)
> 4541 roots[grabbed++] = btrfs_grab_root(root);
> 4542 if (grabbed >= ARRAY_SIZE(roots))
> 4543 break;
> 4544 }
> 4545 spin_unlock(&fs_info->fs_roots_lock);
> 4546
> 4547 for (i = 0; i < grabbed; i++) {
> 4548 if (!roots[i])
> 4549 continue;
> 4550 index = roots[i]->root_key.objectid;
> 4551 err = btrfs_orphan_cleanup(roots[i]);
> 4552 if (err)
> 4553 break;
> 4554 btrfs_put_root(roots[i]);
> 4555 }
> 4556 index++;
> 4557 }
> 4558
> 4559 /* Release the roots that remain uncleaned due to error */
> --> 4560 for (; i < grabbed; i++) {
>
> This code is unreachable now.
How is it unreachable, if we error in the middle of
btrfs_orphan_cleanup, we'd break and this loop will cleanup the rest of
the roots,
>
> 4561 if (roots[i])
> 4562 btrfs_put_root(roots[i]);
> 4563 }
> 4564 return err;
> 4565 }
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] btrfs: turn fs_roots_radix in btrfs_fs_info into an XArray
2022-05-16 10:39 ` Nikolay Borisov
@ 2022-05-16 11:02 ` Dan Carpenter
2022-05-16 11:45 ` Nikolay Borisov
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2022-05-16 11:02 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: gniebler, linux-btrfs
On Mon, May 16, 2022 at 01:39:52PM +0300, Nikolay Borisov wrote:
>
>
> On 16.05.22 г. 11:04 ч., Dan Carpenter wrote:
> > Hello Gabriel Niebler,
> >
> > The patch 06a79e50ff00: "btrfs: turn fs_roots_radix in btrfs_fs_info
> > into an XArray" from May 3, 2022, leads to the following Smatch
> > static checker warning:
> >
> > fs/btrfs/disk-io.c:4560 btrfs_cleanup_fs_roots()
> > warn: ignoring unreachable code.
> >
> > fs/btrfs/disk-io.c
> > 4520 int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
> > 4521 {
> > 4522 struct btrfs_root *roots[8];
> > 4523 unsigned long index = 0;
> > 4524 int i;
> > 4525 int err = 0;
> > 4526 int grabbed;
> > 4527
> > 4528 while (1) {
> > 4529 struct btrfs_root *root;
> > 4530
> > 4531 spin_lock(&fs_info->fs_roots_lock);
> > 4532 if (!xa_find(&fs_info->fs_roots, &index, ULONG_MAX, XA_PRESENT)) {
> > 4533 spin_unlock(&fs_info->fs_roots_lock);
> > 4534 return err;
> > 4535 }
> > 4536
> > 4537 grabbed = 0;
> > 4538 xa_for_each_start(&fs_info->fs_roots, index, root, index) {
> > 4539 /* Avoid grabbing roots in dead_roots */
> > 4540 if (btrfs_root_refs(&root->root_item) > 0)
> > 4541 roots[grabbed++] = btrfs_grab_root(root);
> > 4542 if (grabbed >= ARRAY_SIZE(roots))
> > 4543 break;
breaks out of xa_for_each_start() loop.
> > 4544 }
> > 4545 spin_unlock(&fs_info->fs_roots_lock);
> > 4546
> > 4547 for (i = 0; i < grabbed; i++) {
> > 4548 if (!roots[i])
> > 4549 continue;
> > 4550 index = roots[i]->root_key.objectid;
> > 4551 err = btrfs_orphan_cleanup(roots[i]);
> > 4552 if (err)
> > 4553 break;
breaks out of for loop.
> > 4554 btrfs_put_root(roots[i]);
> > 4555 }
> > 4556 index++;
> > 4557 }
> > 4558
> > 4559 /* Release the roots that remain uncleaned due to error */
> > --> 4560 for (; i < grabbed; i++) {
> >
> > This code is unreachable now.
>
> How is it unreachable, if we error in the middle of btrfs_orphan_cleanup,
> we'd break and this loop will cleanup the rest of the roots,
There are breaks inside the inner loops but nothing breaks out of the
while (1) loop.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] btrfs: turn fs_roots_radix in btrfs_fs_info into an XArray
2022-05-16 11:02 ` Dan Carpenter
@ 2022-05-16 11:45 ` Nikolay Borisov
2022-05-16 15:18 ` David Sterba
0 siblings, 1 reply; 7+ messages in thread
From: Nikolay Borisov @ 2022-05-16 11:45 UTC (permalink / raw)
To: Dan Carpenter; +Cc: gniebler, linux-btrfs
On 16.05.22 г. 14:02 ч., Dan Carpenter wrote:
> On Mon, May 16, 2022 at 01:39:52PM +0300, Nikolay Borisov wrote:
>>
>>
>> On 16.05.22 г. 11:04 ч., Dan Carpenter wrote:
>>> Hello Gabriel Niebler,
>>>
>>> The patch 06a79e50ff00: "btrfs: turn fs_roots_radix in btrfs_fs_info
>>> into an XArray" from May 3, 2022, leads to the following Smatch
>>> static checker warning:
>>>
>>> fs/btrfs/disk-io.c:4560 btrfs_cleanup_fs_roots()
>>> warn: ignoring unreachable code.
>>>
>>> fs/btrfs/disk-io.c
>>> 4520 int btrfs_cleanup_fs_roots(struct btrfs_fs_info *fs_info)
>>> 4521 {
>>> 4522 struct btrfs_root *roots[8];
>>> 4523 unsigned long index = 0;
>>> 4524 int i;
>>> 4525 int err = 0;
>>> 4526 int grabbed;
>>> 4527
>>> 4528 while (1) {
>>> 4529 struct btrfs_root *root;
>>> 4530
>>> 4531 spin_lock(&fs_info->fs_roots_lock);
>>> 4532 if (!xa_find(&fs_info->fs_roots, &index, ULONG_MAX, XA_PRESENT)) {
>>> 4533 spin_unlock(&fs_info->fs_roots_lock);
>>> 4534 return err;
>>> 4535 }
>>> 4536
>>> 4537 grabbed = 0;
>>> 4538 xa_for_each_start(&fs_info->fs_roots, index, root, index) {
>>> 4539 /* Avoid grabbing roots in dead_roots */
>>> 4540 if (btrfs_root_refs(&root->root_item) > 0)
>>> 4541 roots[grabbed++] = btrfs_grab_root(root);
>>> 4542 if (grabbed >= ARRAY_SIZE(roots))
>>> 4543 break;
>
> breaks out of xa_for_each_start() loop.
This is fine, as when we fill root[] then we'd process it in the
subsequent for loop.
>
>>> 4544 }
>>> 4545 spin_unlock(&fs_info->fs_roots_lock);
>>> 4546
>>> 4547 for (i = 0; i < grabbed; i++) {
>>> 4548 if (!roots[i])
>>> 4549 continue;
>>> 4550 index = roots[i]->root_key.objectid;
>>> 4551 err = btrfs_orphan_cleanup(roots[i]);
>>> 4552 if (err)
>>> 4553 break;
>
> breaks out of for loop.
However, this is indeed a genuine bug, and the fix for it is:
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index fe309db9f5ff..f33093513360 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4549,12 +4549,13 @@ int btrfs_cleanup_fs_roots(struct btrfs_fs_info
*fs_info)
index = roots[i]->root_key.objectid;
err = btrfs_orphan_cleanup(roots[i]);
if (err)
- break;
+ goto out;
btrfs_put_root(roots[i]);
}
index++;
}
+out:
/* Release the roots that remain uncleaned due to error */
for (; i < grabbed; i++) {
if (roots[i])
>
>>> 4554 btrfs_put_root(roots[i]);
>>> 4555 }
>>> 4556 index++;
>>> 4557 }
>>> 4558
>>> 4559 /* Release the roots that remain uncleaned due to error */
>>> --> 4560 for (; i < grabbed; i++) {
>>>
>>> This code is unreachable now.
>>
>> How is it unreachable, if we error in the middle of btrfs_orphan_cleanup,
>> we'd break and this loop will cleanup the rest of the roots,
>
> There are breaks inside the inner loops but nothing breaks out of the
> while (1) loop.
>
> regards,
> dan carpenter
>
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [bug report] btrfs: turn fs_roots_radix in btrfs_fs_info into an XArray
2022-05-16 11:45 ` Nikolay Borisov
@ 2022-05-16 15:18 ` David Sterba
0 siblings, 0 replies; 7+ messages in thread
From: David Sterba @ 2022-05-16 15:18 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: Dan Carpenter, gniebler, linux-btrfs
On Mon, May 16, 2022 at 02:45:57PM +0300, Nikolay Borisov wrote:
> On 16.05.22 г. 14:02 ч., Dan Carpenter wrote:
> > On Mon, May 16, 2022 at 01:39:52PM +0300, Nikolay Borisov wrote:
> >> On 16.05.22 г. 11:04 ч., Dan Carpenter wrote:
> >>> 4538 xa_for_each_start(&fs_info->fs_roots, index, root, index) {
> >>> 4539 /* Avoid grabbing roots in dead_roots */
> >>> 4540 if (btrfs_root_refs(&root->root_item) > 0)
> >>> 4541 roots[grabbed++] = btrfs_grab_root(root);
> >>> 4542 if (grabbed >= ARRAY_SIZE(roots))
> >>> 4543 break;
> >
> > breaks out of xa_for_each_start() loop.
>
> This is fine, as when we fill root[] then we'd process it in the
> subsequent for loop.
>
> >
> >>> 4544 }
> >>> 4545 spin_unlock(&fs_info->fs_roots_lock);
> >>> 4546
> >>> 4547 for (i = 0; i < grabbed; i++) {
> >>> 4548 if (!roots[i])
> >>> 4549 continue;
> >>> 4550 index = roots[i]->root_key.objectid;
> >>> 4551 err = btrfs_orphan_cleanup(roots[i]);
> >>> 4552 if (err)
> >>> 4553 break;
> >
> > breaks out of for loop.
>
> However, this is indeed a genuine bug, and the fix for it is:
>
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index fe309db9f5ff..f33093513360 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4549,12 +4549,13 @@ int btrfs_cleanup_fs_roots(struct btrfs_fs_info
> *fs_info)
> index = roots[i]->root_key.objectid;
> err = btrfs_orphan_cleanup(roots[i]);
> if (err)
> - break;
> + goto out;
> btrfs_put_root(roots[i]);
> }
> index++;
> }
>
> +out:
> /* Release the roots that remain uncleaned due to error */
> for (; i < grabbed; i++) {
> if (roots[i])
Thanks Dan for the report, fixup folded and pushed.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-05-16 15:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-16 8:04 [bug report] btrfs: turn fs_roots_radix in btrfs_fs_info into an XArray Dan Carpenter
2022-05-16 10:39 ` Nikolay Borisov
2022-05-16 11:02 ` Dan Carpenter
2022-05-16 11:45 ` Nikolay Borisov
2022-05-16 15:18 ` David Sterba
-- strict thread matches above, loose matches on Subject: below --
2022-05-04 15:29 Dan Carpenter
2022-05-11 13:10 ` David Sterba
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox