[Devel] jbd2: fix FS corruption possibility in jbd2_journal_destroy() on umount path

Submitted by Anatoly Stepanov on Dec. 20, 2016, 11:39 a.m.


Message ID 1482233955-171663-3-git-send-email-astepanov@cloudlinux.com
State New
Series "jbd2: fix incorrect unlock on j_list_lock"
Headers show

Commit Message

Anatoly Stepanov Dec. 20, 2016, 11:39 a.m.
From: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

On umount path, jbd2_journal_destroy() writes latest transaction ID
(->j_tail_sequence) to be used at next mount.

The bug is that ->j_tail_sequence is not holding latest transaction ID
in some cases. So, at next mount, there is chance to conflict with
remaining (not overwritten yet) transactions.

	mount (id=10)
	write transaction (id=11)
	write transaction (id=12)
	umount (id=10) <= the bug doesn't write latest ID

	mount (id=10)
	write transaction (id=11)

	[recovery process]
		transaction (id=11)
		transaction (id=12) <= valid transaction ID, but old commit
                                       must not replay

Like above, this bug become the cause of recovery failure, or FS

So why ->j_tail_sequence doesn't point latest ID?

Because if checkpoint transactions was reclaimed by memory pressure
(i.e. bdev_try_to_free_page()), then ->j_tail_sequence is not updated.
(And another case is, __jbd2_journal_clean_checkpoint_list() is called
with empty transaction.)

So in above cases, ->j_tail_sequence is not pointing latest
transaction ID at umount path. Plus, REQ_FLUSH for checkpoint is not
done too.

So, to fix this problem with minimum changes, this patch updates
->j_tail_sequence, and issue REQ_FLUSH.  (With more complex changes,
some optimizations would be possible to avoid unnecessary REQ_FLUSH
for example though.)


	journal->j_tail_sequence =

Increment of ->j_transaction_sequence seems to be unnecessary, but
ext3 does this.

Signed-off-by: OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>
Signed-off-by: Theodore Ts'o <tytso@mit.edu>
Cc: stable@vger.kernel.org
 fs/jbd2/journal.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Patch hide | download patch | download mbox

diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 7bf1683..de73a95 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -1430,11 +1430,12 @@  out:
  * jbd2_mark_journal_empty() - Mark on disk journal as empty.
  * @journal: The journal to update.
+ * @write_op: With which operation should we write the journal sb
  * Update a journal's dynamic superblock fields to show that journal is empty.
  * Write updated superblock to disk waiting for IO to complete.
-static void jbd2_mark_journal_empty(journal_t *journal)
+static void jbd2_mark_journal_empty(journal_t *journal, int write_op)
 	journal_superblock_t *sb = journal->j_superblock;
@@ -1452,7 +1453,7 @@  static void jbd2_mark_journal_empty(journal_t *journal)
 	sb->s_start    = cpu_to_be32(0);
-	jbd2_write_superblock(journal, WRITE_FUA);
+	jbd2_write_superblock(journal, write_op);
 	/* Log is no longer empty */
@@ -1738,7 +1739,13 @@  int jbd2_journal_destroy(journal_t *journal)
 	if (journal->j_sb_buffer) {
 		if (!is_journal_aborted(journal)) {
-			jbd2_mark_journal_empty(journal);
+			write_lock(&journal->j_state_lock);
+			journal->j_tail_sequence =
+				++journal->j_transaction_sequence;
+			write_unlock(&journal->j_state_lock);
+			jbd2_mark_journal_empty(journal, WRITE_FLUSH_FUA);
 		} else
 			err = -EIO;
@@ -1997,7 +2004,7 @@  int jbd2_journal_flush(journal_t *journal)
 	 * the magic code for a fully-recovered superblock.  Any future
 	 * commits of data to the journal will restore the current
 	 * s_start value. */
-	jbd2_mark_journal_empty(journal);
+	jbd2_mark_journal_empty(journal, WRITE_FUA);
@@ -2043,7 +2050,7 @@  int jbd2_journal_wipe(journal_t *journal, int write)
 	if (write) {
 		/* Lock to make assertions happy... */
-		jbd2_mark_journal_empty(journal);
+		jbd2_mark_journal_empty(journal, WRITE_FUA);