ext4:fix file system corrupted when rmdir non empty directory with IO error

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 <[email protected]>

---
  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(-)

fs/crypto/policy.c +1 -3

index ed3d623724cd..373945022bb6 100644
@@ -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,

fs/ext4/ext4.h +2 -2

index bcd3b9bf8069..e799cc269f7f 100644
@@ -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);

fs/ext4/inline.c +11 -12

index e42941803605..c9b02127ff95 100644
@@ -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);

fs/ext4/ioctl.c +2 -3

index a8022c2c6a58..3845fd554249 100644
@@ -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;
-		}
  	}
 
  	/*

fs/ext4/namei.c +15 -12

index 8cf0a924a49b..2862deb374f7 100644
@@ -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;

fs/f2fs/dir.c +4 -4

index a0e51937d92e..3de5a1343070 100644
@@ -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,

fs/f2fs/f2fs.h +1 -1

index 5c30a65467e2..09617d7b37fd 100644
@@ -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)
  {

fs/f2fs/file.c +5 -2

index cfdc41f87f5d..a3b60d6a58f7 100644
@@ -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)) {

fs/f2fs/namei.c +4 -6

index 13a0ffc39fa4..e4d1821b707b 100644
@@ -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;

fs/ubifs/crypto.c +2 -2

index c57b46a352d8..3ef2017c1444 100644
@@ -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,

include/linux/fscrypt.h +1 -1

index 91ea9477e9bd..9d3b8df3f5ea 100644
@@ -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,

Quoted 77 lines

fs/f2fs/dir.c +1 -1

index a0e51937d92e..3de5a1343070 100644
@@ -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);

fs/f2fs/dir.c +2 -2

@@ -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,

fs/f2fs/f2fs.h +1 -1

index 5c30a65467e2..09617d7b37fd 100644
@@ -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)
   {

fs/f2fs/file.c +5 -2

index cfdc41f87f5d..a3b60d6a58f7 100644
@@ -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)) {

fs/f2fs/namei.c +3 -3

index 13a0ffc39fa4..e4d1821b707b 100644
@@ -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,

fs/f2fs/namei.c static int f2fs_rename(struct user_namespace *mnt_userns, struct inode *old_dir, · line 1007

Quoted 8 lines

fs/f2fs/namei.c +1 -3

       }
       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;

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.

Quoted 7 lines · Ye Bin

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.

Does the same issue exist for f2fs and ubifs? We could solve the specific bug much more simply by having ext4_empty_dir() return FALSE if we aren't able to read the directory block. Yes, it means that we don't return as specific an error code in the case of an I/O error --- although I believe we do syslog a warning --- but it makes for a much simpler patch that doesn't requiring getting acked-by's from the fscrypt, f2fs and ubifs folks.

- Ted
.