Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Ye Bin <yebin10@huawei.com>
To:     <tytso@mit.edu>, <adilger.kernel@dilger.ca>,
        <linux-ext4@vger.kernel.org>
CC:     <jaegeuk@kernel.org>, <chao@kernel.org>,
        <linux-f2fs-devel@lists.sourceforge.net>,
        <linux-kernel@vger.kernel.org>, <jack@suse.cz>,
        <lczerner@redhat.com>, Ye Bin <yebin10@huawei.com>
Subject: [PATCH -next v2] ext4:fix file system corrupted when rmdir non empty directory with IO error
Date:   Fri, 11 Feb 2022 17:35:27 +0800
Message-ID: <20220211093527.3335518-1-yebin10@huawei.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7BIT
Content-Type:   text/plain; charset=US-ASCII
Precedence: bulk

We inject IO error when rmdir non empty direcory, then got issue as follows:
step1: mkfs.ext4 -F /dev/sda
step2: mount /dev/sda  test
step3: cd test
step4: mkdir -p 1/2
step5: rmdir 1
	[  110.920551] ext4_empty_dir: inject fault
	[  110.921926] EXT4-fs warning (device sda): ext4_rmdir:3113: inode #12:
	comm rmdir: empty directory '1' has too many links (3)
step6: cd ..
step7: umount test
step8: fsck.ext4 -f /dev/sda
	e2fsck 1.42.9 (28-Dec-2013)
	Pass 1: Checking inodes, blocks, and sizes
	Pass 2: Checking directory structure
	Entry '..' in .../??? (13) has deleted/unused inode 12.  Clear<y>? yes
	Pass 3: Checking directory connectivity
	Unconnected directory inode 13 (...)
	Connect to /lost+found<y>? yes
	Pass 4: Checking reference counts
	Inode 13 ref count is 3, should be 2.  Fix<y>? yes
	Pass 5: Checking group summary information

	/dev/sda: ***** FILE SYSTEM WAS MODIFIED *****
	/dev/sda: 12/131072 files (0.0% non-contiguous), 26157/524288 blocks

ext4_rmdir
	if (!ext4_empty_dir(inode))
		goto end_rmdir;
ext4_empty_dir
	bh = ext4_read_dirblock(inode, 0, DIRENT_HTREE);
	if (IS_ERR(bh))
		return true;
Now if read directory block failed, 'ext4_empty_dir' will return true, assume
directory is empty. Obviously, it will lead to above issue.
To solve this issue, if read directory block failed 'ext4_empty_dir' just assume
directory isn't empty. To avoid making things worse when file system is already
corrupted, 'ext4_empty_dir' also assume directory isn't empty.
To distinguish the error type, return the exact error code to the caller.

Signed-off-by: Ye Bin <yebin10@huawei.com>
---
 fs/crypto/policy.c      |  4 +---
 fs/ext4/ext4.h          |  4 ++--
 fs/ext4/inline.c        | 23 +++++++++++------------
 fs/ext4/ioctl.c         |  5 ++---
 fs/ext4/namei.c         | 27 +++++++++++++++------------
 fs/f2fs/dir.c           |  8 ++++----
 fs/f2fs/f2fs.h          |  2 +-
 fs/f2fs/file.c          |  7 +++++--
 fs/f2fs/namei.c         | 10 ++++------
 fs/ubifs/crypto.c       |  4 ++--
 include/linux/fscrypt.h |  2 +-
 11 files changed, 48 insertions(+), 48 deletions(-)

diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index ed3d623724cd..373945022bb6 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -480,9 +480,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
 			ret = -ENOTDIR;
 		else if (IS_DEADDIR(inode))
 			ret = -ENOENT;
