Re: [bcache:nvdimm-meta 11/12] drivers/md/bcache/journal.c:114 journal_read_bucket() error: potentially dereferencing uninitialized 'j'.

[Coly Li]

On 8/5/21 7:18 PM, Dan Carpenter wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/colyli/linux-bcache.git nvdimm-meta
> head:   a12f8ec824edd1317f14882c7d0aee5e5c941edd
> commit: 5f408d113974d2bb3eb1b237d549724f7509ab23 [11/12] bcache: read jset from NVDIMM pages for journal replay
> config: x86_64-randconfig-m001-20210804 (attached as .config)
> compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
>
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Reported-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
>
> smatch warnings:
> drivers/md/bcache/journal.c:114 journal_read_bucket() error: potentially dereferencing uninitialized 'j'.
>
> vim +/j +114 drivers/md/bcache/journal.c
>
> cafe563591446c Kent Overstreet   2013-03-23  106  
> cafe563591446c Kent Overstreet   2013-03-23  107  		/* This function could be simpler now since we no longer write
> cafe563591446c Kent Overstreet   2013-03-23  108  		 * journal entries that overlap bucket boundaries; this means
> cafe563591446c Kent Overstreet   2013-03-23  109  		 * the start of a bucket will always have a valid journal entry
> cafe563591446c Kent Overstreet   2013-03-23  110  		 * if it has any journal entries at all.
> cafe563591446c Kent Overstreet   2013-03-23  111  		 */
>
> On my kernel there is a "j = data;" line here, but I guess it got
> removed so that's why Smatch is complaining?

Removing "j = data" is on purpose, the jset *j is initialized by the
previous code block which I list here,
  96         while (offset < ca->sb.bucket_size) {
  97 reread:         left = ca->sb.bucket_size - offset;
  98                 len = min_t(unsigned int, left, PAGE_SECTORS <<
JSET_BITS);
  99
 100                 if (!bch_has_feature_nvdimm_meta(&ca->sb))
 101                         j = __jnl_rd_bkt(ca, bucket_index, len,
offset, &cl);
 102 #if defined(CONFIG_BCACHE_NVM_PAGES)
 103                 else
 104                         j = __jnl_rd_nvm_bkt(ca, bucket_index, len,
offset);
 105 #endif
 106
 107                 /* This function could be simpler now since we no
longer write
 108                  * journal entries that overlap bucket boundaries;
this means
 109                  * the start of a bucket will always have a valid
journal entry
 110                  * if it has any journal entries at all.
 111                  */
 112                 while (len) {

jset *j is initialized at line 101 for non CONFIG_BCACHE_NVM_PAGES
condition, and at line 104 for configed CONFIG_BCACHE_NVM_PAGES condition.

The warning might be from a missing condition check for "else if
(bch_has_feature_nvdimm_meta(&ca->sb))" in code block line 100 to line
105. The static checking tool may think for such condition branch, jset
*j is undefined and referenced by the following code. But if
CONFIG_BCACHE_NVM_PAGES is not configured, such condition branch won't
happen, the supported feature set checking will make sure if the cache
device is formatted to use nvdimm but the kernel does not support yet,
the kernel will report unsupported feature and fail the registration.

Your remind is informative and helpful, before reconstruct  the source
code files to handle the config condition more clean, I will add code
comments at line 102 to explain how the undefined jset *j won't happen.

Thanks.

Coly Li

> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@xxxxxxxxxxxx
>