* Re: [PATCH v3] jbd2: Fix block tag checksum verification brokenness
@ 2013-06-03 2:16 George Spelvin
2013-06-03 7:55 ` Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: George Spelvin @ 2013-06-03 2:16 UTC (permalink / raw)
To: darrick.wong; +Cc: linux, linux-fsdevel
> 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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] jbd2: Fix block tag checksum verification brokenness
2013-06-03 2:16 [PATCH v3] jbd2: Fix block tag checksum verification brokenness George Spelvin
@ 2013-06-03 7:55 ` Darrick J. Wong
2013-06-04 0:40 ` Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2013-06-03 7:55 UTC (permalink / raw)
To: George Spelvin; +Cc: linux-fsdevel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3] jbd2: Fix block tag checksum verification brokenness
2013-06-03 7:55 ` Darrick J. Wong
@ 2013-06-04 0:40 ` Darrick J. Wong
2013-06-04 3:32 ` George Spelvin
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2013-06-04 0:40 UTC (permalink / raw)
To: George Spelvin, Theodore Ts'o; +Cc: linux-fsdevel, linux-ext4
[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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] jbd2: Fix block tag checksum verification brokenness
@ 2013-05-10 23:38 Darrick J. Wong
2013-05-11 5:18 ` Andreas Dilger
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2013-05-10 23:38 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Al Viro, Andreas Dilger, linux-fsdevel@vger.kernel.org,
linux-ext4, linux-kernel
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,
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2] jbd2: Fix block tag checksum verification brokenness
2013-05-10 23:38 [PATCH v2] " Darrick J. Wong
@ 2013-05-11 5:18 ` Andreas Dilger
2013-05-13 21:47 ` [PATCH v3] " Darrick J. Wong
0 siblings, 1 reply; 6+ messages in thread
From: Andreas Dilger @ 2013-05-11 5:18 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Theodore Ts'o, Al Viro, linux-fsdevel@vger.kernel.org,
linux-ext4
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
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v3] jbd2: Fix block tag checksum verification brokenness
2013-05-11 5:18 ` Andreas Dilger
@ 2013-05-13 21:47 ` Darrick J. Wong
2013-05-28 11:34 ` Theodore Ts'o
0 siblings, 1 reply; 6+ messages in thread
From: Darrick J. Wong @ 2013-05-13 21:47 UTC (permalink / raw)
To: Theodore Ts'o
Cc: Andreas Dilger, Al Viro, linux-fsdevel@vger.kernel.org,
linux-ext4
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,
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v3] jbd2: Fix block tag checksum verification brokenness
2013-05-13 21:47 ` [PATCH v3] " Darrick J. Wong
@ 2013-05-28 11:34 ` Theodore Ts'o
0 siblings, 0 replies; 6+ messages in thread
From: Theodore Ts'o @ 2013-05-28 11:34 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Andreas Dilger, Al Viro, linux-fsdevel@vger.kernel.org,
linux-ext4
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-06-04 3:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-06-03 2:16 [PATCH v3] jbd2: Fix block tag checksum verification brokenness George Spelvin
2013-06-03 7:55 ` Darrick J. Wong
2013-06-04 0:40 ` Darrick J. Wong
2013-06-04 3:32 ` George Spelvin
-- strict thread matches above, loose matches on Subject: below --
2013-05-10 23:38 [PATCH v2] " Darrick J. Wong
2013-05-11 5:18 ` Andreas Dilger
2013-05-13 21:47 ` [PATCH v3] " Darrick J. Wong
2013-05-28 11:34 ` Theodore Ts'o
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).