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: <20110922191840.GM12086@tux1.beaverton.ibm.com>
Date:	Thu, 22 Sep 2011 12:18:40 -0700
From:	"Darrick J. Wong" <djwong@...ibm.com>
To:	Amir Goldstein <amir73il@...il.com>
Cc:	"Theodore Ts'o" <tytso@....edu>,
	Ext4 Developers List <linux-ext4@...r.kernel.org>
Subject: Re: [PATCH 1/2] libext2fs: add metadata checksum and snapshot
	feature flags

On Thu, Sep 22, 2011 at 05:12:32AM +0300, Amir Goldstein wrote:
> On Wed, Sep 21, 2011 at 11:40 PM, Darrick J. Wong <djwong@...ibm.com> wrote:
> > On Wed, Sep 21, 2011 at 11:03:57AM +0300, Amir Goldstein wrote:
> >> On Tue, Sep 20, 2011 at 9:56 PM, Darrick J. Wong <djwong@...ibm.com> wrote:
> >> > On Thu, Sep 15, 2011 at 06:50:51PM -0400, Theodore Ts'o wrote:
> > <snip>
> >> >> diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
> >> >> index 4fec5db..1c86cb2 100644
> >> >> --- a/lib/ext2fs/ext2_fs.h
> >> >> +++ b/lib/ext2fs/ext2_fs.h
> >> >> @@ -142,7 +142,9 @@ struct ext2_group_desc
> >> >>       __u16   bg_free_inodes_count;   /* Free inodes count */
> >> >>       __u16   bg_used_dirs_count;     /* Directories count */
> >> >>       __u16   bg_flags;
> >> >> -     __u32   bg_reserved[2];
> >> >> +     __u32   bg_exclude_bitmap_lo;   /* Exclude bitmap for snapshots */
> >> >> +     __u16   bg_block_bitmap_csum_lo;/* crc32c(s_uuid+grp_num+bitmap) LSB */
> >> >> +     __u16   bg_inode_bitmap_csum_lo;/* crc32c(s_uuid+grp_num+bitmap) LSB */
> >> >>       __u16   bg_itable_unused;       /* Unused inodes count */
> >> >>       __u16   bg_checksum;            /* crc16(s_uuid+grouo_num+group_desc)*/
> >> >>  };
> >> >> @@ -159,7 +161,9 @@ struct ext4_group_desc
> >> >>       __u16   bg_free_inodes_count;   /* Free inodes count */
> >> >>       __u16   bg_used_dirs_count;     /* Directories count */
> >> >>       __u16   bg_flags;               /* EXT4_BG_flags (INODE_UNINIT, etc) */
> >> >> -     __u32   bg_reserved[2];         /* Likely block/inode bitmap checksum */
> >> >> +     __u32   bg_exclude_bitmap_lo;   /* Exclude bitmap for snapshots */
> >> >> +     __u16   bg_block_bitmap_csum_lo;/* crc32c(s_uuid+grp_num+bitmap) LSB */
> >> >> +     __u16   bg_inode_bitmap_csum_lo;/* crc32c(s_uuid+grp_num+bitmap) LSB */
> >> >>       __u16   bg_itable_unused;       /* Unused inodes count */
> >> >>       __u16   bg_checksum;            /* crc16(sb_uuid+group+desc) */
> >> >>       __u32   bg_block_bitmap_hi;     /* Blocks bitmap block MSB */
> >> >> @@ -169,7 +173,10 @@ struct ext4_group_desc
> >> >>       __u16   bg_free_inodes_count_hi;/* Free inodes count MSB */
> >> >>       __u16   bg_used_dirs_count_hi;  /* Directories count MSB */
> >> >>       __u16   bg_itable_unused_hi;    /* Unused inodes count MSB */
> >> >> -     __u32   bg_reserved2[3];
> >> >> +     __u32   bg_exclude_bitmap_hi;   /* Exclude bitmap block MSB */
> >> >> +     __u16   bg_block_bitmap_csum_hi;/* crc32c(s_uuid+grp_num+bitmap) MSB */
> >> >> +     __u16   bg_inode_bitmap_csum_hi;/* crc32c(s_uuid+grp_num+bitmap) MSB */
> >> >> +     __u32   bg_reserved;
> >> >
> >> > Should we attach a checksum to the exclude bitmap?  That would, unfortunately,
> >> > put the 64-byte block group pretty close to full, unless we're ok with making
> >> > the bg even bigger...
> >>
> >> Good point.
> >> My initial approach (which I probably expressed somewhere) was that when
> >> exclude_bitmap feature is set, the block bitmap checksum should cover both
> >> block bitmap and exclude bitmap.
> >> The reason being that exclude bitmap adds little entropy over block bitmap:
> >> - it should always be a sub set of block bitmap bits
> >> - clearing exclude bit is done together with clearing the block used bit
> >> - the only case of modifying exclude bitmap only is when a used block becomes
> >>   excluded (a.k.a moved to snapshot)
> >>
> >> so how do you feel about compressing 2 blocks into 16bits of checksum :-/?
> >
> > Hrm.... is it generally the case that both block and exclude bitmaps are loaded
> > at the same time?  I don't think it'd be difficult to checksum both blocks
> > unless it's common that one of the two are not in memory.
> >
> 
> Hrm indeed. no. exclude bitmap is currently not loaded always together with
> block bitmap, so unless this is changed, we'll have to think of something else.

Or... does it make sense to preload the exclude bitmap at the same time as the
block bitmap?  If I'm reading you correctly, (block_bitmap | exclude_bitmap)
yields a bitmap of all blocks that are not available, correct?  So in the case
that exclude bitmaps are enabled, you'd need both to be in memory anyway.

> However that brings me to wonder:
> block/inode/exclude bitmap seldom change more than a handful of bits/words
> at a time.
> Does it make sense to diff update the CRC of the bitmaps instead of
> recomputing it?

Yes, that would be a good (later) optimization at least to consider.

--D
--
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