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 > ================================================================================ > > . > _______________________________________________ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel