On Thu, Jul 29, 2021 at 10:27 AM Dan Carpenter wrote: > > [ I don't know why Smatch is suddenly complaining about this code when > it's 5 years old. But we may as well remove the NULL check. - dan] > > Hi, > > url: https://github.com/0day-ci/linux/commits/fdmanana-kernel-org/btrfs-fsync-changes-a-bug-fix-and-a-couple-improvements/20210727-182628 > base: https://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git for-next > config: arm64-randconfig-m031-20210728 (attached as .config) > compiler: aarch64-linux-gcc (GCC) 10.3.0 > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot > Reported-by: Dan Carpenter > > New smatch warnings: > fs/btrfs/inode.c:9338 btrfs_rename_exchange() warn: variable dereferenced before check 'new_inode' (see line 9225) > > Old smatch warnings: > fs/btrfs/inode.c:285 insert_inline_extent() error: we previously assumed 'compressed_pages' could be null (see line 254) > > vim +/new_inode +9338 fs/btrfs/inode.c > > cdd1fedf8261cd Dan Fuhry 2016-03-17 9118 static int btrfs_rename_exchange(struct inode *old_dir, > cdd1fedf8261cd Dan Fuhry 2016-03-17 9119 struct dentry *old_dentry, > cdd1fedf8261cd Dan Fuhry 2016-03-17 9120 struct inode *new_dir, > cdd1fedf8261cd Dan Fuhry 2016-03-17 9121 struct dentry *new_dentry) > cdd1fedf8261cd Dan Fuhry 2016-03-17 9122 { > 0b246afa62b0cf Jeff Mahoney 2016-06-22 9123 struct btrfs_fs_info *fs_info = btrfs_sb(old_dir->i_sb); > cdd1fedf8261cd Dan Fuhry 2016-03-17 9124 struct btrfs_trans_handle *trans; > cdd1fedf8261cd Dan Fuhry 2016-03-17 9125 struct btrfs_root *root = BTRFS_I(old_dir)->root; > cdd1fedf8261cd Dan Fuhry 2016-03-17 9126 struct btrfs_root *dest = BTRFS_I(new_dir)->root; > cdd1fedf8261cd Dan Fuhry 2016-03-17 9127 struct inode *new_inode = new_dentry->d_inode; > cdd1fedf8261cd Dan Fuhry 2016-03-17 9128 struct inode *old_inode = old_dentry->d_inode; > 95582b00838837 Deepa Dinamani 2018-05-08 9129 struct timespec64 ctime = current_time(old_inode); > 4a0cc7ca6c40b6 Nikolay Borisov 2017-01-10 9130 u64 old_ino = btrfs_ino(BTRFS_I(old_inode)); > 4a0cc7ca6c40b6 Nikolay Borisov 2017-01-10 9131 u64 new_ino = btrfs_ino(BTRFS_I(new_inode)); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > "new_inode" can't be NULL. Or more properly we are toasted if it is. > > cdd1fedf8261cd Dan Fuhry 2016-03-17 9132 u64 old_idx = 0; > cdd1fedf8261cd Dan Fuhry 2016-03-17 9133 u64 new_idx = 0; > cdd1fedf8261cd Dan Fuhry 2016-03-17 9134 int ret; > > [ snip ] > > cdd1fedf8261cd Dan Fuhry 2016-03-17 9322 out_fail: > 86e8aa0e772cab Filipe Manana 2016-05-05 9323 /* > 86e8aa0e772cab Filipe Manana 2016-05-05 9324 * If we have pinned a log and an error happened, we unpin tasks > 86e8aa0e772cab Filipe Manana 2016-05-05 9325 * trying to sync the log and force them to fallback to a transaction > 86e8aa0e772cab Filipe Manana 2016-05-05 9326 * commit if the log currently contains any of the inodes involved in > 86e8aa0e772cab Filipe Manana 2016-05-05 9327 * this rename operation (to ensure we do not persist a log with an > 86e8aa0e772cab Filipe Manana 2016-05-05 9328 * inconsistent state for any of these inodes or leading to any > 86e8aa0e772cab Filipe Manana 2016-05-05 9329 * inconsistencies when replayed). If the transaction was aborted, the > 86e8aa0e772cab Filipe Manana 2016-05-05 9330 * abortion reason is propagated to userspace when attempting to commit > 86e8aa0e772cab Filipe Manana 2016-05-05 9331 * the transaction. If the log does not contain any of these inodes, we > 86e8aa0e772cab Filipe Manana 2016-05-05 9332 * allow the tasks to sync it. > 86e8aa0e772cab Filipe Manana 2016-05-05 9333 */ > 86e8aa0e772cab Filipe Manana 2016-05-05 9334 if (ret && (root_log_pinned || dest_log_pinned)) { > 0f8939b8ac8623 Nikolay Borisov 2017-01-18 9335 if (btrfs_inode_in_log(BTRFS_I(old_dir), fs_info->generation) || > 0f8939b8ac8623 Nikolay Borisov 2017-01-18 9336 btrfs_inode_in_log(BTRFS_I(new_dir), fs_info->generation) || > 0f8939b8ac8623 Nikolay Borisov 2017-01-18 9337 btrfs_inode_in_log(BTRFS_I(old_inode), fs_info->generation) || > 86e8aa0e772cab Filipe Manana 2016-05-05 @9338 (new_inode && > ^^^^^^^^^ > This check is not required and only upsets the static checker. Indeed, it's not needed as new_inode can never be NULL for a rename exchange operation. The code was likely copy-pasted from btrfs_rename(), used for regular renames, where new_inode can be NULL. I just sent a patch to remove the check: https://lore.kernel.org/linux-btrfs/8dd8e8f3020a5bd13ae22a1f21b8328adc1f4636.1627568438.git.fdmanana(a)suse.com/ Thanks. > > 0f8939b8ac8623 Nikolay Borisov 2017-01-18 9339 btrfs_inode_in_log(BTRFS_I(new_inode), fs_info->generation))) > 907877664e2d85 David Sterba 2019-03-20 9340 btrfs_set_log_full_commit(trans); > 86e8aa0e772cab Filipe Manana 2016-05-05 9341 > 86e8aa0e772cab Filipe Manana 2016-05-05 9342 if (root_log_pinned) { > 86e8aa0e772cab Filipe Manana 2016-05-05 9343 btrfs_end_log_trans(root); > 86e8aa0e772cab Filipe Manana 2016-05-05 9344 root_log_pinned = false; > 86e8aa0e772cab Filipe Manana 2016-05-05 9345 } > 86e8aa0e772cab Filipe Manana 2016-05-05 9346 if (dest_log_pinned) { > 86e8aa0e772cab Filipe Manana 2016-05-05 9347 btrfs_end_log_trans(dest); > 86e8aa0e772cab Filipe Manana 2016-05-05 9348 dest_log_pinned = false; > 86e8aa0e772cab Filipe Manana 2016-05-05 9349 } > 86e8aa0e772cab Filipe Manana 2016-05-05 9350 } > c5b4a50b74018b Filipe Manana 2018-06-11 9351 ret2 = btrfs_end_transaction(trans); > c5b4a50b74018b Filipe Manana 2018-06-11 9352 ret = ret ? ret : ret2; > cdd1fedf8261cd Dan Fuhry 2016-03-17 9353 out_notrans: > 943eb3bf25f4a7 Josef Bacik 2019-11-19 9354 if (new_ino == BTRFS_FIRST_FREE_OBJECTID || > 943eb3bf25f4a7 Josef Bacik 2019-11-19 9355 old_ino == BTRFS_FIRST_FREE_OBJECTID) > 0b246afa62b0cf Jeff Mahoney 2016-06-22 9356 up_read(&fs_info->subvol_sem); > cdd1fedf8261cd Dan Fuhry 2016-03-17 9357 > cdd1fedf8261cd Dan Fuhry 2016-03-17 9358 return ret; > cdd1fedf8261cd Dan Fuhry 2016-03-17 9359 } > > --- > 0-DAY CI Kernel Test Service, Intel Corporation > https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org >