linux-ext4 - Re: [PATCH 1/2] libext2fs: add metadata checksum and snapshot feature flags

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <14955E98-C987-40D6-A881-5D40077C2FB2@dilger.ca>
Date:	Thu, 15 Sep 2011 17:34:41 -0600
From:	Andreas Dilger <adilger@...ger.ca>
To:	Ted Ts'o <tytso@....edu>
Cc:	Ext4 Developers List <linux-ext4@...r.kernel.org>,
	"Darrick J. Wong" <djwong@...ibm.com>,
	Amir Goldstein <amir73il@...il.com>
Subject: Re: [PATCH 1/2] libext2fs: add metadata checksum and snapshot feature flags

On 2011-09-15, at 5:11 PM, Ted Ts'o wrote:
> On Thu, Sep 15, 2011 at 05:09:13PM -0600, Andreas Dilger wrote:
>> 
>> I thought it would be better to move s_checksum to be the last field in the
>> superblock to avoid multiple calls to the CRC function? 
> 
> Did you see my comment about just zero'ing the checksum field before
> running the CRC?  We're going to have to do that for other data
> structures, such as the inode structure, and it's what we do with the
> block group descriptor checksum.

That isn't correct.  The group descriptor checksum is computed in chunks:

__le16 ext4_group_desc_csum(struct ext4_sb_info *sbi, __u32 block_group,
                            struct ext4_group_desc *gdp)
{
                int offset = offsetof(struct ext4_group_desc, bg_checksum);
                __le32 le_group = cpu_to_le32(block_group);

                crc = crc16(~0, sbi->s_es->s_uuid, sizeof(sbi->s_es->s_uuid));
                crc = crc16(crc, (__u8 *)&le_group, sizeof(le_group));
                crc = crc16(crc, (__u8 *)gdp, offset);
                offset += sizeof(gdp->bg_checksum); /* skip checksum */
****HERE****
                /* for checksum of struct ext4_group_desc do the rest...*/
                if ((sbi->s_es->s_feature_incompat &
                     cpu_to_le32(EXT4_FEATURE_INCOMPAT_64BIT)) &&
                    offset < le16_to_cpu(sbi->s_es->s_desc_size))
                        crc = crc16(crc, (__u8 *)gdp + offset,
                                    le16_to_cpu(sbi->s_es->s_desc_size) -
                                        offset);
 }

Darrick and I discussed zeroing the checksum fields, but then there is a
race with other threads accessing the same structure.

If we went to a crc32c LSB for filesystems with RO_COMPAT_CSUM it would
be possible to change how it is computed.  Since we have freedom to move
the checksum field now, why have the added complexity to do zeroing of
the field or two chunks?

Since we naturally have to break the checksum calculation for 128-byte
inodes and 32-byte descriptors, due to old versions of those structs,
there is little overhead in just skipping the field, and no races.

Cheers, Andreas





--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html