Re: [PATCH 11/28] ext4: Calculate and verify inode checksums

["Darrick J. Wong" <djwong@us.ibm.com>]

On Wed, Oct 12, 2011 at 12:45:01PM -0700, Andreas Dilger wrote:
> On 2011-10-08, at 12:54 AM, Darrick J. Wong wrote:
> > This patch introduces to ext4 the ability to calculate and verify inode
> > checksums.  This requires the use of a new ro compatibility flag and some
> > accompanying e2fsprogs patches to provide the relevant features in tune2fs and
> > e2fsck.
> > 
> > +static __u32 ext4_inode_csum(struct inode *inode, struct ext4_inode *raw,
> > +			      struct ext4_inode_info *ei)
> > +{
> > +	struct ext4_sb_info *sbi = EXT4_SB(inode->i_sb);
> > +	__u16 crc_lo;
> > +	__u16 crc_hi = 0;
> > +	__u32 crc;
> > +
> > +	crc_lo = raw->i_checksum_lo;
> > +	raw->i_checksum_lo = 0;
> > +	if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE &&
> > +	    EXT4_FITS_IN_INODE(raw, ei, i_checksum_hi)) {
> > +		crc_hi = raw->i_checksum_hi;
> > +		raw->i_checksum_hi = 0;
> > +	}
> > +
> > +	crc = ext4_chksum(sbi, ei->i_uuid_inum_crc, (__u8 *)raw,
> > +			  EXT4_INODE_SIZE(inode->i_sb));
> > +
> > +	raw->i_checksum_lo = crc_lo;
> > +	if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE &&
> > +	    EXT4_FITS_IN_INODE(raw, ei, i_checksum_hi))
> > +		raw->i_checksum_hi = crc_hi;
> > +
> > +	return crc;
> > +}
> 
> This computes both the _lo and _hi parts of the checksum and overwrites what
> is in the inode...

I don't follow your logic ... for the _lo component, first I save the old
i_checksum_lo contents in crc_lo.  Then I stuff zero into i_checksum_lo.  Next
I perform the checksum computation (with the checksum field effectively "zero")
and put the results into crc.  Then I copy whatever I saved in crc_lo back into
i_checksum_lo.

crc_lo, crc_hi, and crc are three separate variables, and neither crc_lo nor
crc_hi are ever assigned any part of crc.  Therefore crc_lo and crc_hi should
always contain the old checksum contents.

Did I miss something?  Afaict the contents of raw should be the same before and
after the call to ext4_inode_csum(), but maybe I've been looking at this too
long. :)

> > +static int ext4_inode_csum_verify(struct inode *inode, struct ext4_inode *raw,
> > +				  struct ext4_inode_info *ei)
> > +{
> > +	__u32 provided, calculated;
> > +
> > +	if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
> > +	    cpu_to_le32(EXT4_OS_LINUX) ||
> > +	    !EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> > +		EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
> > +		return 1;
> > +
> > +	provided = le16_to_cpu(raw->i_checksum_lo);
> > +	calculated = ext4_inode_csum(inode, raw, ei);
> 
> This only saves the _lo part of the checksum before computing the new
> checksum (which overwrites both _lo and _hi fields), so the _hi part
> of the checksum is never properly validated below.
> 
> > +	if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE &&
> > +	    EXT4_FITS_IN_INODE(raw, ei, i_checksum_hi))
> > +		provided |= ((__u32)le16_to_cpu(raw->i_checksum_hi)) << 16;
> 
> This should be moved up to save _hi before calling ext4_inode_csum().
> 
> > +	else
> > +		calculated &= 0xFFFF;
> > +
> > +	return provided == calculated;
> > +}
> 
> > +static void ext4_inode_csum_set(struct inode *inode, struct ext4_inode *raw,
> > +				struct ext4_inode_info *ei)
> > +{
> > +	__u32 crc;
> > +
> > +	if (EXT4_SB(inode->i_sb)->s_es->s_creator_os !=
> > +	    cpu_to_le32(EXT4_OS_LINUX) ||
> > +	    !EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb,
> > +		EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
> > +		return;
> > +
> > +	crc = ext4_inode_csum(inode, raw, ei);
> > +	raw->i_checksum_lo = cpu_to_le16(crc & 0xFFFF);
> > +	if (EXT4_INODE_SIZE(inode->i_sb) > EXT4_GOOD_OLD_INODE_SIZE &&
> > +	    EXT4_FITS_IN_INODE(raw, ei, i_checksum_hi))
> > +		raw->i_checksum_hi = cpu_to_le16(crc >> 16);
> 
> What is the point of storing the returned crc into raw->i_checksum_lo
> and raw->i_checksum_hi, if this is done internal to ext4_inode_csum()
> already?

It shouldn't be doing that (see above).

> Also, would it be better to call the temporary variable "csum" instead
> of "crc", since we may use something other than crc32c as the hash
> function in the future.

I suppose.

--D