-		else if (!inode->i_sb->s_cop->empty_dir(inode))
-			ret = -ENOTEMPTY;
-		else
+		else if (!(ret = inode->i_sb->s_cop->empty_dir(inode)))
 			ret = set_encryption_policy(inode, &policy);
 	} else if (ret == -EINVAL ||
 		   (ret == 0 && !fscrypt_policies_equal(&policy,
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index bcd3b9bf8069..e799cc269f7f 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -3087,7 +3087,7 @@ extern int ext4_generic_delete_entry(struct inode *dir,
 				     void *entry_buf,
 				     int buf_size,
 				     int csum_size);
-extern bool ext4_empty_dir(struct inode *inode);
+extern int ext4_empty_dir(struct inode *inode);
 
 /* resize.c */
 extern void ext4_kvfree_array_rcu(void *to_free);
@@ -3623,7 +3623,7 @@ extern int ext4_delete_inline_entry(handle_t *handle,
 				    struct ext4_dir_entry_2 *de_del,
 				    struct buffer_head *bh,
 				    int *has_inline_data);
-extern bool empty_inline_dir(struct inode *dir, int *has_inline_data);
+extern int empty_inline_dir(struct inode *dir, int *has_inline_data);
 extern struct buffer_head *ext4_get_first_inline_block(struct inode *inode,
 					struct ext4_dir_entry_2 **parent_de,
 					int *retval);
diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
index e42941803605..c9b02127ff95 100644
--- a/fs/ext4/inline.c
+++ b/fs/ext4/inline.c
@@ -1775,22 +1775,22 @@ ext4_get_inline_entry(struct inode *inode,
 	return (struct ext4_dir_entry_2 *)(inline_pos + offset);
 }
 
-bool empty_inline_dir(struct inode *dir, int *has_inline_data)
+int empty_inline_dir(struct inode *dir, int *has_inline_data)
 {
-	int err, inline_size;
+	int inline_size;
 	struct ext4_iloc iloc;
 	size_t inline_len;
 	void *inline_pos;
 	unsigned int offset;
 	struct ext4_dir_entry_2 *de;
-	bool ret = true;
+	int ret = 0;
 
-	err = ext4_get_inode_loc(dir, &iloc);
-	if (err) {
-		EXT4_ERROR_INODE_ERR(dir, -err,
+	ret = ext4_get_inode_loc(dir, &iloc);
+	if (ret) {
+		EXT4_ERROR_INODE_ERR(dir, -ret,
 				     "error %d getting inode %lu block",
-				     err, dir->i_ino);
-		return true;
+				     ret, dir->i_ino);
+		return ret;
 	}
 
 	down_read(&EXT4_I(dir)->xattr_sem);
@@ -1804,7 +1804,7 @@ bool empty_inline_dir(struct inode *dir, int *has_inline_data)
 		ext4_warning(dir->i_sb,
 			     "bad inline directory (dir #%lu) - no `..'",
 			     dir->i_ino);
-		ret = true;
+		ret = -EFSCORRUPTED;
 		goto out;
 	}
 
@@ -1823,16 +1823,15 @@ bool empty_inline_dir(struct inode *dir, int *has_inline_data)
 				     dir->i_ino, le32_to_cpu(de->inode),
 				     le16_to_cpu(de->rec_len), de->name_len,
 				     inline_size);
-			ret = true;
+			ret = -EFSCORRUPTED;
 			goto out;
 		}
 		if (le32_to_cpu(de->inode)) {
-			ret = false;
+			ret = -ENOTEMPTY;
 			goto out;
 		}
 		offset += ext4_rec_len_from_disk(de->rec_len, inline_size);
 	}
-
 out:
 	up_read(&EXT4_I(dir)->xattr_sem);
 	brelse(iloc.bh);
diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
index a8022c2c6a58..3845fd554249 100644
--- a/fs/ext4/ioctl.c
+++ b/fs/ext4/ioctl.c
@@ -620,10 +620,9 @@ static int ext4_ioctl_setflags(struct inode *inode,
 			goto flags_out;
 		}
 
-		if (!ext4_empty_dir(inode)) {
-			err = -ENOTEMPTY;
+		err = ext4_empty_dir(inode);
+		if (err)
 			goto flags_out;
-		}
 	}
 
 	/*
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index 8cf0a924a49b..2862deb374f7 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2976,8 +2976,11 @@ static int ext4_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
 
 /*
  * routine to check that the specified directory is empty (for rmdir)
+ * return value:
+ * 0: directory is empty
+ * <0: error code
  */
-bool ext4_empty_dir(struct inode *inode)
+int ext4_empty_dir(struct inode *inode)
 {
 	unsigned int offset;
 	struct buffer_head *bh;
@@ -2997,14 +3000,14 @@ bool ext4_empty_dir(struct inode *inode)
 	if (inode->i_size < ext4_dir_rec_len(1, NULL) +
 					ext4_dir_rec_len(2, NULL)) {
 		EXT4_ERROR_INODE(inode, "invalid size");
-		return true;
+		return -EFSCORRUPTED;
 	}
 	/* The first directory block must not be a hole,
 	 * so treat it as DIRENT_HTREE
 	 */
 	bh = ext4_read_dirblock(inode, 0, DIRENT_HTREE);
 	if (IS_ERR(bh))
-		return true;
+		return PTR_ERR(bh);
 
 	de = (struct ext4_dir_entry_2 *) bh->b_data;
 	if (ext4_check_dir_entry(inode, NULL, de, bh, bh->b_data, bh->b_size,
@@ -3012,7 +3015,7 @@ bool ext4_empty_dir(struct inode *inode)
 	    le32_to_cpu(de->inode) != inode->i_ino || strcmp(".", de->name)) {
 		ext4_warning_inode(inode, "directory missing '.'");
 		brelse(bh);
-		return true;
+		return -EFSCORRUPTED;
 	}
 	offset = ext4_rec_len_from_disk(de->rec_len, sb->s_blocksize);
 	de = ext4_next_entry(de, sb->s_blocksize);
@@ -3021,7 +3024,7 @@ bool ext4_empty_dir(struct inode *inode)
 	    le32_to_cpu(de->inode) == 0 || strcmp("..", de->name)) {
 		ext4_warning_inode(inode, "directory missing '..'");
 		brelse(bh);
-		return true;
+		return -EFSCORRUPTED;
 	}
 	offset += ext4_rec_len_from_disk(de->rec_len, sb->s_blocksize);
 	while (offset < inode->i_size) {
@@ -3035,7 +3038,7 @@ bool ext4_empty_dir(struct inode *inode)
 				continue;
 			}
 			if (IS_ERR(bh))
-				return true;
+				return PTR_ERR(bh);
 		}
 		de = (struct ext4_dir_entry_2 *) (bh->b_data +
 					(offset & (sb->s_blocksize - 1)));
@@ -3046,12 +3049,12 @@ bool ext4_empty_dir(struct inode *inode)
 		}
 		if (le32_to_cpu(de->inode)) {
 			brelse(bh);
-			return false;
+			return -ENOTEMPTY;
 		}
 		offset += ext4_rec_len_from_disk(de->rec_len, sb->s_blocksize);
 	}
 	brelse(bh);
-	return true;
+	return 0;
 }
 
 static int ext4_rmdir(struct inode *dir, struct dentry *dentry)
@@ -3087,8 +3090,8 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry)
 	if (le32_to_cpu(de->inode) != inode->i_ino)
 		goto end_rmdir;
 
