linux-ext4 - Re: [bug report] ext4: fast commit recovery path

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 27 Oct 2020 11:00:51 -0700
From:   harshad shirwadkar <harshadshirwadkar@...il.com>
To:     Dan Carpenter <dan.carpenter@...cle.com>
Cc:     Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [bug report] ext4: fast commit recovery path

Hi Dan,

On Tue, Oct 27, 2020 at 1:10 AM Dan Carpenter <dan.carpenter@...cle.com> wrote:
>
> Hello Harshad Shirwadkar,
>
> The patch 8016e29f4362: "ext4: fast commit recovery path" from Oct
> 15, 2020, leads to the following static checker warning:
>
>         fs/ext4/fast_commit.c:1620 ext4_fc_replay_add_range()
>         warn: 'path' is an error pointer or valid
>
> fs/ext4/fast_commit.c
>   1600          cur = start;
>   1601          remaining = len;
>   1602          jbd_debug(1, "ADD_RANGE, lblk %d, pblk %lld, len %d, unwritten %d, inode %ld\n",
>   1603                    start, start_pblk, len, ext4_ext_is_unwritten(ex),
>   1604                    inode->i_ino);
>   1605
>   1606          while (remaining > 0) {
>   1607                  map.m_lblk = cur;
>   1608                  map.m_len = remaining;
>   1609                  map.m_pblk = 0;
>   1610                  ret = ext4_map_blocks(NULL, inode, &map, 0);
>   1611
>   1612                  if (ret < 0) {
>   1613                          iput(inode);
>   1614                          return 0;
>   1615                  }
>   1616
>   1617                  if (ret == 0) {
>   1618                          /* Range is not mapped */
>   1619                          path = ext4_find_extent(inode, cur, NULL, 0);
>   1620                          if (!path)
>   1621                                  continue;
>                                 ^^^^^^^^^^^^^^^^
> "path" can't be NULL, this should be an IS_ERR() test.  It's sort of
> surprising to me that we continue here instead of returning an error.
Thanks for pointing this out. We should check using IS_ERR() here.
Also, I agree that instead of "continue" we should be returning an
error. If not we'd be stuck in this loop forever.

Thanks,
Harshad
>
>   1622                          memset(&newex, 0, sizeof(newex));
>   1623                          newex.ee_block = cpu_to_le32(cur);
>   1624                          ext4_ext_store_pblock(
>   1625                                  &newex, start_pblk + cur - start);
>   1626                          newex.ee_len = cpu_to_le16(map.m_len);
>   1627                          if (ext4_ext_is_unwritten(ex))
>   1628                                  ext4_ext_mark_unwritten(&newex);
>   1629                          down_write(&EXT4_I(inode)->i_data_sem);
>   1630                          ret = ext4_ext_insert_extent(
>   1631                                  NULL, inode, &path, &newex, 0);
>   1632                          up_write((&EXT4_I(inode)->i_data_sem));
>   1633                          ext4_ext_drop_refs(path);
>   1634                          kfree(path);
>   1635                          if (ret) {
>   1636                                  iput(inode);
>   1637                                  return 0;
>   1638                          }
>   1639                          goto next;
>   1640                  }
>   1641
>   1642                  if (start_pblk + cur - start != map.m_pblk) {
>
> regards,
> dan carpenter