diff mbox

[2/2] ext4: Spot-check block group sub-table locations

Message ID 20130928001404.25857.43349.stgit@birch.djwong.org
State New, archived
Headers show

Commit Message

Darrick Wong Sept. 28, 2013, 12:14 a.m. UTC 
Perform a quick sanity check of bitmap and inode table block numbers when
loading them, and if there's something suspicious, mark the block group
corrupt.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/ext4/balloc.c |    7 +++++++
 fs/ext4/ext4.h   |    2 ++
 fs/ext4/ialloc.c |    6 ++++++
 fs/ext4/inode.c  |    9 +++++++++
 fs/ext4/super.c  |   21 +++++++++++++++++++++
 5 files changed, 45 insertions(+)



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

Zheng Liu Sept. 29, 2013, 6:44 a.m. UTC | #1 
On Fri, Sep 27, 2013 at 05:14:04PM -0700, Darrick J. Wong wrote:
> Perform a quick sanity check of bitmap and inode table block numbers when
> loading them, and if there's something suspicious, mark the block group
> corrupt.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

One comment below.  Otherwise the patch looks good to me.
Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>

> ---
>  fs/ext4/balloc.c |    7 +++++++
>  fs/ext4/ext4.h   |    2 ++
>  fs/ext4/ialloc.c |    6 ++++++
>  fs/ext4/inode.c  |    9 +++++++++
>  fs/ext4/super.c  |   21 +++++++++++++++++++++
>  5 files changed, 45 insertions(+)
[...]
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 2c2e6cb..d22248e 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -2049,6 +2049,27 @@ void ext4_group_desc_csum_set(struct super_block *sb, __u32 block_group,
>  	gdp->bg_checksum = ext4_group_desc_csum(EXT4_SB(sb), block_group, gdp);
>  }
>  
> +/* returns 1 if the location of a blockgroup data item seems sane */
> +int ext4_is_sane_bgdata_location(struct super_block *sb, ext4_group_t grp,
> +				 ext4_fsblk_t blk)
> +{
> +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> +	ext4_fsblk_t first_block = le32_to_cpu(sbi->s_es->s_first_data_block);
> +	ext4_fsblk_t last_block;
> +
> +	if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_FLEX_BG))
> +		last_block = ext4_blocks_count(sbi->s_es) - 1;
> +	else {
> +		first_block += grp * sbi->s_blocks_per_group;
> +		last_block = first_block + (EXT4_BLOCKS_PER_GROUP(sb) - 1);
> +	}
> +
> +	if (last_block >= ext4_blocks_count(sbi->s_es))
> +		last_block = ext4_blocks_count(sbi->s_es) - 1;
> +
> +	return blk >= first_block && blk <= last_block;
> +}
> +
>  /* Called at mount-time, super-block is locked */
>  static int ext4_check_descriptors(struct super_block *sb,
>  				  ext4_group_t *first_not_zeroed)

FWIW, I notice that in ext4_check_descriptors() we also check the
location of bitmap.  So maybe we could use ext4_is_sane_bgdata_location
in that function.

                                                - Zheng
--
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 Wong Oct. 9, 2013, 10:08 p.m. UTC | #2 
On Sun, Sep 29, 2013 at 02:44:01PM +0800, Zheng Liu wrote:
> On Fri, Sep 27, 2013 at 05:14:04PM -0700, Darrick J. Wong wrote:
> > Perform a quick sanity check of bitmap and inode table block numbers when
> > loading them, and if there's something suspicious, mark the block group
> > corrupt.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> One comment below.  Otherwise the patch looks good to me.
> Reviewed-by: Zheng Liu <wenqing.lz@taobao.com>
> 
> > ---
> >  fs/ext4/balloc.c |    7 +++++++
> >  fs/ext4/ext4.h   |    2 ++
> >  fs/ext4/ialloc.c |    6 ++++++
> >  fs/ext4/inode.c  |    9 +++++++++
> >  fs/ext4/super.c  |   21 +++++++++++++++++++++
> >  5 files changed, 45 insertions(+)
> [...]
> > diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> > index 2c2e6cb..d22248e 100644
> > --- a/fs/ext4/super.c
> > +++ b/fs/ext4/super.c
> > @@ -2049,6 +2049,27 @@ void ext4_group_desc_csum_set(struct super_block *sb, __u32 block_group,
> >  	gdp->bg_checksum = ext4_group_desc_csum(EXT4_SB(sb), block_group, gdp);
> >  }
> >  
> > +/* returns 1 if the location of a blockgroup data item seems sane */
> > +int ext4_is_sane_bgdata_location(struct super_block *sb, ext4_group_t grp,
> > +				 ext4_fsblk_t blk)
> > +{
> > +	struct ext4_sb_info *sbi = EXT4_SB(sb);
> > +	ext4_fsblk_t first_block = le32_to_cpu(sbi->s_es->s_first_data_block);
> > +	ext4_fsblk_t last_block;
> > +
> > +	if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_FLEX_BG))
> > +		last_block = ext4_blocks_count(sbi->s_es) - 1;
> > +	else {
> > +		first_block += grp * sbi->s_blocks_per_group;
> > +		last_block = first_block + (EXT4_BLOCKS_PER_GROUP(sb) - 1);
> > +	}
> > +
> > +	if (last_block >= ext4_blocks_count(sbi->s_es))
> > +		last_block = ext4_blocks_count(sbi->s_es) - 1;
> > +
> > +	return blk >= first_block && blk <= last_block;
> > +}
> > +
> >  /* Called at mount-time, super-block is locked */
> >  static int ext4_check_descriptors(struct super_block *sb,
> >  				  ext4_group_t *first_not_zeroed)
> 
> FWIW, I notice that in ext4_check_descriptors() we also check the
> location of bitmap.  So maybe we could use ext4_is_sane_bgdata_location
> in that function.

