* [bug report] f2fs: use rb_*_cached friends
@ 2018-10-11 12:23 Dan Carpenter
2018-10-12 14:00 ` Chao Yu
0 siblings, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2018-10-11 12:23 UTC (permalink / raw)
To: yuchao0; +Cc: linux-f2fs-devel
Hello Chao Yu,
The patch df634f444ee9: "f2fs: use rb_*_cached friends" from Oct 4,
2018, leads to the following static checker warning:
fs/f2fs/extent_cache.c:606 f2fs_update_extent_tree_range()
error: uninitialized symbol 'leftmost'.
fs/f2fs/extent_cache.c
497 static void f2fs_update_extent_tree_range(struct inode *inode,
498 pgoff_t fofs, block_t blkaddr, unsigned int len)
499 {
500 struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
501 struct extent_tree *et = F2FS_I(inode)->extent_tree;
502 struct extent_node *en = NULL, *en1 = NULL;
503 struct extent_node *prev_en = NULL, *next_en = NULL;
504 struct extent_info ei, dei, prev;
505 struct rb_node **insert_p = NULL, *insert_parent = NULL;
506 unsigned int end = fofs + len;
507 unsigned int pos = (unsigned int)fofs;
508 bool updated = false;
509 bool leftmost;
510
511 if (!et)
512 return;
513
514 trace_f2fs_update_extent_tree_range(inode, fofs, blkaddr, len);
515
516 write_lock(&et->lock);
517
518 if (is_inode_flag_set(inode, FI_NO_EXTENT)) {
519 write_unlock(&et->lock);
520 return;
521 }
522
523 prev = et->largest;
524 dei.len = 0;
525
526 /*
527 * drop largest extent before lookup, in case it's already
528 * been shrunk from extent tree
529 */
530 __drop_largest_extent(et, fofs, len);
531
532 /* 1. lookup first extent node in range [fofs, fofs + len - 1] */
533 en = (struct extent_node *)f2fs_lookup_rb_tree_ret(&et->root,
534 (struct rb_entry *)et->cached_en, fofs,
535 (struct rb_entry **)&prev_en,
536 (struct rb_entry **)&next_en,
537 &insert_p, &insert_parent, false,
538 &leftmost);
^^^^^^^^
Not always initialized in there.
539 if (!en)
540 en = next_en;
541
542 /* 2. invlidate all extent nodes in range [fofs, fofs + len - 1] */
543 while (en && en->ei.fofs < end) {
544 unsigned int org_end;
545 int parts = 0; /* # of parts current extent split into */
546
547 next_en = en1 = NULL;
548
549 dei = en->ei;
550 org_end = dei.fofs + dei.len;
551 f2fs_bug_on(sbi, pos >= org_end);
552
553 if (pos > dei.fofs && pos - dei.fofs >= F2FS_MIN_EXTENT_LEN) {
554 en->ei.len = pos - en->ei.fofs;
555 prev_en = en;
556 parts = 1;
557 }
558
559 if (end < org_end && org_end - end >= F2FS_MIN_EXTENT_LEN) {
560 if (parts) {
561 set_extent_info(&ei, end,
562 end - dei.fofs + dei.blk,
563 org_end - end);
564 en1 = __insert_extent_tree(sbi, et, &ei,
565 NULL, NULL, true);
566 next_en = en1;
567 } else {
568 en->ei.fofs = end;
569 en->ei.blk += end - dei.fofs;
570 en->ei.len -= end - dei.fofs;
571 next_en = en;
572 }
573 parts++;
574 }
575
576 if (!next_en) {
577 struct rb_node *node = rb_next(&en->rb_node);
578
579 next_en = rb_entry_safe(node, struct extent_node,
580 rb_node);
581 }
582
583 if (parts)
584 __try_update_largest_extent(et, en);
585 else
586 __release_extent_node(sbi, et, en);
587
588 /*
589 * if original extent is split into zero or two parts, extent
590 * tree has been altered by deletion or insertion, therefore
591 * invalidate pointers regard to tree.
592 */
593 if (parts != 1) {
594 insert_p = NULL;
595 insert_parent = NULL;
596 }
597 en = next_en;
598 }
599
600 /* 3. update extent in extent cache */
601 if (blkaddr) {
602
603 set_extent_info(&ei, fofs, blkaddr, len);
604 if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en))
605 __insert_extent_tree(sbi, et, &ei,
606 insert_p, insert_parent, leftmost);
^^^^^^^^
Smatch complains, but I'm to stupid to know if it's valid.
607
608 /* give up extent_cache, if split and small updates happen */
609 if (dei.len >= 1 &&
610 prev.len < F2FS_MIN_EXTENT_LEN &&
611 et->largest.len < F2FS_MIN_EXTENT_LEN) {
612 et->largest.len = 0;
613 et->largest_updated = true;
614 set_inode_flag(inode, FI_NO_EXTENT);
615 }
616 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] f2fs: use rb_*_cached friends
2018-10-11 12:23 [bug report] f2fs: use rb_*_cached friends Dan Carpenter
@ 2018-10-12 14:00 ` Chao Yu
2018-10-12 14:18 ` Dan Carpenter
2019-01-10 22:51 ` Eric Biggers
0 siblings, 2 replies; 7+ messages in thread
From: Chao Yu @ 2018-10-12 14:00 UTC (permalink / raw)
To: Dan Carpenter, yuchao0; +Cc: linux-f2fs-devel
Hi Dan,
Firstly, thanks for the report. :)
On 2018-10-11 20:23, Dan Carpenter wrote:
> Hello Chao Yu,
>
> The patch df634f444ee9: "f2fs: use rb_*_cached friends" from Oct 4,
> 2018, leads to the following static checker warning:
>
> fs/f2fs/extent_cache.c:606 f2fs_update_extent_tree_range()
> error: uninitialized symbol 'leftmost'.
>
> fs/f2fs/extent_cache.c
> 497 static void f2fs_update_extent_tree_range(struct inode *inode,
> 498 pgoff_t fofs, block_t blkaddr, unsigned int len)
> 499 {
> 500 struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> 501 struct extent_tree *et = F2FS_I(inode)->extent_tree;
> 502 struct extent_node *en = NULL, *en1 = NULL;
> 503 struct extent_node *prev_en = NULL, *next_en = NULL;
> 504 struct extent_info ei, dei, prev;
> 505 struct rb_node **insert_p = NULL, *insert_parent = NULL;
> 506 unsigned int end = fofs + len;
> 507 unsigned int pos = (unsigned int)fofs;
> 508 bool updated = false;
> 509 bool leftmost;
> 510
> 511 if (!et)
> 512 return;
> 513
> 514 trace_f2fs_update_extent_tree_range(inode, fofs, blkaddr, len);
> 515
> 516 write_lock(&et->lock);
> 517
> 518 if (is_inode_flag_set(inode, FI_NO_EXTENT)) {
> 519 write_unlock(&et->lock);
> 520 return;
> 521 }
> 522
> 523 prev = et->largest;
> 524 dei.len = 0;
> 525
> 526 /*
> 527 * drop largest extent before lookup, in case it's already
> 528 * been shrunk from extent tree
> 529 */
> 530 __drop_largest_extent(et, fofs, len);
> 531
> 532 /* 1. lookup first extent node in range [fofs, fofs + len - 1] */
> 533 en = (struct extent_node *)f2fs_lookup_rb_tree_ret(&et->root,
> 534 (struct rb_entry *)et->cached_en, fofs,
> 535 (struct rb_entry **)&prev_en,
> 536 (struct rb_entry **)&next_en,
> 537 &insert_p, &insert_parent, false,
> 538 &leftmost);
> ^^^^^^^^
> Not always initialized in there.
I think the behavior should be acting as designed.
f2fs_lookup_rb_tree_ret()
{
...
*insert_p = NULL;
*insert_parent = NULL;
*prev_entry = NULL;
*next_entry = NULL;
if (RB_EMPTY_ROOT(&root->rb_root))
return NULL;
if (re) {
if (re->ofs <= ofs && re->ofs + re->len > ofs)
goto lookup_neighbors;
}
Until here, @leftmost has not been initialized, but both *insert_p and
*insert_parent are NULL,
if (leftmost)
*leftmost = true;
...
}
Later in __insert_extent_tree()
we will not hit below condition because insert_p & insert_parent are not valid:
if (insert_p && insert_parent) {
parent = insert_parent;
p = insert_p;
goto do_insert;
}
leftmost = true;
So finally, we will initialize leftmost's value here, anyway, I think there is
such place we use an uninitialized variable.
BTW, do we have any chance to detect such case in smatch?
Thanks,
>
> 539 if (!en)
> 540 en = next_en;
> 541
> 542 /* 2. invlidate all extent nodes in range [fofs, fofs + len - 1] */
> 543 while (en && en->ei.fofs < end) {
> 544 unsigned int org_end;
> 545 int parts = 0; /* # of parts current extent split into */
> 546
> 547 next_en = en1 = NULL;
> 548
> 549 dei = en->ei;
> 550 org_end = dei.fofs + dei.len;
> 551 f2fs_bug_on(sbi, pos >= org_end);
> 552
> 553 if (pos > dei.fofs && pos - dei.fofs >= F2FS_MIN_EXTENT_LEN) {
> 554 en->ei.len = pos - en->ei.fofs;
> 555 prev_en = en;
> 556 parts = 1;
> 557 }
> 558
> 559 if (end < org_end && org_end - end >= F2FS_MIN_EXTENT_LEN) {
> 560 if (parts) {
> 561 set_extent_info(&ei, end,
> 562 end - dei.fofs + dei.blk,
> 563 org_end - end);
> 564 en1 = __insert_extent_tree(sbi, et, &ei,
> 565 NULL, NULL, true);
> 566 next_en = en1;
> 567 } else {
> 568 en->ei.fofs = end;
> 569 en->ei.blk += end - dei.fofs;
> 570 en->ei.len -= end - dei.fofs;
> 571 next_en = en;
> 572 }
> 573 parts++;
> 574 }
> 575
> 576 if (!next_en) {
> 577 struct rb_node *node = rb_next(&en->rb_node);
> 578
> 579 next_en = rb_entry_safe(node, struct extent_node,
> 580 rb_node);
> 581 }
> 582
> 583 if (parts)
> 584 __try_update_largest_extent(et, en);
> 585 else
> 586 __release_extent_node(sbi, et, en);
> 587
> 588 /*
> 589 * if original extent is split into zero or two parts, extent
> 590 * tree has been altered by deletion or insertion, therefore
> 591 * invalidate pointers regard to tree.
> 592 */
> 593 if (parts != 1) {
> 594 insert_p = NULL;
> 595 insert_parent = NULL;
> 596 }
> 597 en = next_en;
> 598 }
> 599
> 600 /* 3. update extent in extent cache */
> 601 if (blkaddr) {
> 602
> 603 set_extent_info(&ei, fofs, blkaddr, len);
> 604 if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en))
> 605 __insert_extent_tree(sbi, et, &ei,
> 606 insert_p, insert_parent, leftmost);
> ^^^^^^^^
> Smatch complains, but I'm to stupid to know if it's valid.
>
> 607
> 608 /* give up extent_cache, if split and small updates happen */
> 609 if (dei.len >= 1 &&
> 610 prev.len < F2FS_MIN_EXTENT_LEN &&
> 611 et->largest.len < F2FS_MIN_EXTENT_LEN) {
> 612 et->largest.len = 0;
> 613 et->largest_updated = true;
> 614 set_inode_flag(inode, FI_NO_EXTENT);
> 615 }
> 616 }
>
> regards,
> dan carpenter
>
>
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] f2fs: use rb_*_cached friends
2018-10-12 14:00 ` Chao Yu
@ 2018-10-12 14:18 ` Dan Carpenter
2018-10-12 14:31 ` Chao Yu
2019-01-10 22:51 ` Eric Biggers
1 sibling, 1 reply; 7+ messages in thread
From: Dan Carpenter @ 2018-10-12 14:18 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-f2fs-devel
On Fri, Oct 12, 2018 at 10:00:20PM +0800, Chao Yu wrote:
>
> So finally, we will initialize leftmost's value here, anyway, I think there is
> such place we use an uninitialized variable.
>
> BTW, do we have any chance to detect such case in smatch?
>
Thanks for taking a look at this! I think that's probably a bit
complicated for Smatch...
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] f2fs: use rb_*_cached friends
2018-10-12 14:18 ` Dan Carpenter
@ 2018-10-12 14:31 ` Chao Yu
0 siblings, 0 replies; 7+ messages in thread
From: Chao Yu @ 2018-10-12 14:31 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-f2fs-devel
On 2018-10-12 22:18, Dan Carpenter wrote:
> On Fri, Oct 12, 2018 at 10:00:20PM +0800, Chao Yu wrote:
>>
>> So finally, we will initialize leftmost's value here, anyway, I think there is
Sorry, I mean 'there is no such place...'
>> such place we use an uninitialized variable.
>>
>> BTW, do we have any chance to detect such case in smatch?
>>
>
> Thanks for taking a look at this! I think that's probably a bit
> complicated for Smatch...
Alright, actually, the logic would be a little complicated. :)
Thanks,
>
> regards,
> dan carpenter
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] f2fs: use rb_*_cached friends
2018-10-12 14:00 ` Chao Yu
2018-10-12 14:18 ` Dan Carpenter
@ 2019-01-10 22:51 ` Eric Biggers
2019-01-11 2:00 ` Chao Yu
2019-01-11 6:55 ` Dan Carpenter
1 sibling, 2 replies; 7+ messages in thread
From: Eric Biggers @ 2019-01-10 22:51 UTC (permalink / raw)
To: Chao Yu; +Cc: Dan Carpenter, linux-f2fs-devel
Hi Chao,
On Fri, Oct 12, 2018 at 10:00:20PM +0800, Chao Yu wrote:
> Hi Dan,
>
> Firstly, thanks for the report. :)
>
> On 2018-10-11 20:23, Dan Carpenter wrote:
> > Hello Chao Yu,
> >
> > The patch df634f444ee9: "f2fs: use rb_*_cached friends" from Oct 4,
> > 2018, leads to the following static checker warning:
> >
> > fs/f2fs/extent_cache.c:606 f2fs_update_extent_tree_range()
> > error: uninitialized symbol 'leftmost'.
> >
> > fs/f2fs/extent_cache.c
> > 497 static void f2fs_update_extent_tree_range(struct inode *inode,
> > 498 pgoff_t fofs, block_t blkaddr, unsigned int len)
> > 499 {
> > 500 struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
> > 501 struct extent_tree *et = F2FS_I(inode)->extent_tree;
> > 502 struct extent_node *en = NULL, *en1 = NULL;
> > 503 struct extent_node *prev_en = NULL, *next_en = NULL;
> > 504 struct extent_info ei, dei, prev;
> > 505 struct rb_node **insert_p = NULL, *insert_parent = NULL;
> > 506 unsigned int end = fofs + len;
> > 507 unsigned int pos = (unsigned int)fofs;
> > 508 bool updated = false;
> > 509 bool leftmost;
> > 510
> > 511 if (!et)
> > 512 return;
> > 513
> > 514 trace_f2fs_update_extent_tree_range(inode, fofs, blkaddr, len);
> > 515
> > 516 write_lock(&et->lock);
> > 517
> > 518 if (is_inode_flag_set(inode, FI_NO_EXTENT)) {
> > 519 write_unlock(&et->lock);
> > 520 return;
> > 521 }
> > 522
> > 523 prev = et->largest;
> > 524 dei.len = 0;
> > 525
> > 526 /*
> > 527 * drop largest extent before lookup, in case it's already
> > 528 * been shrunk from extent tree
> > 529 */
> > 530 __drop_largest_extent(et, fofs, len);
> > 531
> > 532 /* 1. lookup first extent node in range [fofs, fofs + len - 1] */
> > 533 en = (struct extent_node *)f2fs_lookup_rb_tree_ret(&et->root,
> > 534 (struct rb_entry *)et->cached_en, fofs,
> > 535 (struct rb_entry **)&prev_en,
> > 536 (struct rb_entry **)&next_en,
> > 537 &insert_p, &insert_parent, false,
> > 538 &leftmost);
> > ^^^^^^^^
> > Not always initialized in there.
>
> I think the behavior should be acting as designed.
>
> f2fs_lookup_rb_tree_ret()
> {
> ...
> *insert_p = NULL;
> *insert_parent = NULL;
> *prev_entry = NULL;
> *next_entry = NULL;
>
> if (RB_EMPTY_ROOT(&root->rb_root))
> return NULL;
>
> if (re) {
> if (re->ofs <= ofs && re->ofs + re->len > ofs)
> goto lookup_neighbors;
> }
>
> Until here, @leftmost has not been initialized, but both *insert_p and
> *insert_parent are NULL,
>
> if (leftmost)
> *leftmost = true;
> ...
> }
>
> Later in __insert_extent_tree()
>
> we will not hit below condition because insert_p & insert_parent are not valid:
> if (insert_p && insert_parent) {
> parent = insert_parent;
> p = insert_p;
> goto do_insert;
> }
>
> leftmost = true;
>
> So finally, we will initialize leftmost's value here, anyway, I think there is
> such place we use an uninitialized variable.
>
> BTW, do we have any chance to detect such case in smatch?
>
> Thanks,
>
> >
> > 539 if (!en)
> > 540 en = next_en;
> > 541
> > 542 /* 2. invlidate all extent nodes in range [fofs, fofs + len - 1] */
> > 543 while (en && en->ei.fofs < end) {
> > 544 unsigned int org_end;
> > 545 int parts = 0; /* # of parts current extent split into */
> > 546
> > 547 next_en = en1 = NULL;
> > 548
> > 549 dei = en->ei;
> > 550 org_end = dei.fofs + dei.len;
> > 551 f2fs_bug_on(sbi, pos >= org_end);
> > 552
> > 553 if (pos > dei.fofs && pos - dei.fofs >= F2FS_MIN_EXTENT_LEN) {
> > 554 en->ei.len = pos - en->ei.fofs;
> > 555 prev_en = en;
> > 556 parts = 1;
> > 557 }
> > 558
> > 559 if (end < org_end && org_end - end >= F2FS_MIN_EXTENT_LEN) {
> > 560 if (parts) {
> > 561 set_extent_info(&ei, end,
> > 562 end - dei.fofs + dei.blk,
> > 563 org_end - end);
> > 564 en1 = __insert_extent_tree(sbi, et, &ei,
> > 565 NULL, NULL, true);
> > 566 next_en = en1;
> > 567 } else {
> > 568 en->ei.fofs = end;
> > 569 en->ei.blk += end - dei.fofs;
> > 570 en->ei.len -= end - dei.fofs;
> > 571 next_en = en;
> > 572 }
> > 573 parts++;
> > 574 }
> > 575
> > 576 if (!next_en) {
> > 577 struct rb_node *node = rb_next(&en->rb_node);
> > 578
> > 579 next_en = rb_entry_safe(node, struct extent_node,
> > 580 rb_node);
> > 581 }
> > 582
> > 583 if (parts)
> > 584 __try_update_largest_extent(et, en);
> > 585 else
> > 586 __release_extent_node(sbi, et, en);
> > 587
> > 588 /*
> > 589 * if original extent is split into zero or two parts, extent
> > 590 * tree has been altered by deletion or insertion, therefore
> > 591 * invalidate pointers regard to tree.
> > 592 */
> > 593 if (parts != 1) {
> > 594 insert_p = NULL;
> > 595 insert_parent = NULL;
> > 596 }
> > 597 en = next_en;
> > 598 }
> > 599
> > 600 /* 3. update extent in extent cache */
> > 601 if (blkaddr) {
> > 602
> > 603 set_extent_info(&ei, fofs, blkaddr, len);
> > 604 if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en))
> > 605 __insert_extent_tree(sbi, et, &ei,
> > 606 insert_p, insert_parent, leftmost);
> > ^^^^^^^^
> > Smatch complains, but I'm to stupid to know if it's valid.
> >
> > 607
> > 608 /* give up extent_cache, if split and small updates happen */
> > 609 if (dei.len >= 1 &&
> > 610 prev.len < F2FS_MIN_EXTENT_LEN &&
> > 611 et->largest.len < F2FS_MIN_EXTENT_LEN) {
> > 612 et->largest.len = 0;
> > 613 et->largest_updated = true;
> > 614 set_inode_flag(inode, FI_NO_EXTENT);
> > 615 }
> > 616 }
> >
> > regards,
> > dan carpenter
> >
I get an UBSAN warning on this same line, so this seems to be a real bug.
It goes away if I initialize 'leftmost'.
To reproduce, build a kernel with CONFIG_F2FS_FS=y CONFIG_F2FS_FS_ENCRYPTION=y
CONFIG_UBSAN=y CONFIG_KASAN=y and run 'kvm-xfstests -c f2fs generic/397'.
(There's probably a better reproducer, but that's how I saw it.)
This is on latest mainline.
================================================================================
UBSAN: Undefined behaviour in fs/f2fs/extent_cache.c:605:4
load of value 255 is not a valid value for type '_Bool'
CPU: 0 PID: 94 Comm: kworker/u4:2 Not tainted 5.0.0-rc1-00043-g1bdbe22749207 #14
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
Workqueue: writeback wb_workfn (flush-254:32)
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x70/0xa5 lib/dump_stack.c:113
ubsan_epilogue+0xd/0x40 lib/ubsan.c:159
__ubsan_handle_load_invalid_value+0x8e/0xa0 lib/ubsan.c:457
f2fs_update_extent_tree_range+0x6e9/0x760 fs/f2fs/extent_cache.c:605
f2fs_update_extent_cache+0xce/0xf0 fs/f2fs/extent_cache.c:804
f2fs_update_data_blkaddr+0x18/0x20 fs/f2fs/data.c:639
f2fs_outplace_write_data+0x63/0x130 fs/f2fs/segment.c:3147
f2fs_do_write_data_page+0x3ae/0x7e0 fs/f2fs/data.c:1892
__write_data_page+0x734/0xd00 fs/f2fs/data.c:1993
f2fs_write_cache_pages+0x1fb/0x610 fs/f2fs/data.c:2160
__f2fs_write_data_pages fs/f2fs/data.c:2273 [inline]
f2fs_write_data_pages+0x3dc/0x450 fs/f2fs/data.c:2300
do_writepages+0x43/0xf0 mm/page-writeback.c:2335
__writeback_single_inode+0x56/0x660 fs/fs-writeback.c:1316
writeback_sb_inodes+0x20b/0x6b0 fs/fs-writeback.c:1580
wb_writeback+0x10f/0x510 fs/fs-writeback.c:1756
wb_do_writeback fs/fs-writeback.c:1901 [inline]
wb_workfn+0xa7/0x670 fs/fs-writeback.c:1942
process_one_work+0x25d/0x670 kernel/workqueue.c:2153
worker_thread+0x3e/0x3d0 kernel/workqueue.c:2296
kthread+0x124/0x140 kernel/kthread.c:246
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
================================================================================
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] f2fs: use rb_*_cached friends
2019-01-10 22:51 ` Eric Biggers
@ 2019-01-11 2:00 ` Chao Yu
2019-01-11 6:55 ` Dan Carpenter
1 sibling, 0 replies; 7+ messages in thread
From: Chao Yu @ 2019-01-11 2:00 UTC (permalink / raw)
To: Eric Biggers, Chao Yu; +Cc: Dan Carpenter, linux-f2fs-devel
Hi Eric,
On 2019/1/11 6:51, Eric Biggers wrote:
> Hi Chao,
>
> On Fri, Oct 12, 2018 at 10:00:20PM +0800, Chao Yu wrote:
>> Hi Dan,
>>
>> Firstly, thanks for the report. :)
>>
>> On 2018-10-11 20:23, Dan Carpenter wrote:
>>> Hello Chao Yu,
>>>
>>> The patch df634f444ee9: "f2fs: use rb_*_cached friends" from Oct 4,
>>> 2018, leads to the following static checker warning:
>>>
>>> fs/f2fs/extent_cache.c:606 f2fs_update_extent_tree_range()
>>> error: uninitialized symbol 'leftmost'.
>>>
>>> fs/f2fs/extent_cache.c
>>> 497 static void f2fs_update_extent_tree_range(struct inode *inode,
>>> 498 pgoff_t fofs, block_t blkaddr, unsigned int len)
>>> 499 {
>>> 500 struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>>> 501 struct extent_tree *et = F2FS_I(inode)->extent_tree;
>>> 502 struct extent_node *en = NULL, *en1 = NULL;
>>> 503 struct extent_node *prev_en = NULL, *next_en = NULL;
>>> 504 struct extent_info ei, dei, prev;
>>> 505 struct rb_node **insert_p = NULL, *insert_parent = NULL;
>>> 506 unsigned int end = fofs + len;
>>> 507 unsigned int pos = (unsigned int)fofs;
>>> 508 bool updated = false;
>>> 509 bool leftmost;
>>> 510
>>> 511 if (!et)
>>> 512 return;
>>> 513
>>> 514 trace_f2fs_update_extent_tree_range(inode, fofs, blkaddr, len);
>>> 515
>>> 516 write_lock(&et->lock);
>>> 517
>>> 518 if (is_inode_flag_set(inode, FI_NO_EXTENT)) {
>>> 519 write_unlock(&et->lock);
>>> 520 return;
>>> 521 }
>>> 522
>>> 523 prev = et->largest;
>>> 524 dei.len = 0;
>>> 525
>>> 526 /*
>>> 527 * drop largest extent before lookup, in case it's already
>>> 528 * been shrunk from extent tree
>>> 529 */
>>> 530 __drop_largest_extent(et, fofs, len);
>>> 531
>>> 532 /* 1. lookup first extent node in range [fofs, fofs + len - 1] */
>>> 533 en = (struct extent_node *)f2fs_lookup_rb_tree_ret(&et->root,
>>> 534 (struct rb_entry *)et->cached_en, fofs,
>>> 535 (struct rb_entry **)&prev_en,
>>> 536 (struct rb_entry **)&next_en,
>>> 537 &insert_p, &insert_parent, false,
>>> 538 &leftmost);
>>> ^^^^^^^^
>>> Not always initialized in there.
>>
>> I think the behavior should be acting as designed.
>>
>> f2fs_lookup_rb_tree_ret()
>> {
>> ...
>> *insert_p = NULL;
>> *insert_parent = NULL;
>> *prev_entry = NULL;
>> *next_entry = NULL;
>>
>> if (RB_EMPTY_ROOT(&root->rb_root))
>> return NULL;
>>
>> if (re) {
>> if (re->ofs <= ofs && re->ofs + re->len > ofs)
>> goto lookup_neighbors;
>> }
>>
>> Until here, @leftmost has not been initialized, but both *insert_p and
>> *insert_parent are NULL,
>>
>> if (leftmost)
>> *leftmost = true;
>> ...
>> }
>>
>> Later in __insert_extent_tree()
>>
>> we will not hit below condition because insert_p & insert_parent are not valid:
>> if (insert_p && insert_parent) {
>> parent = insert_parent;
>> p = insert_p;
>> goto do_insert;
>> }
>>
>> leftmost = true;
>>
>> So finally, we will initialize leftmost's value here, anyway, I think there is
>> such place we use an uninitialized variable.
>>
>> BTW, do we have any chance to detect such case in smatch?
>>
>> Thanks,
>>
>>>
>>> 539 if (!en)
>>> 540 en = next_en;
>>> 541
>>> 542 /* 2. invlidate all extent nodes in range [fofs, fofs + len - 1] */
>>> 543 while (en && en->ei.fofs < end) {
>>> 544 unsigned int org_end;
>>> 545 int parts = 0; /* # of parts current extent split into */
>>> 546
>>> 547 next_en = en1 = NULL;
>>> 548
>>> 549 dei = en->ei;
>>> 550 org_end = dei.fofs + dei.len;
>>> 551 f2fs_bug_on(sbi, pos >= org_end);
>>> 552
>>> 553 if (pos > dei.fofs && pos - dei.fofs >= F2FS_MIN_EXTENT_LEN) {
>>> 554 en->ei.len = pos - en->ei.fofs;
>>> 555 prev_en = en;
>>> 556 parts = 1;
>>> 557 }
>>> 558
>>> 559 if (end < org_end && org_end - end >= F2FS_MIN_EXTENT_LEN) {
>>> 560 if (parts) {
>>> 561 set_extent_info(&ei, end,
>>> 562 end - dei.fofs + dei.blk,
>>> 563 org_end - end);
>>> 564 en1 = __insert_extent_tree(sbi, et, &ei,
>>> 565 NULL, NULL, true);
>>> 566 next_en = en1;
>>> 567 } else {
>>> 568 en->ei.fofs = end;
>>> 569 en->ei.blk += end - dei.fofs;
>>> 570 en->ei.len -= end - dei.fofs;
>>> 571 next_en = en;
>>> 572 }
>>> 573 parts++;
>>> 574 }
>>> 575
>>> 576 if (!next_en) {
>>> 577 struct rb_node *node = rb_next(&en->rb_node);
>>> 578
>>> 579 next_en = rb_entry_safe(node, struct extent_node,
>>> 580 rb_node);
>>> 581 }
>>> 582
>>> 583 if (parts)
>>> 584 __try_update_largest_extent(et, en);
>>> 585 else
>>> 586 __release_extent_node(sbi, et, en);
>>> 587
>>> 588 /*
>>> 589 * if original extent is split into zero or two parts, extent
>>> 590 * tree has been altered by deletion or insertion, therefore
>>> 591 * invalidate pointers regard to tree.
>>> 592 */
>>> 593 if (parts != 1) {
>>> 594 insert_p = NULL;
>>> 595 insert_parent = NULL;
>>> 596 }
>>> 597 en = next_en;
>>> 598 }
>>> 599
>>> 600 /* 3. update extent in extent cache */
>>> 601 if (blkaddr) {
>>> 602
>>> 603 set_extent_info(&ei, fofs, blkaddr, len);
>>> 604 if (!__try_merge_extent_node(sbi, et, &ei, prev_en, next_en))
>>> 605 __insert_extent_tree(sbi, et, &ei,
>>> 606 insert_p, insert_parent, leftmost);
>>> ^^^^^^^^
>>> Smatch complains, but I'm to stupid to know if it's valid.
>>>
>>> 607
>>> 608 /* give up extent_cache, if split and small updates happen */
>>> 609 if (dei.len >= 1 &&
>>> 610 prev.len < F2FS_MIN_EXTENT_LEN &&
>>> 611 et->largest.len < F2FS_MIN_EXTENT_LEN) {
>>> 612 et->largest.len = 0;
>>> 613 et->largest_updated = true;
>>> 614 set_inode_flag(inode, FI_NO_EXTENT);
>>> 615 }
>>> 616 }
>>>
>>> regards,
>>> dan carpenter
>>>
>
> I get an UBSAN warning on this same line, so this seems to be a real bug.
> It goes away if I initialize 'leftmost'.
>
> To reproduce, build a kernel with CONFIG_F2FS_FS=y CONFIG_F2FS_FS_ENCRYPTION=y
> CONFIG_UBSAN=y CONFIG_KASAN=y and run 'kvm-xfstests -c f2fs generic/397'.
> (There's probably a better reproducer, but that's how I saw it.)
>
> This is on latest mainline.
>
> ================================================================================
> UBSAN: Undefined behaviour in fs/f2fs/extent_cache.c:605:4
When passing @leftmost to __insert_extent_tree() located in
extent_cache.c:605, its value could be uninitialized if we go into below
two paths, but notice that insert_p & insert_parent are both invalid.
f2fs_lookup_rb_tree_ret()
{
*insert_p = NULL;
*insert_parent = NULL;
..
if (RB_EMPTY_ROOT(&root->rb_root))
return NULL;
^^^^^^^^^^^^
if (re) {
if (re->ofs <= ofs && re->ofs + re->len > ofs)
goto lookup_neighbors;
^^^^^^^^^^^^^^^^^^^^^^
}
if (leftmost)
*leftmost = true;
}
And then, you can see that we will reinitialize leftmost value if insert_p
& insert_parent are invalid, so before we use @leftmost in
f2fs_lookup_rb_tree_for_insert and __attach_extent_node, it should be
initialized one.
__insert_extent_tree()
{
...
if (insert_p && insert_parent) {
parent = insert_parent;
p = insert_p;
goto do_insert;
}
leftmost = true;
^^^^^^^^^^^^^^^^
p = f2fs_lookup_rb_tree_for_insert(sbi, &et->root, &parent,
ei->fofs, &leftmost);
do_insert:
en = __attach_extent_node(sbi, et, ei, parent, p, leftmost);
...
}
IMO, it can cause UBSAN warning, rather than corrupt extent cache rb-tree,
but, anyway, I agree that we need to fix it to avoid UBSAN warning.
Thanks,
> load of value 255 is not a valid value for type '_Bool'
> CPU: 0 PID: 94 Comm: kworker/u4:2 Not tainted 5.0.0-rc1-00043-g1bdbe22749207 #14
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1 04/01/2014
> Workqueue: writeback wb_workfn (flush-254:32)
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x70/0xa5 lib/dump_stack.c:113
> ubsan_epilogue+0xd/0x40 lib/ubsan.c:159
> __ubsan_handle_load_invalid_value+0x8e/0xa0 lib/ubsan.c:457
> f2fs_update_extent_tree_range+0x6e9/0x760 fs/f2fs/extent_cache.c:605
> f2fs_update_extent_cache+0xce/0xf0 fs/f2fs/extent_cache.c:804
> f2fs_update_data_blkaddr+0x18/0x20 fs/f2fs/data.c:639
> f2fs_outplace_write_data+0x63/0x130 fs/f2fs/segment.c:3147
> f2fs_do_write_data_page+0x3ae/0x7e0 fs/f2fs/data.c:1892
> __write_data_page+0x734/0xd00 fs/f2fs/data.c:1993
> f2fs_write_cache_pages+0x1fb/0x610 fs/f2fs/data.c:2160
> __f2fs_write_data_pages fs/f2fs/data.c:2273 [inline]
> f2fs_write_data_pages+0x3dc/0x450 fs/f2fs/data.c:2300
> do_writepages+0x43/0xf0 mm/page-writeback.c:2335
> __writeback_single_inode+0x56/0x660 fs/fs-writeback.c:1316
> writeback_sb_inodes+0x20b/0x6b0 fs/fs-writeback.c:1580
> wb_writeback+0x10f/0x510 fs/fs-writeback.c:1756
> wb_do_writeback fs/fs-writeback.c:1901 [inline]
> wb_workfn+0xa7/0x670 fs/fs-writeback.c:1942
> process_one_work+0x25d/0x670 kernel/workqueue.c:2153
> worker_thread+0x3e/0x3d0 kernel/workqueue.c:2296
> kthread+0x124/0x140 kernel/kthread.c:246
> ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
> ================================================================================
>
> .
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [bug report] f2fs: use rb_*_cached friends
2019-01-10 22:51 ` Eric Biggers
2019-01-11 2:00 ` Chao Yu
@ 2019-01-11 6:55 ` Dan Carpenter
1 sibling, 0 replies; 7+ messages in thread
From: Dan Carpenter @ 2019-01-11 6:55 UTC (permalink / raw)
To: Eric Biggers; +Cc: linux-f2fs-devel
There have been a couple of these where we pass uninitialized memory to
another function which doesn't use it. It's kind of obvious in
retrospect, but it hadn't occured to me that UBSAN would generate the
warning at the call site. This will make us more inclined to silence
those warnings.
Thanks!
regards,
dan carpenter
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-01-11 6:55 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-11 12:23 [bug report] f2fs: use rb_*_cached friends Dan Carpenter
2018-10-12 14:00 ` Chao Yu
2018-10-12 14:18 ` Dan Carpenter
2018-10-12 14:31 ` Chao Yu
2019-01-10 22:51 ` Eric Biggers
2019-01-11 2:00 ` Chao Yu
2019-01-11 6:55 ` Dan Carpenter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.