| 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
| ||
|
Message-ID: <lnxxxz7xxwhtcywd2yaudkypffubonvl3zu5ehahzznqj5woov@fn6w6asmsw4q>
Date: Tue, 21 Jan 2025 14:11:26 +0100
From: Jan Kara <jack@...e.cz>
To: libaokun@...weicloud.com
Cc: linux-ext4@...r.kernel.org, tytso@....edu, adilger.kernel@...ger.ca,
jack@...e.cz, linux-kernel@...r.kernel.org, yi.zhang@...wei.com,
yangerkun@...wei.com, Baokun Li <libaokun1@...wei.com>
Subject: Re: [PATCH 4/7] ext4: add ext4_sb_rdonly() helper function
On Fri 17-01-25 16:23:12, libaokun@...weicloud.com wrote:
> From: Baokun Li <libaokun1@...wei.com>
>
> Because both SB_RDONLY and EXT4_FLAGS_EMERGENCY_RO indicate the file system
> is read-only, the ext4_sb_rdonly() helper function is added. This function
> returns true if either flag is set, signifying that the file system is
> read-only.
>
> Then replace some sb_rdonly() with ext4_sb_rdonly() to avoid unexpected
> failures of some read-only operations or modification of the superblock
> after setting EXT4_FLAGS_EMERGENCY_RO.
>
> Signed-off-by: Baokun Li <libaokun1@...wei.com>
I'm not sure we really need this. I rather think more places need
additional ext4_emergency_state() checks. Look:
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 6db052a87b9b..70b556c87b88 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -844,7 +844,7 @@ static int ext4_sample_last_mounted(struct super_block *sb,
> if (likely(ext4_test_mount_flag(sb, EXT4_MF_MNTDIR_SAMPLED)))
> return 0;
>
> - if (sb_rdonly(sb) || !sb_start_intwrite_trylock(sb))
> + if (ext4_sb_rdonly(sb) || !sb_start_intwrite_trylock(sb))
We don't want to be modifying superblock if the filesystem is shutdown so I
think we should have here something like:
if (ext4_emergency_state(sb) || sb_rdonly(sb) ||
!sb_start_intwrite_trylock(sb))
> return 0;
>
> ext4_set_mount_flag(sb, EXT4_MF_MNTDIR_SAMPLED);
> diff --git a/fs/ext4/ioctl.c b/fs/ext4/ioctl.c
> index 7b9ce71c1c81..0807ee8cbcdc 100644
> --- a/fs/ext4/ioctl.c
> +++ b/fs/ext4/ioctl.c
> @@ -1705,7 +1705,7 @@ int ext4_update_overhead(struct super_block *sb, bool force)
> {
> struct ext4_sb_info *sbi = EXT4_SB(sb);
>
> - if (sb_rdonly(sb))
> + if (ext4_sb_rdonly(sb))
> return 0;
Similarly here I think we should have:
if (ext4_emergency_state(sb) || sb_rdonly(sb))
return 0;
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index c12133628ee9..fc5d30123f22 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -473,7 +473,7 @@ static void ext4_maybe_update_superblock(struct super_block *sb)
> __u64 lifetime_write_kbytes;
> __u64 diff_size;
>
> - if (sb_rdonly(sb) || !(sb->s_flags & SB_ACTIVE) ||
> + if (ext4_sb_rdonly(sb) || !(sb->s_flags & SB_ACTIVE) ||
> !journal || (journal->j_flags & JBD2_UNMOUNT))
> return;
And here we should add ext4_emergency_state() check as well.
> @@ -707,7 +707,7 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
> if (test_opt(sb, WARN_ON_ERROR))
> WARN_ON_ONCE(1);
>
> - if (!continue_fs && !sb_rdonly(sb)) {
> + if (!continue_fs && !ext4_sb_rdonly(sb)) {
Here I actually think we should just drop the sb_rdonly() check completely?
Because callers have already checked we are not in emergency state yet and
we want to shutdown the fs (or later flag the emergency RO state) even if
the filesystem is mounted read only?
> set_bit(EXT4_FLAGS_SHUTDOWN, &EXT4_SB(sb)->s_ext4_flags);
> if (journal)
> jbd2_journal_abort(journal, -EIO);
> @@ -737,7 +737,7 @@ static void ext4_handle_error(struct super_block *sb, bool force_ro, int error,
> sb->s_id);
> }
>
> - if (sb_rdonly(sb) || continue_fs)
> + if (ext4_sb_rdonly(sb) || continue_fs)
> return;
This will need a bit of reworking with the emergency ro flag anyway so for
now I'd leave it as is.
>
> ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only");
> @@ -765,7 +765,7 @@ static void update_super_work(struct work_struct *work)
> * We use directly jbd2 functions here to avoid recursing back into
> * ext4 error handling code during handling of previous errors.
> */
> - if (!sb_rdonly(sbi->s_sb) && journal) {
> + if (!ext4_sb_rdonly(sbi->s_sb) && journal) {
> struct buffer_head *sbh = sbi->s_sbh;
> bool call_notify_err = false;
Again here I think we should just add ext4_emergency_state() check because
we don't want to be modifying superblock on shutdown filesystem either. And
in the four cases below as well.
> @@ -1325,12 +1325,12 @@ static void ext4_put_super(struct super_block *sb)
> ext4_mb_release(sb);
> ext4_ext_release(sb);
>
> - if (!sb_rdonly(sb) && !aborted) {
> + if (!ext4_sb_rdonly(sb) && !aborted) {
> ext4_clear_feature_journal_needs_recovery(sb);
> ext4_clear_feature_orphan_present(sb);
> es->s_state = cpu_to_le16(sbi->s_mount_state);
> }
> - if (!sb_rdonly(sb))
> + if (!ext4_sb_rdonly(sb))
> ext4_commit_super(sb);
>
> ext4_group_desc_free(sbi);
> @@ -3693,7 +3693,8 @@ static int ext4_run_li_request(struct ext4_li_request *elr)
> if (group >= elr->lr_next_group) {
> ret = 1;
> if (elr->lr_first_not_zeroed != ngroups &&
> - !sb_rdonly(sb) && test_opt(sb, INIT_INODE_TABLE)) {
> + !ext4_sb_rdonly(sb) &&
> + test_opt(sb, INIT_INODE_TABLE)) {
> elr->lr_next_group = elr->lr_first_not_zeroed;
> elr->lr_mode = EXT4_LI_MODE_ITABLE;
> ret = 0;
> @@ -3998,7 +3999,7 @@ int ext4_register_li_request(struct super_block *sb,
> goto out;
> }
>
> - if (sb_rdonly(sb) ||
> + if (ext4_sb_rdonly(sb) ||
> (test_opt(sb, NO_PREFETCH_BLOCK_BITMAPS) &&
> (first_not_zeroed == ngroups || !test_opt(sb, INIT_INODE_TABLE))))
> goto out;
Honza
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists