During a discussion at OLS, I came up with a very simple way of validating
the ext2/3/4 block bitmaps at read time. Until such a time when we have
checksums for the bitmaps we can have a simple but quite robust mechanism
that is useful for ext2/3/4.
When a new block bitmap is read from disk in read_block_bitmap() there are a
few bits that should ALWAYS be set. In particular, the blocks given by
desc->bg_block_bitmap, desc->bg_inode_bitmap, and the inode table in
[desc->bg_inode_table, +sbi->s_itb_per_group]. If those bits (shifted to be
relative to the current group, of course) are not set then the on-disk group
descriptor is corrupt, or there is some problem reading it from disk, and
this needs to generate an extN_error() call[*] to make the fs read-only.
A similar check can be done with the inode bitmap - it should have the
bits at the end of each bitmap set, for bits higher than s_inodes_per_group.
What I'm wondering is if anyone has time to implement this idea? I'm
estimating it wouldn't be about 30 lines of simple code in total.
[*] This reminds me - we should make the default ext4 error behaviour be
the safer "remount-ro" instead of the dangerous "continue".
Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.
Andreas Dilger wrote:
> During a discussion at OLS, I came up with a very simple way of validating
> the ext2/3/4 block bitmaps at read time. Until such a time when we have
> checksums for the bitmaps we can have a simple but quite robust mechanism
> that is useful for ext2/3/4.
>
> When a new block bitmap is read from disk in read_block_bitmap() there are a
> few bits that should ALWAYS be set. In particular, the blocks given by
> desc->bg_block_bitmap, desc->bg_inode_bitmap, and the inode table in
> [desc->bg_inode_table, +sbi->s_itb_per_group]. If those bits (shifted to be
> relative to the current group, of course) are not set then the on-disk group
> descriptor is corrupt, or there is some problem reading it from disk, and
> this needs to generate an extN_error() call[*] to make the fs read-only.
>
> A similar check can be done with the inode bitmap - it should have the
> bits at the end of each bitmap set, for bits higher than s_inodes_per_group.
Something like this ?. If yes i can send a patch with full changelog
diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index 44c6254..b9a334c 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -115,17 +115,50 @@ read_block_bitmap(struct super_block *sb, unsigned int block_group)
{
struct ext4_group_desc * desc;
struct buffer_head * bh = NULL;
+ ext4_fsblk_t bitmap_blk, grp_rel_blk, grp_first_blk;
desc = ext4_get_group_desc (sb, block_group, NULL);
if (!desc)
goto error_out;
- bh = sb_bread(sb, ext4_block_bitmap(sb, desc));
+ bitmap_blk = ext4_block_bitmap(sb, desc);
+ bh = sb_bread(sb, bitmap_blk);
if (!bh)
ext4_error (sb, "read_block_bitmap",
"Cannot read block bitmap - "
"block_group = %d, block_bitmap = %llu",
- block_group,
- ext4_block_bitmap(sb, desc));
+ block_group, bitmap_blk);
+
+ /* check whether block bitmap block number is set */
+ printk("blk bitmap = %llu first block = %llu block group = %u \n",
+ bitmap_blk, ext4_group_first_block_no(sb, block_group),
+ block_group);
+
+ grp_first_blk = ext4_group_first_block_no(sb, block_group);
+
+ grp_rel_blk = bitmap_blk - grp_first_blk;
+ if (!ext4_test_bit(grp_rel_blk, bh->b_data)) {
+ /* bad block bitmap */
+ brelse(bh);
+ return NULL;
+ }
+
+ /* check whether the inode bitmap block number is set */
+ bitmap_blk = ext4_inode_bitmap(sb, desc);
+ grp_rel_blk = bitmap_blk - grp_first_blk;
+ if (!ext4_test_bit(grp_rel_blk, bh->b_data)) {
+ /* bad block bitmap */
+ brelse(bh);
+ return NULL;
+ }
+ /* check whether the inode table block number is set */
+ bitmap_blk = ext4_inode_table(sb, desc);
+ grp_rel_blk = bitmap_blk - grp_first_blk;
+ if (!ext4_test_bit(grp_rel_blk, bh->b_data)) {
+ /* bad block bitmap */
+ brelse(bh);
+ return NULL;
+ }
+
error_out:
return bh;
}
On Jul 09, 2007 22:52 +0530, Aneesh Kumar K.V wrote:
> Something like this ?. If yes i can send a patch with full changelog
>
>
> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> index 44c6254..b9a334c 100644
> --- a/fs/ext4/balloc.c
> +++ b/fs/ext4/balloc.c
> @@ -115,17 +115,50 @@ read_block_bitmap(struct super_block *sb, unsigned
> int block_group)
> {
> struct ext4_group_desc * desc;
> struct buffer_head * bh = NULL;
> + ext4_fsblk_t bitmap_blk, grp_rel_blk, grp_first_blk;
>
> desc = ext4_get_group_desc (sb, block_group, NULL);
> if (!desc)
> goto error_out;
> - bh = sb_bread(sb, ext4_block_bitmap(sb, desc));
> + bitmap_blk = ext4_block_bitmap(sb, desc);
> + bh = sb_bread(sb, bitmap_blk);
> if (!bh)
> ext4_error (sb, "read_block_bitmap",
I prefer ext4_error(sb, __FUNCTION__, ...) since then we don't need to
update the error message when the function name changes, so while we are
here you may as well change it...
> "Cannot read block bitmap - "
> "block_group = %d, block_bitmap = %llu",
> - block_group,
> - ext4_block_bitmap(sb, desc));
> + block_group, bitmap_blk);
> +
> + /* check whether block bitmap block number is set */
> + printk("blk bitmap = %llu first block = %llu block group = %u \n",
> + bitmap_blk, ext4_group_first_block_no(sb, block_group),
> + block_group);
> + grp_first_blk = ext4_group_first_block_no(sb, block_group);
> +
This should be wrapped as ext4_debug(), and you may as well calculate
grp_first_blk and use that for the printed message.
> + grp_rel_blk = bitmap_blk - grp_first_blk;
> + if (!ext4_test_bit(grp_rel_blk, bh->b_data)) {
> + /* bad block bitmap */
> + brelse(bh);
> + return NULL;
> + }
> +
> + /* check whether the inode bitmap block number is set */
> + bitmap_blk = ext4_inode_bitmap(sb, desc);
> + grp_rel_blk = bitmap_blk - grp_first_blk;
> + if (!ext4_test_bit(grp_rel_blk, bh->b_data)) {
> + /* bad block bitmap */
> + brelse(bh);
> + return NULL;
> + }
> + /* check whether the inode table block number is set */
> + bitmap_blk = ext4_inode_table(sb, desc);
> + grp_rel_blk = bitmap_blk - grp_first_blk;
> + if (!ext4_test_bit(grp_rel_blk, bh->b_data)) {
> + /* bad block bitmap */
> + brelse(bh);
> + return NULL;
> + }
Each of these error returns should have an ext4_error() message so that the
filesystem is marked read-only if this happens. It might make sense to
just have the error message referenced by a char *, goto err_brelse and then
a single call and brelse(bh) at the end of the function. I've rewritten
the code below, but note this is a hand-edit and not a patch... Also note
that we can do the same for inode bitmaps, checking the bits between
[sbi->s_inodes_per_group, sb->s_blocksize * 8 - 1] are set.
We likely also need to skip these checks for uninit groups, so the context
of this patch should change to go into the ext4 patch queue (the checks go
into the "else" clause).
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -115,19 +115,54 @@ read_block_bitmap(struct super_block *sb, unsigned int block_group)
{
struct ext4_group_desc * desc;
struct buffer_head * bh = NULL;
+ ext4_fsblk_t bitmap_blk, grp_first_blk;
+ int i;
+ const char *msg;
desc = ext4_get_group_desc (sb, block_group, NULL);
if (!desc)
goto error_out;
- bh = sb_bread(sb, ext4_block_bitmap(sb, desc));
- if (!bh)
- ext4_error (sb, "read_block_bitmap",
- "Cannot read block bitmap - "
- "block_group = %d, block_bitmap = %llu",
- block_group,
- ext4_block_bitmap(sb, desc));
+ bitmap_blk = ext4_block_bitmap(sb, desc);
+ bh = sb_bread(sb, bitmap_blk);
+ if (!bh) {
+ msg = "Cannot read block bitmap"
+ goto error_out;
+ }
+
+ grp_first_blk = ext4_group_first_block_no(sb, block_group);
+
+ ext4_debug("blk bitmap = %llu first block = %llu block group = %u\n",
+ bitmap_blk, grp_first_blk, block_group);
+
+ /* check whether block bitmap block number is set */
+ if (!ext4_test_bit(bitmap_blk - grp_first_blk, bh->b_data)) {
+ msg = "Block bitmap block not marked in use";
+ goto error_brelse;
+ }
+
+ /* check whether the inode bitmap block number is set */
+ bitmap_blk = ext4_inode_bitmap(sb, desc);
+ if (!ext4_test_bit(bitmap_blk - grp_first_blk, bh->b_data)) {
+ msg = "Inode bitmap block not marked in use";
+ goto error_brelse;
+ }
+
+ /* check whether the inode table blocks are set */
+ bitmap_blk = ext4_inode_table(sb, desc);
+ for (i = 0; i < EXT4_SB(sb)->s_itb_per_group; i++, bitmap_blk++)
+ if (!ext4_test_bit(bitmap_blk - grp_first_blk, bh->b_data)){
+ msg = "Inode table block not marked in use";
+ goto error_brelse;
+ }
+ }
+ return bh;
+
+error_brelse:
+ brelse(bh);
error_out:
- return bh;
+ ext4_error(sb, __FUNCTION__, "%s - block_group %u, block %llu\n"
+ block_group, bitmap_blk);
+ return NULL;
}
Cheers, Andreas
--
Andreas Dilger
Principal Software Engineer
Cluster File Systems, Inc.