| 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: <9c62bb25-cfe4-4dbc-a1f0-c0afe5a005b5@huawei.com>
Date: Tue, 21 Jan 2025 22:08:21 +0800
From: Baokun Li <libaokun1@...wei.com>
To: Jan Kara <jack@...e.cz>
CC: <libaokun@...weicloud.com>, <linux-ext4@...r.kernel.org>, <tytso@....edu>,
<adilger.kernel@...ger.ca>, <linux-kernel@...r.kernel.org>,
<yi.zhang@...wei.com>, <yangerkun@...wei.com>, Baokun Li
<libaokun1@...wei.com>, Baokun Li <libaokun@...weicloud.com>
Subject: Re: [PATCH 4/7] ext4: add ext4_sb_rdonly() helper function
On 2025/1/21 21:11, Jan Kara wrote:
> 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:
Make sense. 🤔
>
>> 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))
That's right, if the file system is already down, the caller will
return -EIO before calling ext4_sample_last_mounted().
>> 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;
Alright, even though there could be some overhead inconsistencies here,
we update s_overhead when mounting if bigalloc is not enabled.
>> 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.
Right, the return value doesn't matter here.
>> @@ -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?
Yeah, I totally agree, the error handling is now completely
independent of sb_rdonly(), so we can get rid of it.
>
>> 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.
Yes, we can keep it here for now and then replace it with
ext4_emergency_ro() later on.
>>
>> 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.
Yeah, let me just use ext4_emergency_state() directly instead of
a new helper function in the next version.
Thanks for your review and the detailed explanation!
Regards,
Baokun
>> @@ -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
Powered by blists - more mailing lists