The recent code cleanup of __ext4_fill_super() resulted in a bug
reported by Syzkaller. When I investigated this issue, I found that
the bug was caused by a fragile and partially redundant way error
codes were set to be returned by the __ext4_fill_super() function.
The first patch fixes the bug found by Syzkaller, and the second and
third patch cleans up the error handling in __ext4_fill_super() and
__ext4_multi_mount_protect().
Andreas, please take a look at the second patch, as it changes the
error codes returned in various cases when the MMP feature prevents
the file system from being mounted and other failure cases. For
example, when another system has the file system mounted, mount will
now return EBUSY instead EINVAL. In other cases, if a memory
allocation fails, mount will now return ENOMEM instead of EINVAL. I
think this is an improvement, but there might be some userspace code
that might get confused by this. Since Lustre users tend to be most
common users of the MMP feature, I'd appreciate your review of this
patch.
Theodore Ts'o (3):
ext4: fix lost error code reporting in __ext4_fill_super()
ext4: reflect error codes from ext4_multi_mount_protect() to its
callers
ext4: clean up error handling in __ext4_fill_super()
fs/ext4/mmp.c | 9 ++++++-
fs/ext4/super.c | 68 +++++++++++++++++++++++++++++--------------------
2 files changed, 48 insertions(+), 29 deletions(-)
--
2.31.0
When code was factored out of __ext4_fill_super() into
ext4_percpu_param_init() the error return was discard. This meant
that it was possible for __ext4_fill_super() to return zero,
indicating success, without the struct super getting completely filled
in, leading to a potential NULL pointer dereference.
Reported-by: [email protected]
Fixes: 1f79467c8a6b ("ext4: factor out ext4_percpu_param_init() ...")
Cc: Jason Yan <[email protected]>
Link: https://syzkaller.appspot.com/bug?id=6dac47d5e58af770c0055f680369586ec32e144c
Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/ext4/super.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 403cc0e6cd65..b11907e1fab2 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5503,7 +5503,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
sbi->s_journal->j_commit_callback =
ext4_journal_commit_callback;
- if (ext4_percpu_param_init(sbi))
+ err = ext4_percpu_param_init(sbi);
+ if (err)
goto failed_mount6;
if (ext4_has_feature_flex_bg(sb))
--
2.31.0
This will allow more fine-grained errno codes to be returned by the
mount system call.
Cc: Andreas Dilger <[email protected]>
Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/ext4/mmp.c | 9 ++++++++-
fs/ext4/super.c | 14 +++++++++-----
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 4681fff6665f..4022bc713421 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -282,6 +282,7 @@ int ext4_multi_mount_protect(struct super_block *sb,
if (mmp_block < le32_to_cpu(es->s_first_data_block) ||
mmp_block >= ext4_blocks_count(es)) {
ext4_warning(sb, "Invalid MMP block in superblock");
+ retval = -EINVAL;
goto failed;
}
@@ -307,6 +308,7 @@ int ext4_multi_mount_protect(struct super_block *sb,
if (seq == EXT4_MMP_SEQ_FSCK) {
dump_mmp_msg(sb, mmp, "fsck is running on the filesystem");
+ retval = -EBUSY;
goto failed;
}
@@ -320,6 +322,7 @@ int ext4_multi_mount_protect(struct super_block *sb,
if (schedule_timeout_interruptible(HZ * wait_time) != 0) {
ext4_warning(sb, "MMP startup interrupted, failing mount\n");
+ retval = -ETIMEDOUT;
goto failed;
}
@@ -330,6 +333,7 @@ int ext4_multi_mount_protect(struct super_block *sb,
if (seq != le32_to_cpu(mmp->mmp_seq)) {
dump_mmp_msg(sb, mmp,
"Device is already active on another node.");
+ retval = -EBUSY;
goto failed;
}
@@ -349,6 +353,7 @@ int ext4_multi_mount_protect(struct super_block *sb,
*/
if (schedule_timeout_interruptible(HZ * wait_time) != 0) {
ext4_warning(sb, "MMP startup interrupted, failing mount");
+ retval = -ETIMEDOUT;
goto failed;
}
@@ -359,6 +364,7 @@ int ext4_multi_mount_protect(struct super_block *sb,
if (seq != le32_to_cpu(mmp->mmp_seq)) {
dump_mmp_msg(sb, mmp,
"Device is already active on another node.");
+ retval = -EBUSY;
goto failed;
}
@@ -378,6 +384,7 @@ int ext4_multi_mount_protect(struct super_block *sb,
EXT4_SB(sb)->s_mmp_tsk = NULL;
ext4_warning(sb, "Unable to create kmmpd thread for %s.",
sb->s_id);
+ retval = -ENOMEM;
goto failed;
}
@@ -385,5 +392,5 @@ int ext4_multi_mount_protect(struct super_block *sb,
failed:
brelse(bh);
- return 1;
+ return retval;
}
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index b11907e1fab2..9a8af70815b1 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5329,9 +5329,11 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
ext4_has_feature_orphan_present(sb) ||
ext4_has_feature_journal_needs_recovery(sb));
- if (ext4_has_feature_mmp(sb) && !sb_rdonly(sb))
- if (ext4_multi_mount_protect(sb, le64_to_cpu(es->s_mmp_block)))
+ if (ext4_has_feature_mmp(sb) && !sb_rdonly(sb)) {
+ err = ext4_multi_mount_protect(sb, le64_to_cpu(es->s_mmp_block));
+ if (err)
goto failed_mount3a;
+ }
/*
* The first inode we look at is the journal inode. Don't try
@@ -6566,12 +6568,14 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
goto restore_opts;
sb->s_flags &= ~SB_RDONLY;
- if (ext4_has_feature_mmp(sb))
- if (ext4_multi_mount_protect(sb,
- le64_to_cpu(es->s_mmp_block))) {
+ if (ext4_has_feature_mmp(sb)) {
+ err = ext4_multi_mount_protect(sb,
+ le64_to_cpu(es->s_mmp_block));
+ if (err) {
err = -EROFS;
goto restore_opts;
}
+ }
#ifdef CONFIG_QUOTA
enable_quota = 1;
#endif
--
2.31.0
There were two ways to return an error code; one was via setting the
'err' variable, and the second, if err was zero, was via the 'ret'
variable. This was both confusing and fragile, and when code was
factored out of __ext4_fill_super(), some of the error codes returned
by the original code was replaced by -EINVAL, and in one case, the
error code was placed by 0, triggering a kernel null pointer
dereference.
Clean this up by removing the 'ret' variable, leaving only one way to
setfthe error code to be returned, and restore the errno codes that
were returned via the the mount system call as they were before we
started refactoring __ext4_fill_super().
Signed-off-by: Theodore Ts'o <[email protected]>
---
fs/ext4/super.c | 51 ++++++++++++++++++++++++++++---------------------
1 file changed, 29 insertions(+), 22 deletions(-)
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 9a8af70815b1..662e49195b65 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -5196,10 +5196,9 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
struct ext4_sb_info *sbi = EXT4_SB(sb);
ext4_fsblk_t logical_sb_block;
struct inode *root;
- int ret = -ENOMEM;
unsigned int i;
int needs_recovery;
- int err = 0;
+ int err;
ext4_group_t first_not_zeroed;
struct ext4_fs_context *ctx = fc->fs_private;
int silent = fc->sb_flags & SB_SILENT;
@@ -5212,8 +5211,6 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
sbi->s_sectors_written_start =
part_stat_read(sb->s_bdev, sectors[STAT_WRITE]);
- /* -EINVAL is default */
- ret = -EINVAL;
err = ext4_load_super(sb, &logical_sb_block, silent);
if (err)
goto out_fail;
@@ -5239,7 +5236,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
*/
sbi->s_li_wait_mult = EXT4_DEF_LI_WAIT_MULT;
- if (ext4_inode_info_init(sb, es))
+ err = ext4_inode_info_init(sb, es);
+ if (err)
goto failed_mount;
err = parse_apply_sb_mount_options(sb, ctx);
@@ -5255,10 +5253,12 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
ext4_apply_options(fc, sb);
- if (ext4_encoding_init(sb, es))
+ err = ext4_encoding_init(sb, es);
+ if (err)
goto failed_mount;
- if (ext4_check_journal_data_mode(sb))
+ err = ext4_check_journal_data_mode(sb);
+ if (err)
goto failed_mount;
sb->s_flags = (sb->s_flags & ~SB_POSIXACL) |
@@ -5267,18 +5267,22 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
/* i_version is always enabled now */
sb->s_flags |= SB_I_VERSION;
- if (ext4_check_feature_compatibility(sb, es, silent))
+ err = ext4_check_feature_compatibility(sb, es, silent);
+ if (err)
goto failed_mount;
- if (ext4_block_group_meta_init(sb, silent))
+ err = ext4_block_group_meta_init(sb, silent);
+ if (err)
goto failed_mount;
ext4_hash_info_init(sb);
- if (ext4_handle_clustersize(sb))
+ err = ext4_handle_clustersize(sb);
+ if (err)
goto failed_mount;
- if (ext4_check_geometry(sb, es))
+ err = ext4_check_geometry(sb, es);
+ if (err)
goto failed_mount;
timer_setup(&sbi->s_err_report, print_daily_error_info, 0);
@@ -5289,8 +5293,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
if (err)
goto failed_mount3;
- /* Register extent status tree shrinker */
- if (ext4_es_register_shrinker(sbi))
+ err = ext4_es_register_shrinker(sbi);
+ if (err)
goto failed_mount3;
sbi->s_stripe = ext4_get_stripe_size(sbi);
@@ -5335,6 +5339,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
goto failed_mount3a;
}
+ err = -EINVAL;
/*
* The first inode we look at is the journal inode. Don't try
* root first: it may be modified in the journal!
@@ -5386,6 +5391,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
if (!sbi->s_ea_block_cache) {
ext4_msg(sb, KERN_ERR,
"Failed to create ea_block_cache");
+ err = -EINVAL;
goto failed_mount_wq;
}
@@ -5394,6 +5400,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
if (!sbi->s_ea_inode_cache) {
ext4_msg(sb, KERN_ERR,
"Failed to create ea_inode_cache");
+ err = -EINVAL;
goto failed_mount_wq;
}
}
@@ -5428,7 +5435,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
alloc_workqueue("ext4-rsv-conversion", WQ_MEM_RECLAIM | WQ_UNBOUND, 1);
if (!EXT4_SB(sb)->rsv_conversion_wq) {
printk(KERN_ERR "EXT4-fs: failed to create workqueue\n");
- ret = -ENOMEM;
+ err = -ENOMEM;
goto failed_mount4;
}
@@ -5440,28 +5447,28 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
root = ext4_iget(sb, EXT4_ROOT_INO, EXT4_IGET_SPECIAL);
if (IS_ERR(root)) {
ext4_msg(sb, KERN_ERR, "get root inode failed");
- ret = PTR_ERR(root);
+ err = PTR_ERR(root);
root = NULL;
goto failed_mount4;
}
if (!S_ISDIR(root->i_mode) || !root->i_blocks || !root->i_size) {
ext4_msg(sb, KERN_ERR, "corrupt root inode, run e2fsck");
iput(root);
+ err = -EFSCORRUPTED;
goto failed_mount4;
}
sb->s_root = d_make_root(root);
if (!sb->s_root) {
ext4_msg(sb, KERN_ERR, "get root dentry failed");
- ret = -ENOMEM;
+ err = -ENOMEM;
goto failed_mount4;
}
- ret = ext4_setup_super(sb, es, sb_rdonly(sb));
- if (ret == -EROFS) {
+ err = ext4_setup_super(sb, es, sb_rdonly(sb));
+ if (err == -EROFS) {
sb->s_flags |= SB_RDONLY;
- ret = 0;
- } else if (ret)
+ } else if (err)
goto failed_mount4a;
ext4_set_resv_clusters(sb);
@@ -5514,7 +5521,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
ext4_msg(sb, KERN_ERR,
"unable to initialize "
"flex_bg meta info!");
- ret = -ENOMEM;
+ err = -ENOMEM;
goto failed_mount6;
}
@@ -5640,7 +5647,7 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
ext4_blkdev_remove(sbi);
out_fail:
sb->s_fs_info = NULL;
- return err ? err : ret;
+ return err;
}
static int ext4_fill_super(struct super_block *sb, struct fs_context *fc)
--
2.31.0
On 2023/4/28 11:16, Theodore Ts'o wrote:
> This will allow more fine-grained errno codes to be returned by the
> mount system call.
>
> Cc: Andreas Dilger <[email protected]>
> Signed-off-by: Theodore Ts'o <[email protected]>
> ---
> fs/ext4/mmp.c | 9 ++++++++-
> fs/ext4/super.c | 14 +++++++++-----
> 2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index 4681fff6665f..4022bc713421 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -282,6 +282,7 @@ int ext4_multi_mount_protect(struct super_block *sb,
> if (mmp_block < le32_to_cpu(es->s_first_data_block) ||
> mmp_block >= ext4_blocks_count(es)) {
> ext4_warning(sb, "Invalid MMP block in superblock");
> + retval = -EINVAL;
> goto failed;
> }
>
> @@ -307,6 +308,7 @@ int ext4_multi_mount_protect(struct super_block *sb,
>
> if (seq == EXT4_MMP_SEQ_FSCK) {
> dump_mmp_msg(sb, mmp, "fsck is running on the filesystem");
> + retval = -EBUSY;
> goto failed;
> }
>
> @@ -320,6 +322,7 @@ int ext4_multi_mount_protect(struct super_block *sb,
>
> if (schedule_timeout_interruptible(HZ * wait_time) != 0) {
> ext4_warning(sb, "MMP startup interrupted, failing mount\n");
> + retval = -ETIMEDOUT;
> goto failed;
> }
>
> @@ -330,6 +333,7 @@ int ext4_multi_mount_protect(struct super_block *sb,
> if (seq != le32_to_cpu(mmp->mmp_seq)) {
> dump_mmp_msg(sb, mmp,
> "Device is already active on another node.");
> + retval = -EBUSY;
> goto failed;
> }
>
> @@ -349,6 +353,7 @@ int ext4_multi_mount_protect(struct super_block *sb,
> */
> if (schedule_timeout_interruptible(HZ * wait_time) != 0) {
> ext4_warning(sb, "MMP startup interrupted, failing mount");
> + retval = -ETIMEDOUT;
> goto failed;
> }
>
> @@ -359,6 +364,7 @@ int ext4_multi_mount_protect(struct super_block *sb,
> if (seq != le32_to_cpu(mmp->mmp_seq)) {
> dump_mmp_msg(sb, mmp,
> "Device is already active on another node.");
> + retval = -EBUSY;
> goto failed;
> }
>
> @@ -378,6 +384,7 @@ int ext4_multi_mount_protect(struct super_block *sb,
> EXT4_SB(sb)->s_mmp_tsk = NULL;
> ext4_warning(sb, "Unable to create kmmpd thread for %s.",
> sb->s_id);
> + retval = -ENOMEM;
> goto failed;
> }
>
> @@ -385,5 +392,5 @@ int ext4_multi_mount_protect(struct super_block *sb,
>
> failed:
> brelse(bh);
> - return 1;
> + return retval;
> }
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index b11907e1fab2..9a8af70815b1 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5329,9 +5329,11 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> ext4_has_feature_orphan_present(sb) ||
> ext4_has_feature_journal_needs_recovery(sb));
>
> - if (ext4_has_feature_mmp(sb) && !sb_rdonly(sb))
> - if (ext4_multi_mount_protect(sb, le64_to_cpu(es->s_mmp_block)))
> + if (ext4_has_feature_mmp(sb) && !sb_rdonly(sb)) {
> + err = ext4_multi_mount_protect(sb, le64_to_cpu(es->s_mmp_block));
> + if (err)
> goto failed_mount3a;
> + }
>
> /*
> * The first inode we look at is the journal inode. Don't try
> @@ -6566,12 +6568,14 @@ static int __ext4_remount(struct fs_context *fc, struct super_block *sb)
> goto restore_opts;
>
> sb->s_flags &= ~SB_RDONLY;
> - if (ext4_has_feature_mmp(sb))
> - if (ext4_multi_mount_protect(sb,
> - le64_to_cpu(es->s_mmp_block))) {
> + if (ext4_has_feature_mmp(sb)) {
> + err = ext4_multi_mount_protect(sb,
> + le64_to_cpu(es->s_mmp_block));
> + if (err) {
> err = -EROFS;
So shall we return the fine-grained errno from
ext4_multi_mount_protect() instead of -EROFS here?
Thanks,
Jason
On 2023/4/28 11:16, Theodore Ts'o wrote:
> When code was factored out of __ext4_fill_super() into
> ext4_percpu_param_init() the error return was discard. This meant
> that it was possible for __ext4_fill_super() to return zero,
> indicating success, without the struct super getting completely filled
> in, leading to a potential NULL pointer dereference.
>
> Reported-by: [email protected]
> Fixes: 1f79467c8a6b ("ext4: factor out ext4_percpu_param_init() ...")
> Cc: Jason Yan <[email protected]>
> Link: https://syzkaller.appspot.com/bug?id=6dac47d5e58af770c0055f680369586ec32e144c
> Signed-off-by: Theodore Ts'o <[email protected]>
> ---
> fs/ext4/super.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 403cc0e6cd65..b11907e1fab2 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5503,7 +5503,8 @@ static int __ext4_fill_super(struct fs_context *fc, struct super_block *sb)
> sbi->s_journal->j_commit_callback =
> ext4_journal_commit_callback;
>
> - if (ext4_percpu_param_init(sbi))
> + err = ext4_percpu_param_init(sbi);
> + if (err)
> goto failed_mount6;
>
> if (ext4_has_feature_flex_bg(sb))
>
Reviewed-by: Jason Yan <[email protected]>
On 2023/4/28 11:16, Theodore Ts'o wrote:
> There were two ways to return an error code; one was via setting the
> 'err' variable, and the second, if err was zero, was via the 'ret'
> variable. This was both confusing and fragile, and when code was
> factored out of __ext4_fill_super(), some of the error codes returned
> by the original code was replaced by -EINVAL, and in one case, the
> error code was placed by 0, triggering a kernel null pointer
> dereference.
>
> Clean this up by removing the 'ret' variable, leaving only one way to
> setfthe error code to be returned, and restore the errno codes that
setfthe -> set the? Otherwise looks good to me:
Reviewed-by: Jason Yan <[email protected]>