On 2013-05-08, at 10:04, Andreas Dilger <adilger@xxxxxxxxx> wrote: > On 2013-05-08, at 9:51, Al Viro <viro@xxxxxxxxxxxxxxxxxx> 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. > > 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). > > Problem solved? Doh, my bad. You are looking at t_checksum and not h_chksum. The t_checksum is definitely a 16-bit value, and I totally agree this code seems bogus. be32_to_cpu() on a 16-bit value is bad, as is the lack of truncation to 16 bits for comparison. I'm not sure how this code could work on either BE or LE systems. Thanks for finding this. 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@xxxxxxxxxxxxxxx >> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
