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: <c8400390-903e-a754-46cb-ed78cb974726@huawei.com> Date: Tue, 9 Jun 2020 15:03:12 +0800 From: "zhangyi (F)" <yi.zhang@...wei.com> To: Jan Kara <jack@...e.cz> CC: <linux-ext4@...r.kernel.org>, <tytso@....edu>, <adilger.kernel@...ger.ca>, <zhangxiaoxu5@...wei.com> Subject: Re: [PATCH] ext4, jbd2: switch to use completion variable instead of JBD2_REC_ERR On 2020/6/8 23:36, Jan Kara wrote: > On Mon 08-06-20 23:08:42, zhangyi (F) wrote: >> Hi, Jan. >> >> On 2020/6/8 15:57, Jan Kara wrote: >>> On Tue 26-05-20 22:20:39, zhangyi (F) wrote: >>>> In the ext4 filesystem with errors=panic, if one process is recording >>>> errno in the superblock when invoking jbd2_journal_abort() due to some >>>> error cases, it could be raced by another __ext4_abort() which is >>>> setting the SB_RDONLY flag but missing panic because errno has not been >>>> recorded. >>>> >>>> jbd2_journal_abort() >>>> journal->j_flags |= JBD2_ABORT; >>>> jbd2_journal_update_sb_errno() >>>> | __ext4_abort() >>>> | sb->s_flags |= SB_RDONLY; >>>> | if (!JBD2_REC_ERR) >>>> | return; >>>> journal->j_flags |= JBD2_REC_ERR; >>>> >>>> Finally, it will no longer trigger panic because the filesystem has >>>> already been set read-only. Fix this by remove JBD2_REC_ERR and switch >>>> to use completion variable instead. >>> >>> Thanks for the patch! I don't quite understand how this last part can >>> happen: "Finally, it will no longer trigger panic because the filesystem has >>> already been set read-only." >>> >>> AFAIU jbd2_journal_abort() gets called somewhere from jbd2 so ext4 doesn't >>> know about it. At the same time ext4_abort() gets called somewhere from >>> ext4 and races as you describe above. OK. But then the next ext4_abort() >>> call should panic() just fine. What am I missing? I understand that we >>> might want that the first ext4_abort() already triggers the panic but I'd >>> like to understand whether that's the bug you're trying to fix or something >>> else... >>> >> Since the fs is marked to read-only in the first ext4_abort(), the >> ext4_journal_check_start() will return -EROFS immediately, so we >> have no chance to invoke ext4_abort() again and trigger panic. >> >> static int ext4_journal_check_start(struct super_block *sb) >> { >> ... >> if (sb_rdonly(sb)) >> return -EROFS; >> ... >> } > > Ah, I see. I didn't look into ext4_journal_check_start() in particular. > Thanks for explanation. > >>> WRT the solution I think that the completion you add unnecessarily >>> complicates matters. I'd rather introduce j_abort_mutex to the journal and >>> all jbd2_journal_abort() calls will take it and release it once everything >>> is done. That way we can remove JBD2_REC_ERR, races are avoided, and the >>> filesystem (ext4 or ocfs2) knows that after its call to >>> jbd2_journal_abort() completes, journal abort is completed (either by us or >>> someone else) and so we are free to panic. No need for strange >>> wait_for_completion() calls in ext4_handle_error() or __ext4_abort() and >>> the error handling is again fully self-contained within the jbd2 layer. >>> >> >> Now, the race condition is between jbd2_journal_abort() and >> ext4_handle_error()/__ext4_abort(), so if we only use j_abort_mutex, it >> will re-introduce the problem which 4327ba52afd03 want to fix, think >> about below case: >> >> jbd2_journal_commit_transaction() ext4_journal_check_start() ext4_journal_check_start() >> jbd2_journal_abort() >> lock j_abort_mutex >> journal->j_flags |= JBD2_ABORT; >> __ext4_abort() >> __ext4_abort() >> sb->s_flags |= SB_RDONLY; >> panic() <-- system panic here due to "sb_rdonly()==true" >> jbd2_journal_abort() <-- block >> jbd2_journal_update_sb_errno <-- not write to disk >> unlock j_abort_mutex >> >> The system will panic before the error info is written to the journal's >> super block. Use j_abort_mutex to avoid the race between jbd2_journal_abort() >> and ext4_handle_error()/__ext4_abort() is depends on the both of those two >> ext4 error handlers invoke jbd2_journal_abort(), if not, the race will >> re-open. > > Yes, you're right. Or we could move sb->s_flags |= SB_RDONLY in > __ext4_abort() after jbd2_journal_abort() call, can't we? > OK, it looks good, thanks for your suggestion, will do. Thanks, Yi. > >>>> Fixes: 4327ba52afd03 ("ext4, jbd2: ensure entering into panic after recording an error in superblock") >>>> Signed-off-by: zhangyi (F) <yi.zhang@...wei.com> >>>> --- >>>> fs/ext4/super.c | 25 +++++++++++++------------ >>>> fs/jbd2/journal.c | 6 ++---- >>>> include/linux/jbd2.h | 6 +++++- >>>> 3 files changed, 20 insertions(+), 17 deletions(-) >>>> >>>> diff --git a/fs/ext4/super.c b/fs/ext4/super.c >>>> index bf5fcb477f66..987a0bd5b78a 100644 >>>> --- a/fs/ext4/super.c >>>> +++ b/fs/ext4/super.c >>>> @@ -495,6 +495,8 @@ static bool system_going_down(void) >>>> >>>> static void ext4_handle_error(struct super_block *sb) >>>> { >>>> + struct ext4_sb_info *sbi = EXT4_SB(sb); >>>> + >>>> if (test_opt(sb, WARN_ON_ERROR)) >>>> WARN_ON_ONCE(1); >>>> >>>> @@ -502,9 +504,9 @@ static void ext4_handle_error(struct super_block *sb) >>>> return; >>>> >>>> if (!test_opt(sb, ERRORS_CONT)) { >>>> - journal_t *journal = EXT4_SB(sb)->s_journal; >>>> + journal_t *journal = sbi->s_journal; >>>> >>>> - EXT4_SB(sb)->s_mount_flags |= EXT4_MF_FS_ABORTED; >>>> + sbi->s_mount_flags |= EXT4_MF_FS_ABORTED; >>>> if (journal) >>>> jbd2_journal_abort(journal, -EIO); >>>> } >>>> @@ -522,9 +524,8 @@ static void ext4_handle_error(struct super_block *sb) >>>> smp_wmb(); >>>> sb->s_flags |= SB_RDONLY; >>>> } else if (test_opt(sb, ERRORS_PANIC)) { >>>> - if (EXT4_SB(sb)->s_journal && >>>> - !(EXT4_SB(sb)->s_journal->j_flags & JBD2_REC_ERR)) >>>> - return; >>>> + if (sbi->s_journal && is_journal_aborted(sbi->s_journal)) >>>> + wait_for_completion(&sbi->s_journal->j_record_errno); >>>> panic("EXT4-fs (device %s): panic forced after error\n", >>>> sb->s_id); >>>> } >>>> @@ -710,10 +711,11 @@ void __ext4_std_error(struct super_block *sb, const char *function, >>>> void __ext4_abort(struct super_block *sb, const char *function, >>>> unsigned int line, int error, const char *fmt, ...) >>>> { >>>> + struct ext4_sb_info *sbi = EXT4_SB(sb); >>>> struct va_format vaf; >>>> va_list args; >>>> >>>> - if (unlikely(ext4_forced_shutdown(EXT4_SB(sb)))) >>>> + if (unlikely(ext4_forced_shutdown(sbi))) >>>> return; >>>> >>>> save_error_info(sb, error, 0, 0, function, line); >>>> @@ -726,20 +728,19 @@ void __ext4_abort(struct super_block *sb, const char *function, >>>> >>>> if (sb_rdonly(sb) == 0) { >>>> ext4_msg(sb, KERN_CRIT, "Remounting filesystem read-only"); >>>> - EXT4_SB(sb)->s_mount_flags |= EXT4_MF_FS_ABORTED; >>>> + sbi->s_mount_flags |= EXT4_MF_FS_ABORTED; >>>> /* >>>> * Make sure updated value of ->s_mount_flags will be visible >>>> * before ->s_flags update >>>> */ >>>> smp_wmb(); >>>> sb->s_flags |= SB_RDONLY; >>>> - if (EXT4_SB(sb)->s_journal) >>>> - jbd2_journal_abort(EXT4_SB(sb)->s_journal, -EIO); >>>> + if (sbi->s_journal) >>>> + jbd2_journal_abort(sbi->s_journal, -EIO); >>>> } >>>> if (test_opt(sb, ERRORS_PANIC) && !system_going_down()) { >>>> - if (EXT4_SB(sb)->s_journal && >>>> - !(EXT4_SB(sb)->s_journal->j_flags & JBD2_REC_ERR)) >>>> - return; >>>> + if (sbi->s_journal && is_journal_aborted(sbi->s_journal)) >>>> + wait_for_completion(&sbi->s_journal->j_record_errno); >>>> panic("EXT4-fs panic from previous error\n"); >>>> } >>>> } >>>> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c >>>> index a49d0e670ddf..b8acdb2f7ac7 100644 >>>> --- a/fs/jbd2/journal.c >>>> +++ b/fs/jbd2/journal.c >>>> @@ -1140,6 +1140,7 @@ static journal_t *journal_init_common(struct block_device *bdev, >>>> init_waitqueue_head(&journal->j_wait_commit); >>>> init_waitqueue_head(&journal->j_wait_updates); >>>> init_waitqueue_head(&journal->j_wait_reserved); >>>> + init_completion(&journal->j_record_errno); >>>> mutex_init(&journal->j_barrier); >>>> mutex_init(&journal->j_checkpoint_mutex); >>>> spin_lock_init(&journal->j_revoke_lock); >>>> @@ -2188,10 +2189,7 @@ void jbd2_journal_abort(journal_t *journal, int errno) >>>> * layer could realise that a filesystem check is needed. >>>> */ >>>> jbd2_journal_update_sb_errno(journal); >>>> - >>>> - write_lock(&journal->j_state_lock); >>>> - journal->j_flags |= JBD2_REC_ERR; >>>> - write_unlock(&journal->j_state_lock); >>>> + complete_all(&journal->j_record_errno); >>>> } >>>> >>>> /** >>>> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h >>>> index f613d8529863..0f623b0c347f 100644 >>>> --- a/include/linux/jbd2.h >>>> +++ b/include/linux/jbd2.h >>>> @@ -765,6 +765,11 @@ struct journal_s >>>> */ >>>> int j_errno; >>>> >>>> + /** >>>> + * @j_record_errno: complete to record errno in the journal superblock >>>> + */ >>>> + struct completion j_record_errno; >>>> + >>>> /** >>>> * @j_sb_buffer: The first part of the superblock buffer. >>>> */ >>>> @@ -1247,7 +1252,6 @@ JBD2_FEATURE_INCOMPAT_FUNCS(csum3, CSUM_V3) >>>> #define JBD2_ABORT_ON_SYNCDATA_ERR 0x040 /* Abort the journal on file >>>> * data write error in ordered >>>> * mode */ >>>> -#define JBD2_REC_ERR 0x080 /* The errno in the sb has been recorded */ >>>> >>>> /* >>>> * Function declarations for the journaling transaction and buffer >>>> -- >>>> 2.21.3 >>>> >>
Powered by blists - more mailing lists