diff mbox

[05/23] ext4: Calculate and verify superblock checksum

Message ID 20120306204828.1663.39312.stgit@elm3b70.beaverton.ibm.com
State Superseded, archived
Delegated to: Theodore Ts'o
Headers show

Commit Message

Darrick J. Wong March 6, 2012, 8:48 p.m. UTC 
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      |   10 ++++++++++
 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    |    4 +++-
 fs/ext4/super.c     |   47 +++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 76 insertions(+), 8 deletions(-)



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

Comments

Theodore Ts'o April 27, 2012, 8:05 p.m. UTC | #1 
On Tue, Mar 06, 2012 at 12:48:28PM -0800, Darrick J. Wong wrote:
> diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
> index aca1790..90f7c2e 100644
> --- a/fs/ext4/ext4_jbd2.c
> +++ b/fs/ext4/ext4_jbd2.c
> @@ -138,16 +138,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;

What's the point of having the ext4_handle_dirty_super_now() variant?

I don't see the point of having this?

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Darrick J. Wong April 30, 2012, 4:19 p.m. UTC | #2 
On Fri, Apr 27, 2012 at 04:05:00PM -0400, Ted Ts'o wrote:
> On Tue, Mar 06, 2012 at 12:48:28PM -0800, Darrick J. Wong wrote:
> > diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
> > index aca1790..90f7c2e 100644
> > --- a/fs/ext4/ext4_jbd2.c
> > +++ b/fs/ext4/ext4_jbd2.c
> > @@ -138,16 +138,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;
> 
> What's the point of having the ext4_handle_dirty_super_now() variant?
> 
> I don't see the point of having this?

I believe I was trying to get rid of the open-coded mark_buffer_dirty(sb)
calls.  There isn't much of reason to have the now=0 path, though; if a caller
really wants to mark s_dirt and nothing else, there's ext4_mark_super_dirty()
for that.  That said, I wonder about that -- is it desirable to be able to say
"I've changed the superblock, now update the checksum but don't do anything to
write it out to disk right now"?

After a few months' break, it seems clear to me that we could make the "else if
(now)" clause the "else" clause and get rid of the now parameter.  Anything
that really wants to set s_dirt=1 with no further action can call
ext4_mark_super_dirty().

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

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

Patch

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index a4b2689..0f48aea 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -1273,6 +1273,9 @@  struct ext4_sb_info {
 
 	/* Reference to checksum algorithm driver via cryptoapi */
 	struct crypto_shash *s_chksum_driver;
+
+	/* Precomputed FS UUID checksum for seeding other checksums */
+	__u32 s_csum_seed;
 };
 
 static inline struct ext4_sb_info *EXT4_SB(struct super_block *sb)
@@ -1985,6 +1988,10 @@  extern int ext4_group_extend(struct super_block *sb,
 extern int ext4_resize_fs(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);
@@ -2260,6 +2267,9 @@  static inline void ext4_unlock_group(struct super_block *sb,
 
 static inline void ext4_mark_super_dirty(struct super_block *sb)
 {
+	struct ext4_super_block *es = EXT4_SB(sb)->s_es;
+
+	ext4_superblock_csum_set(sb, es);
 	if (EXT4_SB(sb)->s_journal == NULL)
 		sb->s_dirt =1;
 }
diff --git a/fs/ext4/ext4_jbd2.c b/fs/ext4/ext4_jbd2.c
index aca1790..90f7c2e 100644
--- a/fs/ext4/ext4_jbd2.c
+++ b/fs/ext4/ext4_jbd2.c
@@ -138,16 +138,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 feaa82f..f3afe9c 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3926,8 +3926,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 8b56fdf..e62e517 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -2021,7 +2021,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;
@@ -2094,7 +2094,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 c6898e1..20c0a54 100644
--- a/fs/ext4/resize.c
+++ b/fs/ext4/resize.c
@@ -796,7 +796,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);
 
@@ -968,6 +968,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;
 
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index cfd8aa3..9881e90 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -121,6 +121,38 @@  static int ext4_verify_csum_type(struct super_block *sb,
 	return es->s_checksum_type == EXT4_CRC32C_CHKSUM;
 }
 
+static __le32 ext4_superblock_csum(struct super_block *sb,
+				   struct ext4_super_block *es)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	int offset = offsetof(struct ext4_super_block, s_checksum);
+	__u32 csum;
+
+	csum = ext4_chksum(sbi, ~0, (char *)es, offset);
+
+	return cpu_to_le32(csum);
+}
+
+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))
+		return 1;
+
+	return es->s_checksum == ext4_superblock_csum(sb, es);
+}
+
+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;
@@ -3210,6 +3242,20 @@  static int ext4_fill_super(struct super_block *sb, void *data, int silent)
 		}
 	}
 
+	/* Check superblock checksum */
+	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;
+	}
+
+	/* Precompute checksum seed for all metadata */
+	if (EXT4_HAS_RO_COMPAT_FEATURE(sb,
+			EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
+		sbi->s_csum_seed = ext4_chksum(sbi, ~0, es->s_uuid,
+					       sizeof(es->s_uuid));
+
 	/* Set defaults before we parse the mount options */
 	def_mount_opts = le32_to_cpu(es->s_default_mount_opts);
 	set_opt(sb, INIT_INODE_TABLE);
@@ -4218,6 +4264,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);