[PATCH v3] jbd2: Fix block tag checksum verification brokenness

[PATCH v3] jbd2: Fix block tag checksum verification brokenness

[Darrick J. Wong]

Al Viro complained of a ton of bogosity with regards to the jbd2 block tag
header checksum.  This one checksum is 16 bits, so cut off the upper 16 bits
and treat it as a 16-bit value and don't mess around with be32* conversions.
Fortunately metadata checksumming is still "experimental" and not in a shipping
e2fsprogs, so there should be few users affected by this.

v2: Eliminate unnecessary variables and make it clear(er) which values are 32
bits wide.

v3: Add a comment about why we're only storing half of the crc32c.

Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/jbd2/commit.c   |   13 +++++++------
 fs/jbd2/recovery.c |   11 +++++------
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 0f53946..e61d722 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -343,20 +343,21 @@ static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
 {
 	struct page *page = bh->b_page;
 	__u8 *addr;
-	__u32 csum;
+	__u32 csum32;
 
 	if (!JBD2_HAS_INCOMPAT_FEATURE(j, JBD2_FEATURE_INCOMPAT_CSUM_V2))
 		return;
 
 	sequence = cpu_to_be32(sequence);
 	addr = kmap_atomic(page);
-	csum = jbd2_chksum(j, j->j_csum_seed, (__u8 *)&sequence,
-			  sizeof(sequence));
-	csum = jbd2_chksum(j, csum, addr + offset_in_page(bh->b_data),
-			  bh->b_size);
+	csum32 = jbd2_chksum(j, j->j_csum_seed, (__u8 *)&sequence,
+			     sizeof(sequence));
+	csum32 = jbd2_chksum(j, csum32, addr + offset_in_page(bh->b_data),
+			     bh->b_size);
 	kunmap_atomic(addr);
 
-	tag->t_checksum = cpu_to_be32(csum);
+	/* We only have space to store the lower 16 bits of the crc32c. */
+	tag->t_checksum = cpu_to_be16(csum32);
 }
 /*
  * jbd2_journal_commit_transaction
diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index 626846b..d4851464 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -399,18 +399,17 @@ static int jbd2_commit_block_csum_verify(journal_t *j, void *buf)
 static int jbd2_block_tag_csum_verify(journal_t *j, journal_block_tag_t *tag,
 				      void *buf, __u32 sequence)
 {
-	__u32 provided, calculated;
+	__u32 csum32;
 
 	if (!JBD2_HAS_INCOMPAT_FEATURE(j, JBD2_FEATURE_INCOMPAT_CSUM_V2))
 		return 1;
 
 	sequence = cpu_to_be32(sequence);
-	calculated = jbd2_chksum(j, j->j_csum_seed, (__u8 *)&sequence,
-				 sizeof(sequence));
-	calculated = jbd2_chksum(j, calculated, buf, j->j_blocksize);
-	provided = be32_to_cpu(tag->t_checksum);
+	csum32 = jbd2_chksum(j, j->j_csum_seed, (__u8 *)&sequence,
+			     sizeof(sequence));
+	csum32 = jbd2_chksum(j, csum32, buf, j->j_blocksize);
 
-	return provided == cpu_to_be32(calculated);
+	return tag->t_checksum == cpu_to_be16(csum32);
 }
 
 static int do_one_pass(journal_t *journal,

Re: [PATCH v3] jbd2: Fix block tag checksum verification brokenness

[Theodore Ts'o]

On Mon, May 13, 2013 at 02:47:50PM -0700, Darrick J. Wong wrote:
> Al Viro complained of a ton of bogosity with regards to the jbd2 block tag
> header checksum.  This one checksum is 16 bits, so cut off the upper 16 bits
> and treat it as a 16-bit value and don't mess around with be32* conversions.
> Fortunately metadata checksumming is still "experimental" and not in a shipping
> e2fsprogs, so there should be few users affected by this.
> 
> v2: Eliminate unnecessary variables and make it clear(er) which values are 32
> bits wide.
> 
> v3: Add a comment about why we're only storing half of the crc32c.
> 
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks, applied.

					- Ted

Re: [PATCH v3] jbd2: Fix block tag checksum verification brokenness

[George Spelvin]

> Fortunately metadata checksumming is still "experimental" and not in a shipping
> e2fsprogs, so there should be few users affected by this.

Am I reading this patch correctly that this changes which half of
the 32-bit checksum is stored on little-endian (e.g. x86) machines?
Before, it stored the low 16 bits of cpu_to_be32(csum), meaning the
high 16 bits of csum.

Now, it's cpu_to_be16(csum32), which is the low 16 bits of csum32.

It so happens that I have multiple metadata_csum file systems.
(I enabled it a while ago on a machine where I wasn't sure if corruption
was RAM or disk, and have been using it on SSE4.2 machines since.)

Is there an upgrade path?  Also, what's the corresponding e2fsprogs
commit that supports this change?

Re: [PATCH v3] jbd2: Fix block tag checksum verification brokenness

[Darrick J. Wong]

On Sun, Jun 02, 2013 at 10:16:30PM -0400, George Spelvin wrote:
> > Fortunately metadata checksumming is still "experimental" and not in a shipping
> > e2fsprogs, so there should be few users affected by this.
> 
> Am I reading this patch correctly that this changes which half of
> the 32-bit checksum is stored on little-endian (e.g. x86) machines?
> Before, it stored the low 16 bits of cpu_to_be32(csum), meaning the
> high 16 bits of csum.

...unless you're using a BE system in which case it's the low 16 bits. :)

> Now, it's cpu_to_be16(csum32), which is the low 16 bits of csum32.
> 
> It so happens that I have multiple metadata_csum file systems.
> (I enabled it a while ago on a machine where I wasn't sure if corruption
> was RAM or disk, and have been using it on SSE4.2 machines since.)
> 
> Is there an upgrade path?  Also, what's the corresponding e2fsprogs
> commit that supports this change?

The kernel patch fixes journal bogosity when moving a disk between big and
little endian systems.  e2fsprogs didn't have the brokenness, so there's no
change needed.  In theory you'd only hit this if you happened to crash an x86
box with an ext4 fs, move the disk to a ppc, and try to recover it there.

The upgrade path is to umount cleanly and reboot with a patched kernel while
hoping that you don't crash while rebooting.  Sorry about the bumpy
metadata_csum ride.

--D

Re: [PATCH v3] jbd2: Fix block tag checksum verification brokenness

[Darrick J. Wong]

[adding tytso and linux-ext4 to cc]

On Mon, Jun 03, 2013 at 12:55:21AM -0700, Darrick J. Wong wrote:
> On Sun, Jun 02, 2013 at 10:16:30PM -0400, George Spelvin wrote:
> > > Fortunately metadata checksumming is still "experimental" and not in a shipping
> > > e2fsprogs, so there should be few users affected by this.
> > 
> > Am I reading this patch correctly that this changes which half of
> > the 32-bit checksum is stored on little-endian (e.g. x86) machines?
> > Before, it stored the low 16 bits of cpu_to_be32(csum), meaning the
> > high 16 bits of csum.
> 
> ...unless you're using a BE system in which case it's the low 16 bits. :)
> 
> > Now, it's cpu_to_be16(csum32), which is the low 16 bits of csum32.
> > 
> > It so happens that I have multiple metadata_csum file systems.
> > (I enabled it a while ago on a machine where I wasn't sure if corruption
> > was RAM or disk, and have been using it on SSE4.2 machines since.)
> > 
> > Is there an upgrade path?  Also, what's the corresponding e2fsprogs
> > commit that supports this change?
> 
> The kernel patch fixes journal bogosity when moving a disk between big and
> little endian systems.  e2fsprogs didn't have the brokenness, so there's no

Drat, I was writing this email too late at night.  There /is/ a related
e2fsprogs patch for this:
http://comments.gmane.org/gmane.comp.file-systems.ext4/38621

Hey Ted, any thoughts on this jbd2/e2fsprogs patch pair?  Al acked the jbd2
part.

--D

> change needed.  In theory you'd only hit this if you happened to crash an x86
> box with an ext4 fs, move the disk to a ppc, and try to recover it there.
> 
> The upgrade path is to umount cleanly and reboot with a patched kernel while
> hoping that you don't crash while rebooting.  Sorry about the bumpy
> metadata_csum ride.
> 
> --D

Re: [PATCH v3] jbd2: Fix block tag checksum verification brokenness

[George Spelvin]

On Mon, Jun 03, 2013 at 12:55:21AM -0700, Darrick J. Wong wrote:
> Drat, I was writing this email too late at night.  There /is/ a related
> e2fsprogs patch for this:
> http://comments.gmane.org/gmane.comp.file-systems.ext4/38621

> Hey Ted, any thoughts on this jbd2/e2fsprogs patch pair?  Al acked the jbd2
> part.

He's already pulled it into ext4/dev (eee06c56).  I've been watching
it because an earlier version (which got merged into 3.10-rc1) has
an essential fix to update extent block checksums properly during
defragmentation.

I just have to be careful not to pull the *current* ext4/dev or I'll
break things.  :-(