-	retval = -ENOTEMPTY;
-	if (!ext4_empty_dir(inode))
+	retval = ext4_empty_dir(inode);
+	if (retval)
 		goto end_rmdir;
 
 	handle = ext4_journal_start(dir, EXT4_HT_DIR,
@@ -3787,8 +3790,8 @@ static int ext4_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
 
 	if (S_ISDIR(old.inode->i_mode)) {
 		if (new.inode) {
-			retval = -ENOTEMPTY;
-			if (!ext4_empty_dir(new.inode))
+			retval = ext4_empty_dir(new.inode);
+			if (retval)
 				goto end_rename;
 		} else {
 			retval = -EMLINK;
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index a0e51937d92e..3de5a1343070 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -953,7 +953,7 @@ void f2fs_delete_entry(struct f2fs_dir_entry *dentry, struct page *page,
 		f2fs_drop_nlink(dir, inode);
 }
 
-bool f2fs_empty_dir(struct inode *dir)
+int f2fs_empty_dir(struct inode *dir)
 {
 	unsigned long bidx;
 	struct page *dentry_page;
@@ -970,7 +970,7 @@ bool f2fs_empty_dir(struct inode *dir)
 			if (PTR_ERR(dentry_page) == -ENOENT)
 				continue;
 			else
-				return false;
+				return PTR_ERR(dentry_page);
 		}
 
 		dentry_blk = page_address(dentry_page);
@@ -985,9 +985,9 @@ bool f2fs_empty_dir(struct inode *dir)
 		f2fs_put_page(dentry_page, 1);
 
 		if (bit_pos < NR_DENTRY_IN_BLOCK)
-			return false;
+			return -ENOTEMPTY;
 	}
-	return true;
+	return 0;
 }
 
 int f2fs_fill_dentries(struct dir_context *ctx, struct f2fs_dentry_ptr *d,
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 5c30a65467e2..09617d7b37fd 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3465,7 +3465,7 @@ int f2fs_do_add_link(struct inode *dir, const struct qstr *name,
 void f2fs_delete_entry(struct f2fs_dir_entry *dentry, struct page *page,
 			struct inode *dir, struct inode *inode);
 int f2fs_do_tmpfile(struct inode *inode, struct inode *dir);
-bool f2fs_empty_dir(struct inode *dir);
+int f2fs_empty_dir(struct inode *dir);
 
 static inline int f2fs_add_link(struct dentry *dentry, struct inode *inode)
 {
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index cfdc41f87f5d..a3b60d6a58f7 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1846,10 +1846,13 @@ static int f2fs_setflags_common(struct inode *inode, u32 iflags, u32 mask)
 		return -EPERM;
 
 	if ((iflags ^ masked_flags) & F2FS_CASEFOLD_FL) {
+		int ret;
+
 		if (!f2fs_sb_has_casefold(F2FS_I_SB(inode)))
 			return -EOPNOTSUPP;
-		if (!f2fs_empty_dir(inode))
-			return -ENOTEMPTY;
+		ret = f2fs_empty_dir(inode);
+		if (ret)
+			return ret;
 	}
 
 	if (iflags & (F2FS_COMPR_FL | F2FS_NOCOMP_FL)) {
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index 13a0ffc39fa4..e4d1821b707b 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -786,10 +786,10 @@ static int f2fs_mkdir(struct user_namespace *mnt_userns, struct inode *dir,
 static int f2fs_rmdir(struct inode *dir, struct dentry *dentry)
 {
 	struct inode *inode = d_inode(dentry);
+	int ret;
 
-	if (f2fs_empty_dir(inode))
-		return f2fs_unlink(dir, dentry);
-	return -ENOTEMPTY;
+	ret = f2fs_empty_dir(inode);
+	return ret ? : f2fs_unlink(dir, dentry);
 }
 
 static int f2fs_mknod(struct user_namespace *mnt_userns, struct inode *dir,
@@ -1001,9 +1001,7 @@ static int f2fs_rename(struct user_namespace *mnt_userns, struct inode *old_dir,
 	}
 
 	if (new_inode) {
-
-		err = -ENOTEMPTY;
-		if (old_dir_entry && !f2fs_empty_dir(new_inode))
+		if (old_dir_entry && (err = f2fs_empty_dir(new_inode)))
 			goto out_dir;
 
 		err = -ENOENT;
diff --git a/fs/ubifs/crypto.c b/fs/ubifs/crypto.c
index c57b46a352d8..3ef2017c1444 100644
--- a/fs/ubifs/crypto.c
+++ b/fs/ubifs/crypto.c
@@ -19,9 +19,9 @@ static int ubifs_crypt_set_context(struct inode *inode, const void *ctx,
 			       ctx, len, 0, false);
 }
 
-static bool ubifs_crypt_empty_dir(struct inode *inode)
+static int ubifs_crypt_empty_dir(struct inode *inode)
 {
-	return ubifs_check_dir_empty(inode) == 0;
+	return ubifs_check_dir_empty(inode);
 }
 
 int ubifs_encrypt(const struct inode *inode, struct ubifs_data_node *dn,
diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h
index 91ea9477e9bd..9d3b8df3f5ea 100644
--- a/include/linux/fscrypt.h
+++ b/include/linux/fscrypt.h
@@ -116,7 +116,7 @@ struct fscrypt_operations {
 	/*
 	 * Check whether a directory is empty.  i_rwsem will be held for write.
 	 */
-	bool (*empty_dir)(struct inode *inode);
+	int (*empty_dir)(struct inode *inode);
 
 	/*
 	 * Check whether the filesystem's inode numbers and UUID are stable,
-- 
2.31.1