[add ext4 list to cc]
On Wed, May 08, 2013 at 06:07:52PM +0100, Al Viro wrote:
> On Wed, May 08, 2013 at 09:45:08AM -0700, Darrick J. Wong wrote:
>
> > The journal block tag checksum is 16 bits long.
> >
> > > Problem solved?
> >
> > I wish. Now we know what I'll be patching today...
> >
> > Anyhow, thank you for catching this.
>
> make C=2 fs/jbd2/ is a good idea (with recent enough sparse); to get rid of
> false positives re endianness, apply the patch below - that'll reduce the
> warnings to
> fs/jbd2/commit.c:358:25: warning: incorrect type in assignment (different base types)
> fs/jbd2/commit.c:358:25: expected restricted __be16 [usertype] t_checksum
> fs/jbd2/commit.c:358:25: got restricted __be32 [usertype] <noident>
> fs/jbd2/recovery.c:411:20: warning: cast to restricted __be32
> fs/jbd2/recovery.c:411:20: warning: cast from restricted __be16
> fs/jbd2/recovery.c:411:20: warning: cast to restricted __be32
> fs/jbd2/recovery.c:411:20: warning: cast from restricted __be16
> fs/jbd2/recovery.c:411:20: warning: cast to restricted __be32
> fs/jbd2/recovery.c:411:20: warning: cast from restricted __be16
> fs/jbd2/recovery.c:411:20: warning: cast to restricted __be32
> fs/jbd2/recovery.c:411:20: warning: cast from restricted __be16
> fs/jbd2/recovery.c:411:20: warning: cast to restricted __be32
> fs/jbd2/recovery.c:411:20: warning: cast from restricted __be16
> fs/jbd2/recovery.c:411:20: warning: cast to restricted __be32
> fs/jbd2/recovery.c:411:20: warning: cast from restricted __be16
> fs/jbd2/recovery.c:411:18: warning: incorrect type in assignment (different base types)
> fs/jbd2/recovery.c:411:18: expected restricted __be32 [usertype] provided
> fs/jbd2/recovery.c:411:18: got unsigned int
Interesting... I have something that calls itself sparse 0.4.3 and I don't see
any endianness warnings at all. Is endian checking new for 0.4.4? Or maybe I
simply don't have it configured correctly?
> which is where that problem lives (writing and checking tag->t_checksum resp.)
> FWIW, I think that the contents of that field in all existing fs images should
> be treated as junk - current kernels will puke if they happen to try and
> check them anyway. Perhaps the right fix would be to store cpu_to_be16(csum)
> on the write side and do return cpu_to_be16(calculated) == tag->t_checksum
> on the check side - that would be independent from host endianness, as well
> as ignoring the right bits...
>
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index 0f53946..f75fbd7 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -339,7 +339,7 @@ static void jbd2_descr_block_csum_set(journal_t *j,
> }
>
> static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
> - struct buffer_head *bh, __u32 sequence)
> + struct buffer_head *bh, __be32 sequence)
Hmm, I thought "__u" implied native endian. Any reason why we need to force
the parameter to "__be"? I'd rather leave the call sites alone, but I don't
have any strong opinions either way.
Anyway, patches to fix the kernel and e2fsprogs are on their way.
--D
> {
> struct page *page = bh->b_page;
> __u8 *addr;
> @@ -348,7 +348,6 @@ static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
> 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));
> @@ -695,7 +694,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
> write_tag_block(tag_bytes, tag, jh2bh(jh)->b_blocknr);
> tag->t_flags = cpu_to_be16(tag_flag);
> jbd2_block_tag_csum_set(journal, tag, jh2bh(new_jh),
> - commit_transaction->t_tid);
> + cpu_to_be32(commit_transaction->t_tid));
> tagp += tag_bytes;
> space_left -= tag_bytes;
>
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 626846b..cbbea8c 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -178,7 +178,8 @@ static int jbd2_descr_block_csum_verify(journal_t *j,
> void *buf)
> {
> struct jbd2_journal_block_tail *tail;
> - __u32 provided, calculated;
> + __be32 provided;
> + __u32 calculated;
>
> if (!JBD2_HAS_INCOMPAT_FEATURE(j, JBD2_FEATURE_INCOMPAT_CSUM_V2))
> return 1;
> @@ -190,8 +191,7 @@ static int jbd2_descr_block_csum_verify(journal_t *j,
> calculated = jbd2_chksum(j, j->j_csum_seed, buf, j->j_blocksize);
> tail->t_checksum = provided;
>
> - provided = be32_to_cpu(provided);
> - return provided == calculated;
> + return provided == cpu_to_be32(calculated);
> }
>
> /*
> @@ -381,7 +381,8 @@ static int calc_chksums(journal_t *journal, struct buffer_head *bh,
> static int jbd2_commit_block_csum_verify(journal_t *j, void *buf)
> {
> struct commit_header *h;
> - __u32 provided, calculated;
> + __be32 provided;
> + __u32 calculated;
>
> if (!JBD2_HAS_INCOMPAT_FEATURE(j, JBD2_FEATURE_INCOMPAT_CSUM_V2))
> return 1;
> @@ -392,19 +393,18 @@ static int jbd2_commit_block_csum_verify(journal_t *j, void *buf)
> calculated = jbd2_chksum(j, j->j_csum_seed, buf, j->j_blocksize);
> h->h_chksum[0] = provided;
>
> - provided = be32_to_cpu(provided);
> - return provided == calculated;
> + return provided == cpu_to_be32(calculated);
> }
>
> static int jbd2_block_tag_csum_verify(journal_t *j, journal_block_tag_t *tag,
> - void *buf, __u32 sequence)
> + void *buf, __be32 sequence)
> {
> - __u32 provided, calculated;
> + __be32 provided;
> + __u32 calculated;
>
> 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);
> @@ -592,7 +592,7 @@ static int do_one_pass(journal_t *journal,
> /* Look for block corruption */
> if (!jbd2_block_tag_csum_verify(
> journal, tag, obh->b_data,
> - be32_to_cpu(tmp->h_sequence))) {
> + tmp->h_sequence)) {
> brelse(obh);
> success = -EIO;
> printk(KERN_ERR "JBD: Invalid "
> @@ -809,7 +809,8 @@ static int jbd2_revoke_block_csum_verify(journal_t *j,
> void *buf)
> {
> struct jbd2_journal_revoke_tail *tail;
> - __u32 provided, calculated;
> + __be32 provided;
> + __u32 calculated;
>
> if (!JBD2_HAS_INCOMPAT_FEATURE(j, JBD2_FEATURE_INCOMPAT_CSUM_V2))
> return 1;
> @@ -821,8 +822,7 @@ static int jbd2_revoke_block_csum_verify(journal_t *j,
> calculated = jbd2_chksum(j, j->j_csum_seed, buf, j->j_blocksize);
> tail->r_checksum = provided;
>
> - provided = be32_to_cpu(provided);
> - return provided == calculated;
> + return provided == cpu_to_be32(calculated);
> }
>
> /* Scan a revoke record, marking all blocks mentioned as revoked. */
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
