From: Chao Yu <chao@kernel.org>
To: Dan Carpenter <dan.carpenter@oracle.com>, yuchao0@huawei.com
Cc: linux-f2fs-devel@lists.sourceforge.net
Subject: Re: [bug report] f2fs: use rb_*_cached friends
Date: Fri, 12 Oct 2018 22:00:20 +0800 [thread overview]
Message-ID: <de7d9aa6-b828-f2d9-4568-bfc758fad9d0@kernel.org> (raw)
In-Reply-To: <20181011122342.GA23299@mwanda>
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
>
next prev parent reply other threads:[~2018-10-12 14:00 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-11 12:23 [bug report] f2fs: use rb_*_cached friends Dan Carpenter
2018-10-12 14:00 ` Chao Yu [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=de7d9aa6-b828-f2d9-4568-bfc758fad9d0@kernel.org \
--to=chao@kernel.org \
--cc=dan.carpenter@oracle.com \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=yuchao0@huawei.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).