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
