[bug report] ext4: Fix possible corruption when moving a directory

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hello Jan Kara,

The patch 0813299c586b: "ext4: Fix possible corruption when moving a
directory" from Jan 26, 2023, leads to the following Smatch static
checker warning:

	fs/ext4/namei.c:4017 ext4_rename()
	error: double unlocked '&old.inode->i_rwsem' (orig line 3882)

fs/ext4/namei.c
    3766 static int ext4_rename(struct mnt_idmap *idmap, struct inode *old_dir,
    3767                        struct dentry *old_dentry, struct inode *new_dir,
    3768                        struct dentry *new_dentry, unsigned int flags)
    3769 {
    3770         handle_t *handle = NULL;
    3771         struct ext4_renament old = {
    3772                 .dir = old_dir,
    3773                 .dentry = old_dentry,
    3774                 .inode = d_inode(old_dentry),
    3775         };
    3776         struct ext4_renament new = {
    3777                 .dir = new_dir,
    3778                 .dentry = new_dentry,
    3779                 .inode = d_inode(new_dentry),
    3780         };
    3781         int force_reread;
    3782         int retval;
    3783         struct inode *whiteout = NULL;
    3784         int credits;
    3785         u8 old_file_type;
    3786 
    3787         if (new.inode && new.inode->i_nlink == 0) {
    3788                 EXT4_ERROR_INODE(new.inode,
    3789                                  "target of rename is already freed");
    3790                 return -EFSCORRUPTED;
    3791         }
    3792 
    3793         if ((ext4_test_inode_flag(new_dir, EXT4_INODE_PROJINHERIT)) &&
    3794             (!projid_eq(EXT4_I(new_dir)->i_projid,
    3795                         EXT4_I(old_dentry->d_inode)->i_projid)))
    3796                 return -EXDEV;
    3797 
    3798         retval = dquot_initialize(old.dir);
    3799         if (retval)
    3800                 return retval;
    3801         retval = dquot_initialize(old.inode);
    3802         if (retval)
    3803                 return retval;
    3804         retval = dquot_initialize(new.dir);
    3805         if (retval)
    3806                 return retval;
    3807 
    3808         /* Initialize quotas before so that eventual writes go
    3809          * in separate transaction */
    3810         if (new.inode) {
    3811                 retval = dquot_initialize(new.inode);
    3812                 if (retval)
    3813                         return retval;
    3814         }
    3815 
    3816         old.bh = ext4_find_entry(old.dir, &old.dentry->d_name, &old.de, NULL);
    3817         if (IS_ERR(old.bh))
    3818                 return PTR_ERR(old.bh);
    3819         /*
    3820          *  Check for inode number is _not_ due to possible IO errors.
    3821          *  We might rmdir the source, keep it as pwd of some process
    3822          *  and merrily kill the link to whatever was created under the
    3823          *  same name. Goodbye sticky bit ;-<
    3824          */
    3825         retval = -ENOENT;
    3826         if (!old.bh || le32_to_cpu(old.de->inode) != old.inode->i_ino)
    3827                 goto release_bh;
    3828 
    3829         new.bh = ext4_find_entry(new.dir, &new.dentry->d_name,
    3830                                  &new.de, &new.inlined);
    3831         if (IS_ERR(new.bh)) {
    3832                 retval = PTR_ERR(new.bh);
    3833                 new.bh = NULL;
    3834                 goto release_bh;
    3835         }
    3836         if (new.bh) {
    3837                 if (!new.inode) {
    3838                         brelse(new.bh);
    3839                         new.bh = NULL;
    3840                 }
    3841         }
    3842         if (new.inode && !test_opt(new.dir->i_sb, NO_AUTO_DA_ALLOC))
    3843                 ext4_alloc_da_blocks(old.inode);
    3844 
    3845         credits = (2 * EXT4_DATA_TRANS_BLOCKS(old.dir->i_sb) +
    3846                    EXT4_INDEX_EXTRA_TRANS_BLOCKS + 2);
    3847         if (!(flags & RENAME_WHITEOUT)) {
    3848                 handle = ext4_journal_start(old.dir, EXT4_HT_DIR, credits);
    3849                 if (IS_ERR(handle)) {
    3850                         retval = PTR_ERR(handle);
    3851                         goto release_bh;
    3852                 }
    3853         } else {
    3854                 whiteout = ext4_whiteout_for_rename(idmap, &old, credits, &handle);
    3855                 if (IS_ERR(whiteout)) {
    3856                         retval = PTR_ERR(whiteout);
    3857                         goto release_bh;
    3858                 }
    3859         }
    3860 
    3861         old_file_type = old.de->file_type;
    3862         if (IS_DIRSYNC(old.dir) || IS_DIRSYNC(new.dir))
    3863                 ext4_handle_sync(handle);
    3864 
    3865         if (S_ISDIR(old.inode->i_mode)) {
    3866                 if (new.inode) {
    3867                         retval = -ENOTEMPTY;
    3868                         if (!ext4_empty_dir(new.inode))
    3869                                 goto end_rename;
    3870                 } else {
    3871                         retval = -EMLINK;
    3872                         if (new.dir != old.dir && EXT4_DIR_LINK_MAX(new.dir))
    3873                                 goto end_rename;
    3874                 }
    3875                 /*
    3876                  * We need to protect against old.inode directory getting
    3877                  * converted from inline directory format into a normal one.
    3878                  */
    3879                 inode_lock_nested(old.inode, I_MUTEX_NONDIR2);
    3880                 retval = ext4_rename_dir_prepare(handle, &old);
    3881                 if (retval) {
    3882                         inode_unlock(old.inode);

The issue here is that ext4_rename_dir_prepare() sets old.dir_bh and
then returns -EFSCORRUPTED.  It results in an unlock here and then again
after the goto.

    3883                         goto end_rename;
    3884                 }
    3885         }
    3886         /*
    3887          * If we're renaming a file within an inline_data dir and adding or
    3888          * setting the new dirent causes a conversion from inline_data to
    3889          * extents/blockmap, we need to force the dirent delete code to
    3890          * re-read the directory, or else we end up trying to delete a dirent
    3891          * from what is now the extent tree root (or a block map).
    3892          */
    3893         force_reread = (new.dir->i_ino == old.dir->i_ino &&
    3894                         ext4_test_inode_flag(new.dir, EXT4_INODE_INLINE_DATA));
    3895 
    3896         if (whiteout) {
    3897                 /*
    3898                  * Do this before adding a new entry, so the old entry is sure
    3899                  * to be still pointing to the valid old entry.
    3900                  */
    3901                 retval = ext4_setent(handle, &old, whiteout->i_ino,
    3902                                      EXT4_FT_CHRDEV);
    3903                 if (retval)
    3904                         goto end_rename;
    3905                 retval = ext4_mark_inode_dirty(handle, whiteout);
    3906                 if (unlikely(retval))
    3907                         goto end_rename;
    3908 
    3909         }
    3910         if (!new.bh) {
    3911                 retval = ext4_add_entry(handle, new.dentry, old.inode);
    3912                 if (retval)
    3913                         goto end_rename;
    3914         } else {
    3915                 retval = ext4_setent(handle, &new,
    3916                                      old.inode->i_ino, old_file_type);
    3917                 if (retval)
    3918                         goto end_rename;
    3919         }
    3920         if (force_reread)
    3921                 force_reread = !ext4_test_inode_flag(new.dir,
    3922                                                      EXT4_INODE_INLINE_DATA);
    3923 
    3924         /*
    3925          * Like most other Unix systems, set the ctime for inodes on a
    3926          * rename.
    3927          */
    3928         old.inode->i_ctime = current_time(old.inode);
    3929         retval = ext4_mark_inode_dirty(handle, old.inode);
    3930         if (unlikely(retval))
    3931                 goto end_rename;
    3932 
    3933         if (!whiteout) {
    3934                 /*
    3935                  * ok, that's it
    3936                  */
    3937                 ext4_rename_delete(handle, &old, force_reread);
    3938         }
    3939 
    3940         if (new.inode) {
    3941                 ext4_dec_count(new.inode);
    3942                 new.inode->i_ctime = current_time(new.inode);
    3943         }
    3944         old.dir->i_ctime = old.dir->i_mtime = current_time(old.dir);
    3945         ext4_update_dx_flag(old.dir);
    3946         if (old.dir_bh) {
    3947                 retval = ext4_rename_dir_finish(handle, &old, new.dir->i_ino);
    3948                 if (retval)
    3949                         goto end_rename;
    3950 
    3951                 ext4_dec_count(old.dir);
    3952                 if (new.inode) {
    3953                         /* checked ext4_empty_dir above, can't have another
    3954                          * parent, ext4_dec_count() won't work for many-linked
    3955                          * dirs */
    3956                         clear_nlink(new.inode);
    3957                 } else {
    3958                         ext4_inc_count(new.dir);
    3959                         ext4_update_dx_flag(new.dir);
    3960                         retval = ext4_mark_inode_dirty(handle, new.dir);
    3961                         if (unlikely(retval))
    3962                                 goto end_rename;
    3963                 }
    3964         }
    3965         retval = ext4_mark_inode_dirty(handle, old.dir);
    3966         if (unlikely(retval))
    3967                 goto end_rename;
    3968 
    3969         if (S_ISDIR(old.inode->i_mode)) {
    3970                 /*
    3971                  * We disable fast commits here that's because the
    3972                  * replay code is not yet capable of changing dot dot
    3973                  * dirents in directories.
    3974                  */
    3975                 ext4_fc_mark_ineligible(old.inode->i_sb,
    3976                         EXT4_FC_REASON_RENAME_DIR, handle);
    3977         } else {
    3978                 struct super_block *sb = old.inode->i_sb;
    3979 
    3980                 if (new.inode)
    3981                         ext4_fc_track_unlink(handle, new.dentry);
    3982                 if (test_opt2(sb, JOURNAL_FAST_COMMIT) &&
    3983                     !(EXT4_SB(sb)->s_mount_state & EXT4_FC_REPLAY) &&
    3984                     !(ext4_test_mount_flag(sb, EXT4_MF_FC_INELIGIBLE))) {
    3985                         __ext4_fc_track_link(handle, old.inode, new.dentry);
    3986                         __ext4_fc_track_unlink(handle, old.inode, old.dentry);
    3987                         if (whiteout)
    3988                                 __ext4_fc_track_create(handle, whiteout,
    3989                                                        old.dentry);
    3990                 }
    3991         }
    3992 
    3993         if (new.inode) {
    3994                 retval = ext4_mark_inode_dirty(handle, new.inode);
    3995                 if (unlikely(retval))
    3996                         goto end_rename;
    3997                 if (!new.inode->i_nlink)
    3998                         ext4_orphan_add(handle, new.inode);
    3999         }
    4000         retval = 0;
    4001 
    4002 end_rename:
    4003         if (whiteout) {
    4004                 if (retval) {
    4005                         ext4_resetent(handle, &old,
    4006                                       old.inode->i_ino, old_file_type);
    4007                         drop_nlink(whiteout);
    4008                         ext4_orphan_add(handle, whiteout);
    4009                 }
    4010                 unlock_new_inode(whiteout);
    4011                 ext4_journal_stop(handle);
    4012                 iput(whiteout);
    4013         } else {
    4014                 ext4_journal_stop(handle);
    4015         }
    4016         if (old.dir_bh)
--> 4017                 inode_unlock(old.inode);
    4018 release_bh:
    4019         brelse(old.dir_bh);
    4020         brelse(old.bh);
    4021         brelse(new.bh);
    4022         return retval;
    4023 }

regards,
dan carpenter



[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux