linux-ext4 - Re: journal recovery problems with metadata

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>] [day] [month] [year] [list]

Date:	Sun, 10 Aug 2014 18:35:33 -0400
From:	TR Reardon <thomas_reardon@...mail.com>
To:	"Theodore Ts'o" <tytso@....edu>,
	"Darrick J. Wong" <darrick.wong@...cle.com>
CC:	linux-ext4@...r.kernel.org
Subject: Re: journal recovery problems with metadata_csum, *non-64bit*

Ok, I found the problem in jbd2, and have a solution, though it's
debatable what the ideal solution is.  For now, the simplest patch is
below, though a similar patch in lib/ext2fs/kernel-jbd.h is required
to get e2fsck back in sync.

The original c3900875 commit adding metadata_csum (ie
journal_checksum_v2) to jbd2 added 2 extra bytes for the block
checksums, in addition to re-allocating 2 bytes from the 4 bytes of
flags.  However, a decision was made to only retain the lower 16-bits
of the crc32c, and thus those extra 2 bytes were unneeded.  But those
2 extra bytes were never "deallocated" from journal_tag_bytes().

Unfortunately, different code relies on JBD_TAG_SIZE32/64 constants
directly rather than the journal_tag_bytes() utility function, in
particular the recovery code which is common to e2fsck and jbd2.  This
led different tools to think they were looking at a 64bit journal when
actually it was 32bit.  Code that relied on journal_tag_bytes()
remained safe, so the block iterators were fine, but any direct use of
those constants [including the hideous greater-than comparison in
read_tag_bytes()] went awry, and journal replay will fail.

As far as I can tell, metadata_csum + journal checksum has never
worked for 32bit filesystems. By a little bit of padding luck, 64bit
worked fine.

Now, as to the solution: depends on whether one feels that existing
in-the-wild journals matter. The original commit was May 2012, are we
past early-adopters now?  If this patch is taken, you shrink the
journal block tags to the intended size but in-the-wild journals will
be broken.  But they already are, so...?  This opens up the
possibility of now using those extra 2 bytes and retaining full 32-bit
crc32c for the block tags.  If going that route, debugs/logdump needs
a fix in addition to changes to jbd2.

FWIW, the "JBD2: Out of memory during recovery." error in
fs/jbd2/recovery.c was opaque at best and should be changed to always
include the block# that caused the problem.

+Reardon

---
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 67b8e30..dc27d09 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2166,15 +2166,11 @@ int jbd2_journal_blocks_per_page(struct inode *inode)
 size_t journal_tag_bytes(journal_t *journal)
 {
        journal_block_tag_t tag;
-       size_t x = 0;
-
-       if (JBD2_HAS_INCOMPAT_FEATURE(journal, JBD2_FEATURE_INCOMPAT_CSUM_V2))
-               x += sizeof(tag.t_checksum);

        if (JBD2_HAS_INCOMPAT_FEATURE(journal, JBD2_FEATURE_INCOMPAT_64BIT))
-               return x + JBD2_TAG_SIZE64;
+               return JBD2_TAG_SIZE64;
        else
-               return x + JBD2_TAG_SIZE32;
+               return JBD2_TAG_SIZE32;
 }

 /*
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html