| 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, 8 Sep 2023 14:26:20 +0200
From: Jan Kara <jack@...e.cz>
To: Zhihao Cheng <chengzhihao1@...wei.com>
Cc: Jan Kara <jack@...e.cz>, tytso@....edu, jack@...e.com,
linux-ext4@...r.kernel.org, yi.zhang@...wei.com
Subject: Re: [PATCH] jbd2: Fix potential data lost in recovering journal
raced with synchronizing fs bdev
On Fri 08-09-23 20:02:36, Zhihao Cheng wrote:
> 在 2023/9/8 19:14, Jan Kara 写道:
> Hi Jan,
> > On Fri 08-09-23 17:28:08, Zhihao Cheng wrote:
> > > JBD2 makes sure journal data is fallen on fs device by sync_blockdev(),
> > > however, other process could intercept the EIO information from bdev's
> > > mapping, which leads journal recovering successful even EIO occurs during
> > > data written back to fs device.
> > >
> > > We found this problem in our product, iscsi + multipath is chosen for block
> > > device of ext4. Unstable network may trigger kpartx to rescan partitions in
> > > device mapper layer. Detailed process is shown as following:
> > >
> > > mount kpartx irq
> > > jbd2_journal_recover
> > > do_one_pass
> > > memcpy(nbh->b_data, obh->b_data) // copy data to fs dev from journal
> > > mark_buffer_dirty // mark bh dirty
> > > vfs_read
> > > generic_file_read_iter // dio
> > > filemap_write_and_wait_range
> > > __filemap_fdatawrite_range
> > > do_writepages
> > > block_write_full_folio
> > > submit_bh_wbc
> > > >> EIO occurs in disk <<
> > > end_buffer_async_write
> > > mark_buffer_write_io_error
> > > mapping_set_error
> > > set_bit(AS_EIO, &mapping->flags) // set!
> > > filemap_check_errors
> > > test_and_clear_bit(AS_EIO, &mapping->flags) // clear!
> > > err2 = sync_blockdev
> > > filemap_write_and_wait
> > > filemap_check_errors
> > > test_and_clear_bit(AS_EIO, &mapping->flags) // false
> > > err2 = 0
> > >
> > > Filesystem is mounted successfully even data from journal is failed written
> > > into disk, and ext4/ocfs2 could become corrupted.
> > >
> > > Fix it by comparing the wb_err state in fs block device before recovering
> > > and after recovering.
> > >
> > > Fetch a reproducer in [Link].
> > >
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217888
> > > Cc: stable@...r.kernel.org
> > > Signed-off-by: Zhihao Cheng <chengzhihao1@...wei.com>
> > > Signed-off-by: Zhang Yi <yi.zhang@...wei.com>
> >
> > Thanks for the patch! It makes sense but it is somewhat inconsistent with
> > how we deal with other checks for metadata IO errors in ext4. We do those
> > checks in ext4 through ext4_check_bdev_write_error(). So I wonder if in
> > this case we shouldn't move the errseq_check_and_advance() in
> > __ext4_fill_super() earlier (before journal setup) and then use it in
> > ext4_load_and_init_journal() to detect errors during background metadata
> > writeback. What do you think?
> >
>
> Do you mean that modify like this?
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 38217422f938..3f6239f8cc4e 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -4907,6 +4907,13 @@ static int ext4_load_and_init_journal(struct
> super_block *sb,
> if (err)
> return err;
>
> + err =
> errseq_check_and_advance(&sb->s_bdev->bd_inode->i_mapping->wb_err,
> + &sbi->s_bdev_wb_err);
> + if (err) {
> + ext4_msg(sb, KERN_ERR, "Failed to sync fs block device");
Maybe make the message (..., "Background error %d when loading journal", err)
> + goto out;
> + }
> +
> if (ext4_has_feature_64bit(sb) &&
> !jbd2_journal_set_features(EXT4_SB(sb)->s_journal, 0, 0,
> JBD2_FEATURE_INCOMPAT_64BIT)) {
> @@ -5365,6 +5372,13 @@ static int __ext4_fill_super(struct fs_context *fc,
> struct super_block *sb)
> goto failed_mount3a;
> }
>
> + /*
> + * Save the original bdev mapping's wb_err value which could be
> + * used to detect the metadata async write error.
> + */
> + spin_lock_init(&sbi->s_bdev_wb_lock);
> + errseq_check_and_advance(&sb->s_bdev->bd_inode->i_mapping->wb_err,
> + &sbi->s_bdev_wb_err);
> err = -EINVAL;
> /*
> * The first inode we look at is the journal inode. Don't try
> @@ -5571,13 +5585,6 @@ static int __ext4_fill_super(struct fs_context *fc,
> struct super_block *sb)
> }
> #endif /* CONFIG_QUOTA */
>
> - /*
> - * Save the original bdev mapping's wb_err value which could be
> - * used to detect the metadata async write error.
> - */
> - spin_lock_init(&sbi->s_bdev_wb_lock);
> - errseq_check_and_advance(&sb->s_bdev->bd_inode->i_mapping->wb_err,
> - &sbi->s_bdev_wb_err);
> EXT4_SB(sb)->s_mount_state |= EXT4_ORPHAN_FS;
> ext4_orphan_cleanup(sb, es);
> EXT4_SB(sb)->s_mount_state &= ~EXT4_ORPHAN_FS;
Yes, this looks good to me.
> If so, there are two points:
> 1. ocfs2 also uses jbd2, we only fix ext4.
Yes, but ocfs2 also currently doesn't handle any IO errors due to failed
background writeback of buffers needing checkpointing which is IMO a much
more probable issue than a problem during journal replay. So yes, we can
think whether we don't want to move all the background EIO handling into
jbd2 but for this particular case I think the inconsistency is worse than
the benefit of fixing ocfs2.
> 2. EIO from ext4_commit_super() in ext4_load_journal is ignored, now ext4
> will fail mounting.
Yes, but why should we continue mounting when we cannot write to the
superblock? I see no real benefit...
Honza
> > > ---
> > > fs/jbd2/recovery.c | 7 +++++++
> > > 1 file changed, 7 insertions(+)
> > >
> > > diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> > > index c269a7d29a46..0fecaa6a3ac6 100644
> > > --- a/fs/jbd2/recovery.c
> > > +++ b/fs/jbd2/recovery.c
> > > @@ -289,6 +289,8 @@ int jbd2_journal_recover(journal_t *journal)
> > > journal_superblock_t * sb;
> > > struct recovery_info info;
> > > + errseq_t wb_err;
> > > + struct address_space *mapping;
> > > memset(&info, 0, sizeof(info));
> > > sb = journal->j_superblock;
> > > @@ -306,6 +308,8 @@ int jbd2_journal_recover(journal_t *journal)
> > > return 0;
> > > }
> > > + mapping = journal->j_fs_dev->bd_inode->i_mapping;
> > > + errseq_check_and_advance(&mapping->wb_err, &wb_err);
> > > err = do_one_pass(journal, &info, PASS_SCAN);
> > > if (!err)
> > > err = do_one_pass(journal, &info, PASS_REVOKE);
> > > @@ -327,6 +331,9 @@ int jbd2_journal_recover(journal_t *journal)
> > > jbd2_journal_clear_revoke(journal);
> > > err2 = sync_blockdev(journal->j_fs_dev);
> > > + if (!err)
> > > + err = err2;
> > > + err2 = errseq_check_and_advance(&mapping->wb_err, &wb_err);
> > > if (!err)
> > > err = err2;
> > > /* Make sure all replayed data is on permanent storage */
> > > --
> > > 2.39.2
> > >
>
--
Jan Kara <jack@...e.com>
SUSE Labs, CR
Powered by blists - more mailing lists