From: "Darrick J. Wong" <djwong@us.ibm.com>
To: Andreas Dilger <adilger.kernel@dilger.ca>
Cc: Theodore Tso <tytso@mit.edu>,
Sunil Mushran <sunil.mushran@oracle.com>,
Amir Goldstein <amir73il@gmail.com>,
linux-kernel <linux-kernel@vger.kernel.org>,
Andi Kleen <andi@firstfloor.org>, Mingming Cao <cmm@us.ibm.com>,
Joel Becker <jlbec@evilplan.org>,
linux-fsdevel <linux-fsdevel@vger.kernel.org>,
linux-ext4@vger.kernel.org, Coly Li <colyli@gmail.com>
Subject: Re: [PATCH 15/16] ext4: Calculate and verify superblock checksum
Date: Fri, 2 Sep 2011 11:40:32 -0700 [thread overview]
Message-ID: <20110902184032.GE12086@tux1.beaverton.ibm.com> (raw)
In-Reply-To: <2882FBB2-797C-4D27-8569-B6826DD34F68@dilger.ca>
On Thu, Sep 01, 2011 at 01:52:43AM -0600, Andreas Dilger wrote:
> On 2011-08-31, at 6:32 PM, Darrick J. Wong wrote:
> > Calculate and verify the superblock checksum. Since the UUID and block group
> > number are embedded in each copy of the superblock, we need only checksum the
> > entire block. Refactor some of the code to eliminate open-coding of the
> > checksum update call.
> >
> > Signed-off-by: Darrick J. Wong <djwong@us.ibm.com>
> > ---
> > fs/ext4/ext4.h | 8 +++++++-
> > fs/ext4/ext4_jbd2.c | 9 ++++++++-
> > fs/ext4/ext4_jbd2.h | 7 +++++--
> > fs/ext4/inode.c | 3 +--
> > fs/ext4/namei.c | 4 ++--
> > fs/ext4/resize.c | 6 +++++-
> > fs/ext4/super.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> > 7 files changed, 71 insertions(+), 9 deletions(-)
> >
> >
> > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> > index b7aa5b5..1e93410 100644
> > --- a/fs/ext4/ext4.h
> > +++ b/fs/ext4/ext4.h
> > @@ -1067,7 +1067,9 @@ struct ext4_super_block {
> > __u8 s_last_error_func[32]; /* function where the error happened */
> > #define EXT4_S_ERR_END offsetof(struct ext4_super_block, s_mount_opts)
> > __u8 s_mount_opts[64];
> > - __le32 s_reserved[112]; /* Padding to the end of the block */
> > + __u32 s_reserved1[3]; /* Padding */
>
> Rather than mark these as reserved, it would be better to fill in the
> intended field names so that there is no confusion later on. I believe
> that there is s_usr_quota_inum and s_grp_quota_inum immediately following
> s_mount_opts, but I'm not sure what the 3rd reserved field is for.
"s_overhead_blocks", which I think is somehow related to bigalloc. They
haven't appeared in the kernel yet, which made me wary of adding fields that
aren't used by this patchset.
> > + __u32 s_checksum; /* crc32c(superblock) */
> > + __le32 s_reserved2[108]; /* Padding to the end of the block */
> > };
> >
> > #define EXT4_S_ERR_LEN (EXT4_S_ERR_END - EXT4_S_ERR_START)
> > @@ -1901,6 +1903,10 @@ extern int ext4_group_extend(struct super_block *sb,
> > ext4_fsblk_t n_blocks_count);
> >
> > /* super.c */
> > +extern int ext4_superblock_csum_verify(struct super_block *sb,
> > + struct ext4_super_block *es);
> > +extern void ext4_superblock_csum_set(struct super_block *sb,
> > + struct ext4_super_block *es);
> > extern void *ext4_kvmalloc(size_t size, gfp_t flags);
> > extern void *ext4_kvzalloc(size_t size, gfp_t flags);
> > extern void ext4_kvfree(void *ptr);
> > diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
> > index f5240aa..04ddc97 100644
> > --- a/fs/ext4/ext4_jbd2.c
> > +++ b/fs/ext4/ext4_jbd2.c
> > @@ -136,16 +136,23 @@ int __ext4_handle_dirty_metadata(const char *where, unsigned int line,
> > }
> >
> > int __ext4_handle_dirty_super(const char *where, unsigned int line,
> > - handle_t *handle, struct super_block *sb)
> > + handle_t *handle, struct super_block *sb,
> > + int now)
> > {
> > struct buffer_head *bh = EXT4_SB(sb)->s_sbh;
> > int err = 0;
> >
> > if (ext4_handle_valid(handle)) {
> > + ext4_superblock_csum_set(sb,
> > + (struct ext4_super_block *)bh->b_data);
> > err = jbd2_journal_dirty_metadata(handle, bh);
> > if (err)
> > ext4_journal_abort_handle(where, line, __func__,
> > bh, handle, err);
> > + } else if (now) {
> > + ext4_superblock_csum_set(sb,
> > + (struct ext4_super_block *)bh->b_data);
> > + mark_buffer_dirty(bh);
> > } else
> > sb->s_dirt = 1;
> > return err;
> > diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
> > index 5802fa1..ed9b78d 100644
> > --- a/fs/ext4/ext4_jbd2.h
> > +++ b/fs/ext4/ext4_jbd2.h
> > @@ -141,7 +141,8 @@ int __ext4_handle_dirty_metadata(const char *where, unsigned int line,
> > struct buffer_head *bh);
> >
> > int __ext4_handle_dirty_super(const char *where, unsigned int line,
> > - handle_t *handle, struct super_block *sb);
> > + handle_t *handle, struct super_block *sb,
> > + int now);
> >
> > #define ext4_journal_get_write_access(handle, bh) \
> > __ext4_journal_get_write_access(__func__, __LINE__, (handle), (bh))
> > @@ -153,8 +154,10 @@ int __ext4_handle_dirty_super(const char *where, unsigned int line,
> > #define ext4_handle_dirty_metadata(handle, inode, bh) \
> > __ext4_handle_dirty_metadata(__func__, __LINE__, (handle), (inode), \
> > (bh))
> > +#define ext4_handle_dirty_super_now(handle, sb) \
> > + __ext4_handle_dirty_super(__func__, __LINE__, (handle), (sb), 1)
> > #define ext4_handle_dirty_super(handle, sb) \
> > - __ext4_handle_dirty_super(__func__, __LINE__, (handle), (sb))
> > + __ext4_handle_dirty_super(__func__, __LINE__, (handle), (sb), 0)
> >
> > handle_t *ext4_journal_start_sb(struct super_block *sb, int nblocks);
> > int __ext4_journal_stop(const char *where, unsigned int line, handle_t *handle);
> > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> > index e24ba98..52e9b67 100644
> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -3761,8 +3761,7 @@ static int ext4_do_update_inode(handle_t *handle,
> > EXT4_FEATURE_RO_COMPAT_LARGE_FILE);
> > sb->s_dirt = 1;
> > ext4_handle_sync(handle);
> > - err = ext4_handle_dirty_metadata(handle, NULL,
> > - EXT4_SB(sb)->s_sbh);
> > + err = ext4_handle_dirty_super_now(handle, sb);
> > }
> > }
> > raw_inode->i_generation = cpu_to_le32(inode->i_generation);
> > diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> > index 2d0fdb9..5ebf281 100644
> > --- a/fs/ext4/namei.c
> > +++ b/fs/ext4/namei.c
> > @@ -2407,7 +2407,7 @@ int ext4_orphan_add(handle_t *handle, struct inode *inode)
> > /* Insert this inode at the head of the on-disk orphan list... */
> > NEXT_ORPHAN(inode) = le32_to_cpu(EXT4_SB(sb)->s_es->s_last_orphan);
> > EXT4_SB(sb)->s_es->s_last_orphan = cpu_to_le32(inode->i_ino);
> > - err = ext4_handle_dirty_metadata(handle, NULL, EXT4_SB(sb)->s_sbh);
> > + err = ext4_handle_dirty_super_now(handle, sb);
> > rc = ext4_mark_iloc_dirty(handle, inode, &iloc);
> > if (!err)
> > err = rc;
> > @@ -2480,7 +2480,7 @@ int ext4_orphan_del(handle_t *handle, struct inode *inode)
> > if (err)
> > goto out_brelse;
> > sbi->s_es->s_last_orphan = cpu_to_le32(ino_next);
> > - err = ext4_handle_dirty_metadata(handle, NULL, sbi->s_sbh);
> > + err = ext4_handle_dirty_super_now(handle, inode->i_sb);
> > } else {
> > struct ext4_iloc iloc2;
> > struct inode *i_prev =
> > diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c
> > index 707d3f1..2ad7008 100644
> > --- a/fs/ext4/resize.c
> > +++ b/fs/ext4/resize.c
> > @@ -511,7 +511,7 @@ static int add_new_gdb(handle_t *handle, struct inode *inode,
> > ext4_kvfree(o_group_desc);
> >
> > le16_add_cpu(&es->s_reserved_gdt_blocks, -1);
> > - err = ext4_handle_dirty_metadata(handle, NULL, EXT4_SB(sb)->s_sbh);
> > + err = ext4_handle_dirty_super_now(handle, sb);
> > if (err)
> > ext4_std_error(sb, err);
> >
> > @@ -682,6 +682,8 @@ static void update_backups(struct super_block *sb,
> > goto exit_err;
> > }
> >
> > + ext4_superblock_csum_set(sb, (struct ext4_super_block *)data);
> > +
> > while ((group = ext4_list_backups(sb, &three, &five, &seven)) < last) {
> > struct buffer_head *bh;
> >
> > @@ -925,6 +927,8 @@ int ext4_group_add(struct super_block *sb, struct ext4_new_group_data *input)
> > /* Update the global fs size fields */
> > sbi->s_groups_count++;
> >
> > + ext4_superblock_csum_set(sb,
> > + (struct ext4_super_block *)primary->b_data);
> > err = ext4_handle_dirty_metadata(handle, NULL, primary);
> > if (unlikely(err)) {
> > ext4_std_error(sb, err);
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 44d0c8d..b254274 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -38,6 +38,7 @@
> > #include <linux/ctype.h>
> > #include <linux/log2.h>
> > #include <linux/crc16.h>
> > +#include <linux/crc32c.h>
> > #include <linux/cleancache.h>
> > #include <asm/uaccess.h>
> >
> > @@ -110,6 +111,41 @@ static struct file_system_type ext3_fs_type = {
> > #define IS_EXT3_SB(sb) (0)
> > #endif
> >
> > +static __le32 ext4_superblock_csum(struct super_block *sb,
> > + struct ext4_super_block *es)
> > +{
> > + int offset = offsetof(struct ext4_super_block, s_checksum);
> > + __u32 crc = 0;
> > +
> > + if (!EXT4_HAS_RO_COMPAT_FEATURE(sb,
> > + EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
> > + return 0;
> > +
> > + crc = crc32c_le(~0, (char *)es, offset);
>
> For consistency, shouldn't this also checksum the fields _after_ offset?
> Otherwise, if new fields are used after s_checksum the update to this
> function may easily be missed, and those new fields would not be covered
> by the checksum. It also means the superblock checksum would be different
> based on which kernel is being used (based on which fields it knows about.
Actually, I had pondered putting the checksum at the very end of the 1k
superblock, which makes the checksum cover the padding too. On the other hand,
I thought I might save us ~400b of crc32c computation time. I don't have a big
objection to crc'ing all the padding.
--D
>
> > +
> > + return cpu_to_le32(crc);
> > +}
> > +
> > +int ext4_superblock_csum_verify(struct super_block *sb,
> > + struct ext4_super_block *es)
> > +{
> > + if (EXT4_HAS_RO_COMPAT_FEATURE(sb,
> > + EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) &&
> > + (es->s_checksum != ext4_superblock_csum(sb, es)))
> > + return 0;
> > + return 1;
> > +}
> > +
> > +void ext4_superblock_csum_set(struct super_block *sb,
> > + struct ext4_super_block *es)
> > +{
> > + if (!EXT4_HAS_RO_COMPAT_FEATURE(sb,
> > + EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
> > + return;
> > +
> > + es->s_checksum = ext4_superblock_csum(sb, es);
> > +}
> > +
> > void *ext4_kvmalloc(size_t size, gfp_t flags)
> > {
> > void *ret;
> > @@ -3151,6 +3187,12 @@ static int ext4_fill_super(struct super_block *sb, void *data, int silent)
> > sb->s_magic = le16_to_cpu(es->s_magic);
> > if (sb->s_magic != EXT4_SUPER_MAGIC)
> > goto cantfind_ext4;
> > + if (!ext4_superblock_csum_verify(sb, es)) {
> > + ext4_msg(sb, KERN_ERR, "VFS: Found ext4 filesystem with "
> > + "invalid superblock checksum. Run e2fsck?");
> > + silent = 1;
> > + goto cantfind_ext4;
> > + }
> > sbi->s_kbytes_written = le64_to_cpu(es->s_kbytes_written);
> >
> > /* Set defaults before we parse the mount options */
> > @@ -4107,6 +4149,7 @@ static int ext4_commit_super(struct super_block *sb, int sync)
> > &EXT4_SB(sb)->s_freeinodes_counter));
> > sb->s_dirt = 0;
> > BUFFER_TRACE(sbh, "marking dirty");
> > + ext4_superblock_csum_set(sb, es);
> > mark_buffer_dirty(sbh);
> > if (sync) {
> > error = sync_dirty_buffer(sbh);
> >
>
next prev parent reply other threads:[~2011-09-02 18:40 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-09-01 0:30 [PATCH v1 00/16] ext4: Add metadata checksumming Darrick J. Wong
2011-09-01 0:30 ` [PATCH 01/16] ext4: ext4_dx_add_entry should dirty directory metadata with the directory inode Darrick J. Wong
2011-09-01 0:30 ` [PATCH 02/16] ext4: ext4_rename should dirty dir_bh with the correct directory Darrick J. Wong
2011-09-01 0:30 ` [PATCH 03/16] ext4: ext4_mkdir should dirty dir_block with the parent inode Darrick J. Wong
2011-09-01 0:30 ` [PATCH 04/16] ext4: Create a new BH_Verified flag to avoid unnecessary metadata validation Darrick J. Wong
2011-09-01 0:31 ` [PATCH 05/16] ext4: Create a rocompat flag for extended metadata checksumming Darrick J. Wong
2011-09-01 0:31 ` [PATCH 06/16] ext4: Calculate and verify inode checksums Darrick J. Wong
2011-09-01 2:30 ` Andreas Dilger
2011-09-02 19:32 ` Darrick J. Wong
2011-09-02 22:02 ` Andreas Dilger
2011-09-05 17:57 ` Darrick J. Wong
2011-09-01 0:31 ` [PATCH 07/16] ext4: Create bitmap checksum helper functions Darrick J. Wong
2011-09-01 0:31 ` [PATCH 08/16] ext4: Calculate and verify checksums for inode bitmaps Darrick J. Wong
2011-09-01 4:49 ` Andreas Dilger
2011-09-02 19:18 ` Darrick J. Wong
2011-09-02 21:27 ` Andreas Dilger
2011-09-05 18:22 ` Darrick J. Wong
2011-09-05 18:27 ` Andi Kleen
2011-09-05 19:45 ` James Bottomley
2011-09-05 22:12 ` Darrick J. Wong
2011-09-01 0:31 ` [PATCH 09/16] ext4: Calculate and verify block bitmap checksum Darrick J. Wong
[not found] ` <2492E720-3316-4561-8C9C-BBC6E8670EAD@dilger.ca>
2011-09-02 19:08 ` Darrick J. Wong
2011-09-02 21:06 ` Andreas Dilger
2011-09-01 0:31 ` [PATCH 10/16] ext4: Verify and calculate checksums for extent tree blocks Darrick J. Wong
[not found] ` <26584BE9-B716-40D5-B3B4-8C5912869648@dilger.ca>
2011-09-02 19:02 ` Darrick J. Wong
2011-09-01 0:31 ` [PATCH 11/16] ext4: Calculate and verify checksums for htree nodes Darrick J. Wong
2011-09-01 0:31 ` [PATCH 12/16] ext4: Calculate and verify checksums of directory leaf blocks Darrick J. Wong
[not found] ` <4D5D8FB2-0D8A-4D30-B6D8-51158395C1C9@dilger.ca>
2011-09-02 18:57 ` Darrick J. Wong
2011-09-02 20:52 ` Andreas Dilger
2011-09-05 18:30 ` Darrick J. Wong
2011-09-01 0:32 ` [PATCH 13/16] ext4: Calculate and verify checksums of extended attribute blocks Darrick J. Wong
[not found] ` <F4A42DCE-F91C-419B-9153-65A7EA91D241@dilger.ca>
2011-09-02 18:43 ` Darrick J. Wong
2011-09-01 0:32 ` [PATCH 14/16] ext4: Make inode checksum cover empty space Darrick J. Wong
[not found] ` <4540D5DB-53D9-4C33-BC6B-868870D42AF3@dilger.ca>
2011-09-02 18:42 ` Darrick J. Wong
2011-09-01 0:32 ` [PATCH 15/16] ext4: Calculate and verify superblock checksum Darrick J. Wong
[not found] ` <2882FBB2-797C-4D27-8569-B6826DD34F68@dilger.ca>
2011-09-02 18:40 ` Darrick J. Wong [this message]
2011-09-01 0:32 ` [PATCH 16/16] jbd2: Support CRC32C transaction checksums Darrick J. Wong
[not found] ` <99019900-30B1-450A-9620-E94371A30CC6@dilger.ca>
2011-09-02 18:31 ` Darrick J. Wong
2011-09-02 14:15 ` [PATCH v1 00/16] ext4: Add metadata checksumming Greg Freemyer
2011-09-02 18:22 ` Darrick J. Wong
2011-09-04 11:41 ` Martin K. Petersen
2011-09-04 16:54 ` Andi Kleen
2011-09-04 17:17 ` Martin K. Petersen
2011-09-04 17:44 ` Andi Kleen
2011-09-04 22:19 ` Martin K. Petersen
2011-09-05 18:55 ` Darrick J. Wong
2011-09-05 18:45 ` Darrick J. Wong
2011-09-06 12:59 ` Martin K. Petersen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20110902184032.GE12086@tux1.beaverton.ibm.com \
--to=djwong@us.ibm.com \
--cc=adilger.kernel@dilger.ca \
--cc=amir73il@gmail.com \
--cc=andi@firstfloor.org \
--cc=cmm@us.ibm.com \
--cc=colyli@gmail.com \
--cc=jlbec@evilplan.org \
--cc=linux-ext4@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=sunil.mushran@oracle.com \
--cc=tytso@mit.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).