From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Darrick J. Wong" Subject: Re: [RFC] mess in jbd2_block_tag_csum_verify() Date: Wed, 8 May 2013 09:45:08 -0700 Message-ID: <20130508164508.GA5625@blackbox.djwong.org> References: <20130508155130.GT25399@ZenIV.linux.org.uk> <41D3CA9A-6284-4E5A-97AD-4D11E2307B01@dilger.ca> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Al Viro , "Darrick J. Wong" , "Theodore Ts'o" , "linux-fsdevel@vger.kernel.org" To: Andreas Dilger Return-path: Received: from userp1040.oracle.com ([156.151.31.81]:40676 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757410Ab3EHQpQ (ORCPT ); Wed, 8 May 2013 12:45:16 -0400 Content-Disposition: inline In-Reply-To: <41D3CA9A-6284-4E5A-97AD-4D11E2307B01@dilger.ca> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: On Wed, May 08, 2013 at 10:04:31AM -0600, Andreas Dilger wrote: > On 2013-05-08, at 9:51, Al Viro 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