[PATCH] jbd2: fix race when writing superblock

[PATCH] jbd2: fix race when writing superblock

[zhangyi (F)]

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@huawei.com>
---
 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

Re: [PATCH] jbd2: fix race when writing superblock

[Jan Kara]

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@huawei.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@suse.com>
SUSE Labs, CR

Re: [PATCH] jbd2: fix race when writing superblock

[Theodore Y. Ts'o]

On Mon, Jan 28, 2019 at 04:06:34PM +0100, Jan Kara wrote:
> 
> 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.

Agreed.  It turns out we always write the superblock after we modify
it, so we have to call lock_buffer() anyway; the patch just moves it
so it happens a bit earlier.

Please take a look at this fix.  Zhangyi, can you confirm whether your
test failures of generic/475 are addressed with this patch?

					- Ted

From 9bb7a0025fc43bc517c5e30c638f9ca389600b15 Mon Sep 17 00:00:00 2001
From: Theodore Ts'o <tytso@mit.edu>
Date: Mon, 11 Feb 2019 14:52:14 -0500
Subject: [PATCH] jbd2: fix race when writing superblock

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)

Fix this by locking the buffer head before modifing it.  We always
write the jbd2 superblock after we modify it, so this just means
calling the lock_buffer() a little earlier.

This checksum corruption problem can be reproduced by xfstests
generic/475.

Reported-by: zhangyi (F) <yi.zhang@huawei.com>
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---
 fs/jbd2/journal.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 88d8f22d2cba..7a38b56c2544 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1356,6 +1356,10 @@ static int journal_reset(journal_t *journal)
 	return jbd2_journal_start_thread(journal);
 }
 
+/*
+ * This function expects that the caller will have locked the journal
+ * buffer head, and will return with it unlocked
+ */
 static int jbd2_write_superblock(journal_t *journal, int write_flags)
 {
 	struct buffer_head *bh = journal->j_sb_buffer;
@@ -1365,7 +1369,6 @@ static int jbd2_write_superblock(journal_t *journal, int write_flags)
 	trace_jbd2_write_superblock(journal, write_flags);
 	if (!(journal->j_flags & JBD2_BARRIER))
 		write_flags &= ~(REQ_FUA | REQ_PREFLUSH);
-	lock_buffer(bh);
 	if (buffer_write_io_error(bh)) {
 		/*
 		 * Oh, dear.  A previous attempt to write the journal
@@ -1424,6 +1427,7 @@ int jbd2_journal_update_sb_log_tail(journal_t *journal, tid_t tail_tid,
 	jbd_debug(1, "JBD2: updating superblock (start %lu, seq %u)\n",
 		  tail_block, tail_tid);
 
+	lock_buffer(journal->j_sb_buffer);
 	sb->s_sequence = cpu_to_be32(tail_tid);
 	sb->s_start    = cpu_to_be32(tail_block);
 
@@ -1454,18 +1458,15 @@ static void jbd2_mark_journal_empty(journal_t *journal, int write_op)
 	journal_superblock_t *sb = journal->j_superblock;
 
 	BUG_ON(!mutex_is_locked(&journal->j_checkpoint_mutex));
-	read_lock(&journal->j_state_lock);
-	/* Is it already empty? */
-	if (sb->s_start == 0) {
-		read_unlock(&journal->j_state_lock);
+	lock_buffer(journal->j_sb_buffer);
+	if (sb->s_start == 0)	/* Is it already empty? */
 		return;
-	}
+
 	jbd_debug(1, "JBD2: Marking journal as empty (seq %d)\n",
 		  journal->j_tail_sequence);
 
 	sb->s_sequence = cpu_to_be32(journal->j_tail_sequence);
 	sb->s_start    = cpu_to_be32(0);
-	read_unlock(&journal->j_state_lock);
 
 	jbd2_write_superblock(journal, write_op);
 
@@ -1488,9 +1489,8 @@ void jbd2_journal_update_sb_errno(journal_t *journal)
 	journal_superblock_t *sb = journal->j_superblock;
 	int errcode;
 
-	read_lock(&journal->j_state_lock);
+	lock_buffer(journal->j_sb_buffer);
 	errcode = journal->j_errno;
-	read_unlock(&journal->j_state_lock);
 	if (errcode == -ESHUTDOWN)
 		errcode = 0;
 	jbd_debug(1, "JBD2: updating superblock error (errno %d)\n", errcode);
-- 
2.19.1

[PATCH -v2] jbd2: fix race when writing superblock

[Theodore Ts'o]

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)

Fix this by locking the buffer head before modifing it.  We always
write the jbd2 superblock after we modify it, so this just means
calling the lock_buffer() a little earlier.

This checksum corruption problem can be reproduced by xfstests
generic/475.

Reported-by: zhangyi (F) <yi.zhang@huawei.com>
Suggested-by: Jan Kara <jack@suse.cz>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
---

v1->v2:
  - Fix missing unlock when jbd2_mark_journal_empty() doesn't need to
    do anything

 fs/jbd2/journal.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 88d8f22d2cba..a8082c86d569 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1356,6 +1356,10 @@ static int journal_reset(journal_t *journal)
 	return jbd2_journal_start_thread(journal);
 }
 
+/*
+ * This function expects that the caller will have locked the journal
+ * buffer head, and will return with it unlocked
+ */
 static int jbd2_write_superblock(journal_t *journal, int write_flags)
 {
 	struct buffer_head *bh = journal->j_sb_buffer;
@@ -1365,7 +1369,6 @@ static int jbd2_write_superblock(journal_t *journal, int write_flags)
 	trace_jbd2_write_superblock(journal, write_flags);
 	if (!(journal->j_flags & JBD2_BARRIER))
 		write_flags &= ~(REQ_FUA | REQ_PREFLUSH);
-	lock_buffer(bh);
 	if (buffer_write_io_error(bh)) {
 		/*
 		 * Oh, dear.  A previous attempt to write the journal
@@ -1424,6 +1427,7 @@ int jbd2_journal_update_sb_log_tail(journal_t *journal, tid_t tail_tid,
 	jbd_debug(1, "JBD2: updating superblock (start %lu, seq %u)\n",
 		  tail_block, tail_tid);
 
+	lock_buffer(journal->j_sb_buffer);
 	sb->s_sequence = cpu_to_be32(tail_tid);
 	sb->s_start    = cpu_to_be32(tail_block);
 
@@ -1454,18 +1458,17 @@ static void jbd2_mark_journal_empty(journal_t *journal, int write_op)
 	journal_superblock_t *sb = journal->j_superblock;
 
 	BUG_ON(!mutex_is_locked(&journal->j_checkpoint_mutex));
-	read_lock(&journal->j_state_lock);
-	/* Is it already empty? */
-	if (sb->s_start == 0) {
-		read_unlock(&journal->j_state_lock);
+	lock_buffer(journal->j_sb_buffer);
+	if (sb->s_start == 0) {		/* Is it already empty? */
+		unlock_buffer(journal->j_sb_buffer);
 		return;
 	}
+
 	jbd_debug(1, "JBD2: Marking journal as empty (seq %d)\n",
 		  journal->j_tail_sequence);
 
 	sb->s_sequence = cpu_to_be32(journal->j_tail_sequence);
 	sb->s_start    = cpu_to_be32(0);
-	read_unlock(&journal->j_state_lock);
 
 	jbd2_write_superblock(journal, write_op);
 
@@ -1488,9 +1491,8 @@ void jbd2_journal_update_sb_errno(journal_t *journal)
 	journal_superblock_t *sb = journal->j_superblock;
 	int errcode;
 
-	read_lock(&journal->j_state_lock);
+	lock_buffer(journal->j_sb_buffer);
 	errcode = journal->j_errno;
-	read_unlock(&journal->j_state_lock);
 	if (errcode == -ESHUTDOWN)
 		errcode = 0;
 	jbd_debug(1, "JBD2: updating superblock error (errno %d)\n", errcode);
-- 
2.19.1

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

[zhangyi (F)]

On 2019/2/12 9:19, Theodore Ts'o 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)
> 
> Fix this by locking the buffer head before modifing it.  We always
> write the jbd2 superblock after we modify it, so this just means
> calling the lock_buffer() a little earlier.
> 

I check the Linux 5.0-rc6 source code, there is one exception, which is
jbd2_journal_set_features(), it will modify the jbd2 superblock buffer
and will not wirte the jbd2 superblock. So the buffer is still lockless
in this case and the race mentioned above is still there.

Thanks,
Yi.

> This checksum corruption problem can be reproduced by xfstests
> generic/475.
> 
> Reported-by: zhangyi (F) <yi.zhang@huawei.com>
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> ---
> 
> v1->v2:
>   - Fix missing unlock when jbd2_mark_journal_empty() doesn't need to
>     do anything
> 
>  fs/jbd2/journal.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 88d8f22d2cba..a8082c86d569 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1356,6 +1356,10 @@ static int journal_reset(journal_t *journal)
>  	return jbd2_journal_start_thread(journal);
>  }
>  
> +/*
> + * This function expects that the caller will have locked the journal
> + * buffer head, and will return with it unlocked
> + */
>  static int jbd2_write_superblock(journal_t *journal, int write_flags)
>  {
>  	struct buffer_head *bh = journal->j_sb_buffer;
> @@ -1365,7 +1369,6 @@ static int jbd2_write_superblock(journal_t *journal, int write_flags)
>  	trace_jbd2_write_superblock(journal, write_flags);
>  	if (!(journal->j_flags & JBD2_BARRIER))
>  		write_flags &= ~(REQ_FUA | REQ_PREFLUSH);
> -	lock_buffer(bh);
>  	if (buffer_write_io_error(bh)) {
>  		/*
>  		 * Oh, dear.  A previous attempt to write the journal
> @@ -1424,6 +1427,7 @@ int jbd2_journal_update_sb_log_tail(journal_t *journal, tid_t tail_tid,
>  	jbd_debug(1, "JBD2: updating superblock (start %lu, seq %u)\n",
>  		  tail_block, tail_tid);
>  
> +	lock_buffer(journal->j_sb_buffer);
>  	sb->s_sequence = cpu_to_be32(tail_tid);
>  	sb->s_start    = cpu_to_be32(tail_block);
>  
> @@ -1454,18 +1458,17 @@ static void jbd2_mark_journal_empty(journal_t *journal, int write_op)
>  	journal_superblock_t *sb = journal->j_superblock;
>  
>  	BUG_ON(!mutex_is_locked(&journal->j_checkpoint_mutex));
> -	read_lock(&journal->j_state_lock);
> -	/* Is it already empty? */
> -	if (sb->s_start == 0) {
> -		read_unlock(&journal->j_state_lock);
> +	lock_buffer(journal->j_sb_buffer);
> +	if (sb->s_start == 0) {		/* Is it already empty? */
> +		unlock_buffer(journal->j_sb_buffer);
>  		return;
>  	}
> +
>  	jbd_debug(1, "JBD2: Marking journal as empty (seq %d)\n",
>  		  journal->j_tail_sequence);
>  
>  	sb->s_sequence = cpu_to_be32(journal->j_tail_sequence);
>  	sb->s_start    = cpu_to_be32(0);
> -	read_unlock(&journal->j_state_lock);
>  
>  	jbd2_write_superblock(journal, write_op);
>  
> @@ -1488,9 +1491,8 @@ void jbd2_journal_update_sb_errno(journal_t *journal)
>  	journal_superblock_t *sb = journal->j_superblock;
>  	int errcode;
>  
> -	read_lock(&journal->j_state_lock);
> +	lock_buffer(journal->j_sb_buffer);
>  	errcode = journal->j_errno;
> -	read_unlock(&journal->j_state_lock);
>  	if (errcode == -ESHUTDOWN)
>  		errcode = 0;
>  	jbd_debug(1, "JBD2: updating superblock error (errno %d)\n", errcode);
> 

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

[Jan Kara]

On Wed 13-02-19 10:17:39, zhangyi (F) wrote:
> On 2019/2/12 9:19, Theodore Ts'o 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)
> > 
> > Fix this by locking the buffer head before modifing it.  We always
> > write the jbd2 superblock after we modify it, so this just means
> > calling the lock_buffer() a little earlier.
> > 
> 
> I check the Linux 5.0-rc6 source code, there is one exception, which is
> jbd2_journal_set_features(), it will modify the jbd2 superblock buffer
> and will not wirte the jbd2 superblock. So the buffer is still lockless
> in this case and the race mentioned above is still there.

So I've ignored jbd2_journal_set_features() as it is used only during mount
and so there's no possibility of a race. However now when checking again I
can see that jbd2_journal_revoke() does call jbd2_journal_set_features() as
well. Although that will modify journal superblock only on the first
revoke, there's a tiny chance that this indeed races with other journal
superblock modification. So I agree we should protect those changes with
buffer lock as well.

								Honza
> 
> Thanks,
> Yi.
> 
> > This checksum corruption problem can be reproduced by xfstests
> > generic/475.
> > 
> > Reported-by: zhangyi (F) <yi.zhang@huawei.com>
> > Suggested-by: Jan Kara <jack@suse.cz>
> > Signed-off-by: Theodore Ts'o <tytso@mit.edu>
> > ---
> > 
> > v1->v2:
> >   - Fix missing unlock when jbd2_mark_journal_empty() doesn't need to
> >     do anything
> > 
> >  fs/jbd2/journal.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> > index 88d8f22d2cba..a8082c86d569 100644
> > --- a/fs/jbd2/journal.c
> > +++ b/fs/jbd2/journal.c
> > @@ -1356,6 +1356,10 @@ static int journal_reset(journal_t *journal)
> >  	return jbd2_journal_start_thread(journal);
> >  }
> >  
> > +/*
> > + * This function expects that the caller will have locked the journal
> > + * buffer head, and will return with it unlocked
> > + */
> >  static int jbd2_write_superblock(journal_t *journal, int write_flags)
> >  {
> >  	struct buffer_head *bh = journal->j_sb_buffer;
> > @@ -1365,7 +1369,6 @@ static int jbd2_write_superblock(journal_t *journal, int write_flags)
> >  	trace_jbd2_write_superblock(journal, write_flags);
> >  	if (!(journal->j_flags & JBD2_BARRIER))
> >  		write_flags &= ~(REQ_FUA | REQ_PREFLUSH);
> > -	lock_buffer(bh);
> >  	if (buffer_write_io_error(bh)) {
> >  		/*
> >  		 * Oh, dear.  A previous attempt to write the journal
> > @@ -1424,6 +1427,7 @@ int jbd2_journal_update_sb_log_tail(journal_t *journal, tid_t tail_tid,
> >  	jbd_debug(1, "JBD2: updating superblock (start %lu, seq %u)\n",
> >  		  tail_block, tail_tid);
> >  
> > +	lock_buffer(journal->j_sb_buffer);
> >  	sb->s_sequence = cpu_to_be32(tail_tid);
> >  	sb->s_start    = cpu_to_be32(tail_block);
> >  
> > @@ -1454,18 +1458,17 @@ static void jbd2_mark_journal_empty(journal_t *journal, int write_op)
> >  	journal_superblock_t *sb = journal->j_superblock;
> >  
> >  	BUG_ON(!mutex_is_locked(&journal->j_checkpoint_mutex));
> > -	read_lock(&journal->j_state_lock);
> > -	/* Is it already empty? */
> > -	if (sb->s_start == 0) {
> > -		read_unlock(&journal->j_state_lock);
> > +	lock_buffer(journal->j_sb_buffer);
> > +	if (sb->s_start == 0) {		/* Is it already empty? */
> > +		unlock_buffer(journal->j_sb_buffer);
> >  		return;
> >  	}
> > +
> >  	jbd_debug(1, "JBD2: Marking journal as empty (seq %d)\n",
> >  		  journal->j_tail_sequence);
> >  
> >  	sb->s_sequence = cpu_to_be32(journal->j_tail_sequence);
> >  	sb->s_start    = cpu_to_be32(0);
> > -	read_unlock(&journal->j_state_lock);
> >  
> >  	jbd2_write_superblock(journal, write_op);
> >  
> > @@ -1488,9 +1491,8 @@ void jbd2_journal_update_sb_errno(journal_t *journal)
> >  	journal_superblock_t *sb = journal->j_superblock;
> >  	int errcode;
> >  
> > -	read_lock(&journal->j_state_lock);
> > +	lock_buffer(journal->j_sb_buffer);
> >  	errcode = journal->j_errno;
> > -	read_unlock(&journal->j_state_lock);
> >  	if (errcode == -ESHUTDOWN)
> >  		errcode = 0;
> >  	jbd_debug(1, "JBD2: updating superblock error (errno %d)\n", errcode);
> > 
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] jbd2: fix race when writing superblock

[Jan Kara]

On Mon 11-02-19 14:58:41, Theodore Y. Ts'o wrote:
> On Mon, Jan 28, 2019 at 04:06:34PM +0100, Jan Kara wrote:
> > 
> > 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.
> 
> Agreed.  It turns out we always write the superblock after we modify
> it, so we have to call lock_buffer() anyway; the patch just moves it
> so it happens a bit earlier.
> 
> Please take a look at this fix.  Zhangyi, can you confirm whether your
> test failures of generic/475 are addressed with this patch?
> 
> 					- Ted
> 
> From 9bb7a0025fc43bc517c5e30c638f9ca389600b15 Mon Sep 17 00:00:00 2001
> From: Theodore Ts'o <tytso@mit.edu>
> Date: Mon, 11 Feb 2019 14:52:14 -0500
> Subject: [PATCH] jbd2: fix race when writing superblock
> 
> 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)
> 
> Fix this by locking the buffer head before modifing it.  We always
> write the jbd2 superblock after we modify it, so this just means
> calling the lock_buffer() a little earlier.
> 
> This checksum corruption problem can be reproduced by xfstests
> generic/475.
> 
> Reported-by: zhangyi (F) <yi.zhang@huawei.com>
> Suggested-by: Jan Kara <jack@suse.cz>
> Signed-off-by: Theodore Ts'o <tytso@mit.edu>

Yeah, that's a good observation. The patch looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/jbd2/journal.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 88d8f22d2cba..7a38b56c2544 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -1356,6 +1356,10 @@ static int journal_reset(journal_t *journal)
>  	return jbd2_journal_start_thread(journal);
>  }
>  
> +/*
> + * This function expects that the caller will have locked the journal
> + * buffer head, and will return with it unlocked
> + */
>  static int jbd2_write_superblock(journal_t *journal, int write_flags)
>  {
>  	struct buffer_head *bh = journal->j_sb_buffer;
> @@ -1365,7 +1369,6 @@ static int jbd2_write_superblock(journal_t *journal, int write_flags)
>  	trace_jbd2_write_superblock(journal, write_flags);
>  	if (!(journal->j_flags & JBD2_BARRIER))
>  		write_flags &= ~(REQ_FUA | REQ_PREFLUSH);
> -	lock_buffer(bh);
>  	if (buffer_write_io_error(bh)) {
>  		/*
>  		 * Oh, dear.  A previous attempt to write the journal
> @@ -1424,6 +1427,7 @@ int jbd2_journal_update_sb_log_tail(journal_t *journal, tid_t tail_tid,
>  	jbd_debug(1, "JBD2: updating superblock (start %lu, seq %u)\n",
>  		  tail_block, tail_tid);
>  
> +	lock_buffer(journal->j_sb_buffer);
>  	sb->s_sequence = cpu_to_be32(tail_tid);
>  	sb->s_start    = cpu_to_be32(tail_block);
>  
> @@ -1454,18 +1458,15 @@ static void jbd2_mark_journal_empty(journal_t *journal, int write_op)
>  	journal_superblock_t *sb = journal->j_superblock;
>  
>  	BUG_ON(!mutex_is_locked(&journal->j_checkpoint_mutex));
> -	read_lock(&journal->j_state_lock);
> -	/* Is it already empty? */
> -	if (sb->s_start == 0) {
> -		read_unlock(&journal->j_state_lock);
> +	lock_buffer(journal->j_sb_buffer);
> +	if (sb->s_start == 0)	/* Is it already empty? */
>  		return;
> -	}
> +
>  	jbd_debug(1, "JBD2: Marking journal as empty (seq %d)\n",
>  		  journal->j_tail_sequence);
>  
>  	sb->s_sequence = cpu_to_be32(journal->j_tail_sequence);
>  	sb->s_start    = cpu_to_be32(0);
> -	read_unlock(&journal->j_state_lock);
>  
>  	jbd2_write_superblock(journal, write_op);
>  
> @@ -1488,9 +1489,8 @@ void jbd2_journal_update_sb_errno(journal_t *journal)
>  	journal_superblock_t *sb = journal->j_superblock;
>  	int errcode;
>  
> -	read_lock(&journal->j_state_lock);
> +	lock_buffer(journal->j_sb_buffer);
>  	errcode = journal->j_errno;
> -	read_unlock(&journal->j_state_lock);
>  	if (errcode == -ESHUTDOWN)
>  		errcode = 0;
>  	jbd_debug(1, "JBD2: updating superblock error (errno %d)\n", errcode);
> -- 
> 2.19.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR