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:

quoted

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. :)

quoted

+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.

quoted

+	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().

quoted

+	else
+		calculated &= 0xFFFF;
+
+	return provided == calculated;
+}

quoted

+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