That could be done, but all we'd be doing is trading this:

	block_bitmap = ext4_block_bitmap(sb, gdp);
	if (block_bitmap < first_block || block_bitmap > last_block) {

for this slightly slower code:

	if (ext4_is_sane_bgdata_location(sb, gdp,
					 ext4_block_bitmap(sb, gdb))) {

I don't know if that's worth the code churn or not.  It's an easy change, but I
don't think it adds much.  Ted -- if you want me to make the change I'll do it,
but if you're content to leave the patch as it is, then so am I.

--D
> 
>                                                 - Zheng
--
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
Theodore Ts'o Oct. 30, 2013, 4:45 p.m. UTC | #3 
On Wed, Oct 09, 2013 at 03:08:24PM -0700, Darrick J. Wong wrote:
> > FWIW, I notice that in ext4_check_descriptors() we also check the
> > location of bitmap.  So maybe we could use ext4_is_sane_bgdata_location
> > in that function.
> 
> That could be done, but all we'd be doing is trading this:
> 
> 	block_bitmap = ext4_block_bitmap(sb, gdp);
> 	if (block_bitmap < first_block || block_bitmap > last_block) {
> 
> for this slightly slower code:
> 
> 	if (ext4_is_sane_bgdata_location(sb, gdp,
> 					 ext4_block_bitmap(sb, gdb))) {
> 
> I don't know if that's worth the code churn or not.  It's an easy change, but I
> don't think it adds much.  Ted -- if you want me to make the change I'll do it,
> but if you're content to leave the patch as it is, then so am I.

The question I have is if we are checking to make sure all of the
block and inode bitmaps are sane at mount time, how much is it worth
it to check to make sure that they are sane each time we need to load
the bitmap?  This is especially true since for ext4 file systems with
flex_bg, the only real check we can do is to is whether the block
number is out of range, which will almost certainly cause a device
error.

						- 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 Wong Oct. 30, 2013, 5:33 p.m. UTC | #4 
On Wed, Oct 30, 2013 at 12:45:07PM -0400, Theodore Ts'o wrote:
> On Wed, Oct 09, 2013 at 03:08:24PM -0700, Darrick J. Wong wrote:
> > > FWIW, I notice that in ext4_check_descriptors() we also check the
> > > location of bitmap.  So maybe we could use ext4_is_sane_bgdata_location
> > > in that function.
> > 
> > That could be done, but all we'd be doing is trading this:
> > 
> > 	block_bitmap = ext4_block_bitmap(sb, gdp);
> > 	if (block_bitmap < first_block || block_bitmap > last_block) {
> > 
> > for this slightly slower code:
> > 
> > 	if (ext4_is_sane_bgdata_location(sb, gdp,
> > 					 ext4_block_bitmap(sb, gdb))) {
> > 
> > I don't know if that's worth the code churn or not.  It's an easy change, but I
> > don't think it adds much.  Ted -- if you want me to make the change I'll do it,
> > but if you're content to leave the patch as it is, then so am I.
> 
> The question I have is if we are checking to make sure all of the
> block and inode bitmaps are sane at mount time, how much is it worth
> it to check to make sure that they are sane each time we need to load
> the bitmap?  This is especially true since for ext4 file systems with
> flex_bg, the only real check we can do is to is whether the block
> number is out of range, which will almost certainly cause a device
> error.

Probably not worth much, but I thought it was cheap compared to making and
bouncing an IO.  Also, if the check fails, we can mark the bg offline and let
fsck sort out the mess.  The regular error path doesn't do this... though we
could change that too.  

--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/balloc.c b/fs/ext4/balloc.c
index 6ea7b14..2f35689 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -396,13 +396,20 @@  ext4_read_block_bitmap_nowait(struct super_block *sb, ext4_group_t block_group)
 	struct ext4_group_desc *desc;
 	struct buffer_head *bh;
 	ext4_fsblk_t bitmap_blk;
+	struct ext4_group_info *grp;
 
 	desc = ext4_get_group_desc(sb, block_group, NULL);
 	if (!desc)
 		return NULL;
 	bitmap_blk = ext4_block_bitmap(sb, desc);
+	if (!ext4_is_sane_bgdata_location(sb, block_group, bitmap_blk)) {
+		grp = ext4_get_group_info(sb, block_group);
+		set_bit(EXT4_GROUP_INFO_BBITMAP_CORRUPT_BIT, &grp->bb_state);
+		goto no_bitmap;
+	}
 	bh = sb_getblk(sb, bitmap_blk);
 	if (unlikely(!bh)) {
+no_bitmap:
 		ext4_error(sb, "Cannot get buffer for block bitmap - "
 			   "block_group = %u, block_bitmap = %llu",
 			   block_group, bitmap_blk);
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index af815ea..afb1bb2 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2531,6 +2531,8 @@  static inline void ext4_unlock_group(struct super_block *sb,
 	spin_unlock(ext4_group_lock_ptr(sb, group));
 }
 
+int ext4_is_sane_bgdata_location(struct super_block *sb, ext4_group_t grp,
+				 ext4_fsblk_t blk);
 /*
  * Block validity checking
  */
diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
index 137193f..4e7994b 100644
--- a/fs/ext4/ialloc.c
+++ b/fs/ext4/ialloc.c
@@ -122,8 +122,14 @@  ext4_read_inode_bitmap(struct super_block *sb, ext4_group_t block_group)
 		return NULL;
 
 	bitmap_blk = ext4_inode_bitmap(sb, desc);
+	if (!ext4_is_sane_bgdata_location(sb, block_group, bitmap_blk)) {
+		grp = ext4_get_group_info(sb, block_group);
+		set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, &grp->bb_state);
+		goto no_bitmap;
+	}
 	bh = sb_getblk(sb, bitmap_blk);
 	if (unlikely(!bh)) {
+no_bitmap:
 		ext4_error(sb, "Cannot read inode bitmap - "
 			    "block_group = %u, inode_bitmap = %llu",
 			    block_group, bitmap_blk);
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 0d424d7..0a54e13 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -3795,6 +3795,7 @@  static int __ext4_get_inode_loc(struct inode *inode,
 	struct super_block	*sb = inode->i_sb;
 	ext4_fsblk_t		block;
 	int			inodes_per_block, inode_offset;
+	struct ext4_group_info	*grp;
 
 	iloc->bh = NULL;
 	if (!ext4_valid_inum(sb, inode->i_ino))
@@ -3814,6 +3815,14 @@  static int __ext4_get_inode_loc(struct inode *inode,
 	block = ext4_inode_table(sb, gdp) + (inode_offset / inodes_per_block);
 	iloc->offset = (inode_offset % inodes_per_block) * EXT4_INODE_SIZE(sb);
 
+	if (!ext4_is_sane_bgdata_location(sb, iloc->block_group, block)) {
+		grp = ext4_get_group_info(sb, iloc->block_group);
+		set_bit(EXT4_GROUP_INFO_IBITMAP_CORRUPT_BIT, &grp->bb_state);
+		ext4_error(sb, "Cannot read inode table - "
+			    "block_group = %u, inode_table_block = %llu",
+			    iloc->block_group, block);
+		return -EIO;
+	}
 	bh = sb_getblk(sb, block);
 	if (unlikely(!bh))
 		return -ENOMEM;
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 2c2e6cb..d22248e 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -2049,6 +2049,27 @@  void ext4_group_desc_csum_set(struct super_block *sb, __u32 block_group,
 	gdp->bg_checksum = ext4_group_desc_csum(EXT4_SB(sb), block_group, gdp);
 }
 
+/* returns 1 if the location of a blockgroup data item seems sane */
+int ext4_is_sane_bgdata_location(struct super_block *sb, ext4_group_t grp,
+				 ext4_fsblk_t blk)
+{
+	struct ext4_sb_info *sbi = EXT4_SB(sb);
+	ext4_fsblk_t first_block = le32_to_cpu(sbi->s_es->s_first_data_block);
+	ext4_fsblk_t last_block;
+
+	if (EXT4_HAS_INCOMPAT_FEATURE(sb, EXT4_FEATURE_INCOMPAT_FLEX_BG))
+		last_block = ext4_blocks_count(sbi->s_es) - 1;
+	else {
+		first_block += grp * sbi->s_blocks_per_group;
+		last_block = first_block + (EXT4_BLOCKS_PER_GROUP(sb) - 1);
+	}
+
+	if (last_block >= ext4_blocks_count(sbi->s_es))
+		last_block = ext4_blocks_count(sbi->s_es) - 1;
+
+	return blk >= first_block && blk <= last_block;
+}
+
 /* Called at mount-time, super-block is locked */
 static int ext4_check_descriptors(struct super_block *sb,
 				  ext4_group_t *first_not_zeroed)