Re: Fwd: Re: [PATCH] btrfs: add more superblock checks
On Tue, Oct 07, 2014 at 04:51:11PM +0800, Qu Wenruo wrote:
+ struct btrfs_super_block *sb = fs_info-super_copy;
+ int ret = 0;
+
+ if (sb-root_level BTRFS_MAX_LEVEL) {
+ printk(KERN_ERR BTRFS: tree_root level too big: %d %d\n,
+ sb-root_level, BTRFS_MAX_LEVEL);
+ ret = -EINVAL;
+ }
+ if (sb-chunk_root_level BTRFS_MAX_LEVEL) {
+ printk(KERN_ERR BTRFS: chunk_root level too big: %d %d\n,
+ sb-chunk_root_level, BTRFS_MAX_LEVEL);
+ ret = -EINVAL;
+ }
+ if (sb-log_root_level BTRFS_MAX_LEVEL) {
+ printk(KERN_ERR BTRFS: log_root level too big: %d %d\n,
+ sb-log_root_level, BTRFS_MAX_LEVEL);
+ ret = -EINVAL;
+ }
+
/*
-* Placeholder for checks
+* The common minimum, we don't know if we can trust the
nodesize/sectorsize
+* items yet, they'll be verified later. Issue just a warning.
*/
- return 0;
+ if (!IS_ALIGNED(sb-root, 4096))
+ printk(KERN_WARNING BTRFS: tree_root block unaligned: %llu\n,
+ sb-root);
+ if (!IS_ALIGNED(sb-chunk_root, 4096))
+ printk(KERN_WARNING BTRFS: tree_root block unaligned: %llu\n,
+ sb-chunk_root);
+ if (!IS_ALIGNED(sb-log_root, 4096))
+ printk(KERN_WARNING BTRFS: tree_root block unaligned: %llu\n,
+ sb-log_root);
1) it is better not to call IS_ALIGNED to immediate value//.
Although current btrfs implement ensures that all sectorsize is larger
equal than page_size,
but Chandan Rajendra is trying to support subpage-sized blocksize,
which may cause false alert later.
The patch reflects current state, so when the subpage blocksize patches
are merged, this will have to be changed accordingly.
It would be much better using btrfs_super_sectorsize() instead to
improve extendability.
See the comment above, we don't trust the superblock yet and cannot use
the sectorsize reliably.
2) missing endian convert.
On big endian system it would be a disaster
btrfs_super_* marco should be used.
Thanks, will fix it.
+ if (memcmp(fs_info-fsid, sb-dev_item.fsid, BTRFS_UUID_SIZE) != 0) {
+ printk(KERN_ERR BTRFS: dev_item UUID does not match fsid: %pU
!= %pU\n,
+ fs_info-fsid, sb-dev_item.fsid);
+ ret = -EINVAL;
+ }
+
+ /*
+* Hint to catch really bogus numbers, bitflips or so, more exact
checks are
+* done later
+*/
+ if (sb-num_devices (1UL 31))
+ printk(KERN_WARNING BTRFS: suspicious number of devices:
%llu\n,
+ sb-num_devices);
What about also check the devid with sb-num_devices too?
Every valid devid should be less equal than sb-num_devices if I am right.
Although iterate dev_item here may be overkilled...
This could be done of course, I've tried to keep the checks very small
and using only directly accessible information. More is possible of
course.
+
+ if (sb-bytenr != BTRFS_SUPER_INFO_OFFSET) {
+ printk(KERN_ERR BTRFS: super offset mismatch %llu != %u\n,
+ sb-bytenr, BTRFS_SUPER_INFO_OFFSET);
+ ret = -EINVAL;
+ }
+
+ /*
+* The generation is a global counter, we'll trust it more than the
others
+* but it's still possible that it's the one that's wrong.
+*/
+ if (sb-generation sb-chunk_root_generation)
+ printk(KERN_WARNING
+ BTRFS: suspicious: generation chunk_root_generation:
%llu %llu\n,
+ sb-generation, sb-chunk_root_generation);
+ if (sb-generation sb-cache_generation sb-cache_generation !=
(u64)-1)
+ printk(KERN_WARNING
+ BTRFS: suspicious: generation cache_generation: %llu
%llu\n,
+ sb-generation, sb-cache_generation);
+
+ return ret;
}
Still the endian problem.
Will fix, thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-btrfs in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Fwd: Re: [PATCH] btrfs: add more superblock checks
Previous reply seems failed to be delivered.
Resend.
转发的消息
主题: Re: [PATCH] btrfs: add more superblock checks
日期: Tue, 07 Oct 2014 16:44:07 +0800
发件人:Qu Wenruo quwen...@cn.fujitsu.com
收件人:David Sterba dste...@suse.cz, linux-btrfs@vger.kernel.org
Thanks a lot for the patch to deal with the broken image.
Also this seems more extendable.
Small comment inlined below.
Original Message
Subject: [PATCH] btrfs: add more superblock checks
From: David Sterba dste...@suse.cz
To: linux-btrfs@vger.kernel.org
Date: 2014年10月01日 01:16
Populate btrfs_check_super_valid() with checks that try to verify
consistency of superblock by additional conditions that may arise from
corrupted devices or bitflips. Some of tests are only hints and issue
warnings instead of failing the mount, basically when the checks are
derived from the data found in the superblock.
Tested on a broken image provided by Qu.
Reported-by: Qu Wenruoquwen...@cn.fujitsu.com
Signed-off-by: David Sterbadste...@suse.cz
---
fs/btrfs/disk-io.c | 67 --
1 file changed, 65 insertions(+), 2 deletions(-)
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index a1d36e62179c..bfb00cb1c84c 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3814,10 +3814,73 @@ int btrfs_read_buffer(struct extent_buffer *buf, u64
parent_transid)
static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info,
int read_only)
{
+ struct btrfs_super_block *sb = fs_info-super_copy;
+ int ret = 0;
+
+ if (sb-root_level BTRFS_MAX_LEVEL) {
+ printk(KERN_ERR BTRFS: tree_root level too big: %d %d\n,
+ sb-root_level, BTRFS_MAX_LEVEL);
+ ret = -EINVAL;
+ }
+ if (sb-chunk_root_level BTRFS_MAX_LEVEL) {
+ printk(KERN_ERR BTRFS: chunk_root level too big: %d %d\n,
+ sb-chunk_root_level, BTRFS_MAX_LEVEL);
+ ret = -EINVAL;
+ }
+ if (sb-log_root_level BTRFS_MAX_LEVEL) {
+ printk(KERN_ERR BTRFS: log_root level too big: %d %d\n,
+ sb-log_root_level, BTRFS_MAX_LEVEL);
+ ret = -EINVAL;
+ }
+
/*
-* Placeholder for checks
+* The common minimum, we don't know if we can trust the
nodesize/sectorsize
+* items yet, they'll be verified later. Issue just a warning.
*/
- return 0;
+ if (!IS_ALIGNED(sb-root, 4096))
+ printk(KERN_WARNING BTRFS: tree_root block unaligned: %llu\n,
+ sb-root);
+ if (!IS_ALIGNED(sb-chunk_root, 4096))
+ printk(KERN_WARNING BTRFS: tree_root block unaligned: %llu\n,
+ sb-chunk_root);
+ if (!IS_ALIGNED(sb-log_root, 4096))
+ printk(KERN_WARNING BTRFS: tree_root block unaligned: %llu\n,
+ sb-log_root);
1) it is better not to call IS_ALIGNED to immediate value//.
Although current btrfs implement ensures that all sectorsize is larger
equal than page_size,
but Chandan Rajendra is trying to support subpage-sized blocksize,
which may cause false alert later.
It would be much better using btrfs_super_sectorsize() instead to
improve extendability.
2) missing endian convert.
On big endian system it would be a disaster
btrfs_super_* marco should be used.
+
+ if (memcmp(fs_info-fsid, sb-dev_item.fsid, BTRFS_UUID_SIZE) != 0) {
+ printk(KERN_ERR BTRFS: dev_item UUID does not match fsid: %pU !=
%pU\n,
+ fs_info-fsid, sb-dev_item.fsid);
+ ret = -EINVAL;
+ }
+
+ /*
+* Hint to catch really bogus numbers, bitflips or so, more exact
checks are
+* done later
+*/
+ if (sb-num_devices (1UL 31))
+ printk(KERN_WARNING BTRFS: suspicious number of devices:
%llu\n,
+ sb-num_devices);
What about also check the devid with sb-num_devices too?
Every valid devid should be less equal than sb-num_devices if I am right.
Although iterate dev_item here may be overkilled...
+
+ if (sb-bytenr != BTRFS_SUPER_INFO_OFFSET) {
+ printk(KERN_ERR BTRFS: super offset mismatch %llu != %u\n,
+ sb-bytenr, BTRFS_SUPER_INFO_OFFSET);
+ ret = -EINVAL;
+ }
+
+ /*
+* The generation is a global counter, we'll trust it more than the
others
+* but it's still possible that it's the one that's wrong.
+*/
+ if (sb-generation sb-chunk_root_generation)
+ printk(KERN_WARNING
+ BTRFS: suspicious: generation chunk_root_generation: %llu
%llu\n,
+ sb-generation, sb-chunk_root_generation);
+ if (sb-generation sb-cache_generation
