| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 25 Feb 2022 16:22:45 +0800
From: yebin <yebin10@...wei.com>
To: <tytso@....edu>, <adilger.kernel@...ger.ca>,
<linux-ext4@...r.kernel.org>
CC: <jaegeuk@...nel.org>, <chao@...nel.org>,
<linux-f2fs-devel@...ts.sourceforge.net>,
<linux-kernel@...r.kernel.org>, <jack@...e.cz>,
<lczerner@...hat.com>
Subject: Re: [PATCH -next v2] ext4:fix file system corrupted when rmdir non
empty directory with IO error
ping...
On 2022/2/11 17:35, Ye Bin wrote:
> 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@...wei.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,
Powered by blists - more mailing lists