linux-kernel - Re: [PATCH 03/22] ext4: Record the checksum algorithm in use in the superblock

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>] [day] [month] [year] [list]

Message-ID: <20111207074257.GJ7137@tux1.beaverton.ibm.com>
Date:	Tue, 6 Dec 2011 23:42:57 -0800
From:	"Darrick J. Wong" <djwong@...ibm.com>
To:	Andreas Dilger <adilger.kernel@...ger.ca>
Cc:	Theodore Tso <tytso@....edu>,
	Sunil Mushran <sunil.mushran@...cle.com>,
	Martin K Petersen <martin.petersen@...cle.com>,
	Greg Freemyer <greg.freemyer@...il.com>,
	Amir Goldstein <amir73il@...il.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	Andi Kleen <andi@...stfloor.org>,
	Mingming Cao <cmm@...ibm.com>,
	Joel Becker <jlbec@...lplan.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	linux-ext4@...r.kernel.org, Coly Li <colyli@...il.com>
Subject: Re: [PATCH 03/22] ext4: Record the checksum algorithm in use in
	the superblock

On Tue, Dec 06, 2011 at 10:01:35PM -0700, Andreas Dilger wrote:
> On 2011-11-28, at 4:26 PM, Darrick J. Wong wrote:
> > Record the type of checksum algorithm we're using for metadata in the
> > superblock, in case we ever want/need to change the algorithm.
> > 
> > @@ -982,6 +982,9 @@ extern void ext4_set_bits(void *bm, int cur, int len);
> > +/* Metadata checksum algorithm codes */
> > +#define EXT4_CRC32C_CHKSUM		1
> 
> It might make sense to add a constant for the existing CRC16 type:
> 
> #define EXT4_CRC16_CHKSUM    0
> #define EXT4_CRC32C_CHKSUM   1
> #define EXT4_CHKSUM_MAX      1

I disagree -- this field is only defined if metadata_csum is set, and most of
the checksumming code doesn't know what crc16 is.  If a user somehow set the
checksum type to "crc16", the code would keep using crc32c.  

If we ever extend ext4_chksum() to support other algorithms I suppose we could
make it smart enough to switch algorithms based on the field setting, but I
don't really want to do for crc16. :)

> > +static int ext4_verify_csum_type(struct super_block *sb,
> > +				 struct ext4_super_block *es)
> > +{
> > +	if (!EXT4_HAS_RO_COMPAT_FEATURE(sb,
> > +					EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
> > +		return 1;
> > +
> > +	return es->s_checksum_type == EXT4_CRC32C_CHKSUM;
> > +}
> 
> Then this checks "return es->s_checksum_type <= EXT4_CHKSUM_MAX;".

Not really -- this would allow us to mount a file system with metadata_csum=1
and csum_type=crc16, and crc16 is not supported by most of the checksumming
code.  I'd rather have the code reject the mount and send fs in for fsck, since
crc32c is the only one it knows about.

> > @@ -3182,6 +3192,14 @@ static int ext4_fill_super(struct super_block *sb, 
> > +	/* Check for a known checksum algorithm */
> > +	if (!ext4_verify_csum_type(sb, es)) {
> > +		ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
> > +			 "unknown checksum algorithm.");
> 
> It would be useful if this error message printed the checksum type.

Agreed.

--D
> 
> Cheers, Andreas
> 
> 
> 
> 
> 

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