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

[PATCH v2] 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.

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

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 750c701..67a8a2f 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -343,20 +343,20 @@ 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);
+	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 v2] jbd2: Fix block tag checksum verification brokenness

[Al Viro]

On Fri, May 10, 2013 at 04:38:07PM -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.
> 
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

ACKed-by: Al Viro <viro@ZenIV.linux.org.uk>

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

[Andreas Dilger]

On 2013-05-10, at 17:38, "Darrick J. Wong" <darrick.wong@oracle.com> wrote:
> static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
> {
> 
> +    tag->t_checksum = cpu_to_be16(csum32);
> }

It wouldn't be a terrible idea to add a comment in these spots, since
it is a bit non-obvious to use cpu_to_be16() on a 32-bit variable. 

Cheers, Andreas

[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

[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.  :-(