Re: [RFC] mess in jbd2_block_tag_csum_verify()

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, May 08, 2013 at 10:04:31AM -0600, Andreas Dilger wrote:
> On 2013-05-08, at 9:51, Al Viro <viro@ZenIV.linux.org.uk> wrote:
> 
> > You have
> >    calculated = jbd2_chksum(j, calculated, buf, j->j_blocksize);
> >    provided = be32_to_cpu(tag->t_checksum);
> > 
> >    return provided == cpu_to_be32(calculated);
> > 
> > in there, which makes no sense whatsoever.  First of all, you are
> > converting big-endian to native, then another native to big-endian
> > and compare results.  The bogosity aside, it's equivalent to simply
> > comparing tag->t_checksum with calculated - cpu_to_be32() is the
> > same mapping as be32_to_cpu() on all architectures and it's a one-to-one
> > mapping, at that.
> 
> I agree this is a bit of extra swabbing that isn't needed. 
> 
> > Bogosity, of course, is that tag->t_checksum is apparently big-endian
> > and definitely a 16bit value.  How in hell is that check going to
> > yield true?  Note that you are asking for 16 bits out of crc32c result
> > to be zero, _NOT_ to be ignored.

Yes, that is effing awful.  It looks like I missed that detail at some point,
probably when tweaking the endian handling or something.

> I think you're mixing up the jbd2 transaction block checksum, which actually
> has up to 128 bits of space (in case we want to move to a better checksum in
> the future) with the ext4 group descriptor checksum (which is only 16 bits
> for compatibility reasons). 

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.

--D
> 
> Cheers, Andreas
> 
> > Producer of that value shoves lower 16 bits of cpu_to_be32(crc) into the
> > on-disk structure.  Also a bloody bad idea, since the values on little-endian
> > and big-endian hosts will be different; move the disk from one box to
> > another and watch the mismatches...
> > 
> > What the hell is going on there?
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html