Hi Dan,
Thanks for your report.
On Tue, Aug 27, 2019 at 12:03:55PM +0300, Dan Carpenter wrote:
> Hello Gao Xiang,
>
> This is a semi-automatic email about new static checker warnings.
>
> The patch 97e86a858bc3: "staging: erofs: tidy up decompression
> frontend" from Jul 31, 2019, leads to the following Smatch complaint:
>
> fs/erofs/zdata.c:670 z_erofs_do_read_page()
> error: we previously assumed 'clt->cl' could be null (see line 596)
>
> fs/erofs/zdata.c
> 595 /* didn't get a valid collection previously
> (very rare) */
> 596 if (!clt->cl)
> ^^^^^^^^
> New NULL check.
>
> 597 goto restart_now;
> 598 goto hitted;
> 599 }
> 600
> 601 /* go ahead the next map_blocks */
> 602 debugln("%s: [out-of-range] pos %llu", __func__, offset
> + cur);
> 603
> 604 if (z_erofs_collector_end(clt))
> 605 fe->backmost = false;
> 606
> 607 map->m_la = offset + cur;
> 608 map->m_llen = 0;
> 609 err = z_erofs_map_blocks_iter(inode, map, 0);
> 610 if (unlikely(err))
> 611 goto err_out;
> 612
> 613 restart_now:
> 614 if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED)))
> 615 goto hitted;
> 616
> 617 err = z_erofs_collector_begin(clt, inode, map);
At a glance, clt->cl will be all initialized in all successful paths
in z_erofs_collector_begin, or it all fall back into err_out...
I have no idea what is wrong here...
Some detailed path from Smatch for NIL dereferences?
Thanks,
Gao Xiang
> 618 if (unlikely(err))
> 619 goto err_out;
> 620
> 621 /* preload all compressed pages (maybe downgrade role
> if necessary) */
> 622 if (should_alloc_managed_pages(fe, sbi->cache_strategy,
> map->m_la))
> 623 cache_strategy = DELAYEDALLOC;
> 624 else
> 625 cache_strategy = DONTALLOC;
> 626
> 627 preload_compressed_pages(clt, MNGD_MAPPING(sbi),
> 628 cache_strategy, pagepool);
> 629
> 630 tight &= (clt->mode >= COLLECT_PRIMARY_HOOKED);
> 631 hitted:
> 632 cur = end - min_t(unsigned int, offset + end -
> map->m_la, end);
> 633 if (unlikely(!(map->m_flags & EROFS_MAP_MAPPED))) {
> 634 zero_user_segment(page, cur, end);
> 635 goto next_part;
> 636 }
> 637
> 638 /* let's derive page type */
> 639 page_type = cur ? Z_EROFS_VLE_PAGE_TYPE_HEAD :
> 640 (!spiltted ? Z_EROFS_PAGE_TYPE_EXCLUSIVE :
> 641 (tight ? Z_EROFS_PAGE_TYPE_EXCLUSIVE :
> 642
> Z_EROFS_VLE_PAGE_TYPE_TAIL_SHARED));
> 643
> 644 if (cur)
> 645 tight &= (clt->mode >=
> COLLECT_PRIMARY_FOLLOWED);
> 646
> 647 retry:
> 648 err = z_erofs_attach_page(clt, page, page_type);
> 649 /* should allocate an additional staging page for
> pagevec */
> 650 if (err == -EAGAIN) {
> 651 struct page *const newpage =
> 652 __stagingpage_alloc(pagepool, GFP_NOFS);
> 653
> 654 err = z_erofs_attach_page(clt, newpage,
> 655
> Z_EROFS_PAGE_TYPE_EXCLUSIVE);
> 656 if (likely(!err))
> 657 goto retry;
> 658 }
> 659
> 660 if (unlikely(err))
> 661 goto err_out;
> 662
> 663 index = page->index - (map->m_la >> PAGE_SHIFT);
> 664
> 665 z_erofs_onlinepage_fixup(page, index, true);
> 666
> 667 /* bump up the number of spiltted parts of a page */
> 668 ++spiltted;
> 669 /* also update nr_pages */
> 670 clt->cl->nr_pages = max_t(pgoff_t, clt->cl->nr_pages,
> index + 1);
> ^^^^^^^^^^^^^^^^^ ^^^^^^^^^^^^^^^^^
> Unchecked dereferences.
>
> 671 next_part:
> 672 /* can be used for verification */
>
> regards,
> dan carpenter
