linux-ext4 - Re: [PATCH] jbd2: fix race when writing superblock

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 28 Jan 2019 16:06:34 +0100
From:   Jan Kara <jack@...e.cz>
To:     "zhangyi (F)" <yi.zhang@...wei.com>
Cc:     linux-ext4@...r.kernel.org, tytso@....edu, jack@...e.cz,
        adilger.kernel@...ger.ca, miaoxie@...wei.com
Subject: Re: [PATCH] jbd2: fix race when writing superblock

On Fri 25-01-19 20:40:23, zhangyi (F) wrote:
> The jbd2 superblock is lockless now, so there is probably a race
> condition between writing it so disk and modifing contents of it, which
> may lead to checksum error. The following race is the one case that we
> have captured.
> 
> jbd2                                fsstress
> jbd2_journal_commit_transaction
>  jbd2_journal_update_sb_log_tail
>   jbd2_write_superblock
>    jbd2_superblock_csum_set         jbd2_journal_revoke
>                                      jbd2_journal_set_features(revork)
>                                      modify superblock
>    submit_bh(checksum incorrect)
> 
> One alternative fix is to lock the buffer everywhere that modifing it,
> but it may expensive. So this patch introduce an in-memory jbd2
> superblock, update it to the on-disk superblock and calculate checksum
> at write time.
> 
> This checksum corruption problem can be reproduced by xfstests
> generic/475.
> 
> Signed-off-by: zhangyi (F) <yi.zhang@...wei.com>

Thanks for the analysis and the patch! I think that copying of the
superblock to a temporary buffer is not free either and the frequent
updates of journal superblock are synchronized by j_checkpoint_mutex
anyway. So I think that using buffer lock when modifying journal
superblock contents is actually the easiest way forward.

								Honza
> ---
>  fs/jbd2/journal.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 8ef6b6d..ad59909 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1133,6 +1133,7 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  	static struct lock_class_key jbd2_trans_commit_key;
>  	journal_t *journal;
>  	int err;
> +	journal_superblock_t *sb = NULL;
>  	struct buffer_head *bh;
>  	int n;
>  
> @@ -1182,6 +1183,10 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  	if (!journal->j_wbuf)
>  		goto err_cleanup;
>  
> +	sb = kzalloc(sizeof(*sb), GFP_KERNEL);
> +	if (!sb)
> +		goto err_cleanup;
> +
>  	bh = getblk_unmovable(journal->j_dev, start, journal->j_blocksize);
>  	if (!bh) {
>  		pr_err("%s: Cannot get buffer for journal superblock\n",
> @@ -1189,11 +1194,12 @@ static journal_t *journal_init_common(struct block_device *bdev,
>  		goto err_cleanup;
>  	}
>  	journal->j_sb_buffer = bh;
> -	journal->j_superblock = (journal_superblock_t *)bh->b_data;
> +	journal->j_superblock = sb;
>  
>  	return journal;
>  
>  err_cleanup:
> +	kfree(sb);
>  	kfree(journal->j_wbuf);
>  	jbd2_journal_destroy_revoke(journal);
>  	kfree(journal);
> @@ -1360,6 +1366,7 @@ static int jbd2_write_superblock(journal_t *journal, int write_flags)
>  {
>  	struct buffer_head *bh = journal->j_sb_buffer;
>  	journal_superblock_t *sb = journal->j_superblock;
> +	journal_superblock_t *raw_sb = (journal_superblock_t *)bh->b_data;
>  	int ret;
>  
>  	trace_jbd2_write_superblock(journal, write_flags);
> @@ -1381,7 +1388,8 @@ static int jbd2_write_superblock(journal_t *journal, int write_flags)
>  		clear_buffer_write_io_error(bh);
>  		set_buffer_uptodate(bh);
>  	}
> -	jbd2_superblock_csum_set(journal, sb);
> +	memcpy(raw_sb, sb, sizeof(journal_superblock_t));
> +	jbd2_superblock_csum_set(journal, raw_sb);
>  	get_bh(bh);
>  	bh->b_end_io = end_buffer_write_sync;
>  	ret = submit_bh(REQ_OP_WRITE, write_flags, bh);
> @@ -1507,7 +1515,7 @@ EXPORT_SYMBOL(jbd2_journal_update_sb_errno);
>  static int journal_get_superblock(journal_t *journal)
>  {
>  	struct buffer_head *bh;
> -	journal_superblock_t *sb;
> +	journal_superblock_t *sb, *raw_sb;
>  	int err = -EIO;
>  
>  	bh = journal->j_sb_buffer;
> @@ -1526,7 +1534,9 @@ static int journal_get_superblock(journal_t *journal)
>  	if (buffer_verified(bh))
>  		return 0;
>  
> +	raw_sb = (journal_superblock_t *)bh->b_data;
>  	sb = journal->j_superblock;
> +	memcpy(sb, raw_sb, sizeof(journal_superblock_t));
>  
>  	err = -EINVAL;
>  
> @@ -1777,6 +1787,7 @@ int jbd2_journal_destroy(journal_t *journal)
>  		jbd2_journal_destroy_revoke(journal);
>  	if (journal->j_chksum_driver)
>  		crypto_free_shash(journal->j_chksum_driver);
> +	kfree(journal->j_superblock);
>  	kfree(journal->j_wbuf);
>  	kfree(journal);
>  
> -- 
> 2.7.4
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR