[PATCH 0/4] Add superblock checksum check for btrfs-progs

[PATCH 0/4] Add superblock checksum check for btrfs-progs

[Qu Wenruo]

Before this patchset, btrfs-progs will overall ignore the superblock
checksum and continue the routine.
Sometimes this may cause disasters like checking a btrfs with corrupted
superblock will lead to crash in btrfs-progs.

This patch introduces superblock checksum check into btrfs_read_dev_super(),
making btrfs-progs much more restricted and robust.
To allow super-recover to open devices, add options to scan all 3
superblocks when using super-recover.
Also updated the related error string and fix a bug in chunk-recover that
will not be triggered until superblock csum is calculated.

Qu Wenruo (4):
  btrfs-progs: Check superblock's checsum when read dev super
  btrfs-progs: Allow btrfs_read_dev_super() to read all 3 super for    
    super_recover.
  btrfs-progs: Add more meaningful return value for
    btrfs_read_dev_super()     and corresponding error string.
  btrfs-progs: Fix size for malloc for superblock checksum.

 btrfs-find-root.c |  9 ++++--
 chunk-recover.c   | 18 +++++++----
 cmds-filesystem.c |  9 ++++--
 disk-io.c         | 91 +++++++++++++++++++++++++++++++++++++------------------
 disk-io.h         |  5 +--
 super-recover.c   |  2 +-
 utils.c           | 16 ++++++----
 volumes.c         |  8 ++---
 volumes.h         |  2 +-
 9 files changed, 104 insertions(+), 56 deletions(-)

-- 
2.0.1

[PATCH v2 1/4] btrfs-progs: Check superblock's checsum when read dev super

[Qu Wenruo]

Btrfs-progs will read the superblock without checking the checksum.
When all superblocks are corrupted, continuing will cause disaster.

So this patch will add checksum check for btrfs-progs when reading
superblocks.

Also fix a bug that btrfs_read_dev_super() only reads sizeof(struct
btrfs_super_block), corrent size should be BTRFS_SUPER_INFO_SIZE.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
v2:
  Use corrent memcmp src.
  Read the whole supblock size(sectorsize) other than
  sizeof(btrfs_super_block).
---
 disk-io.c | 46 +++++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 17 deletions(-)

diff --git a/disk-io.c b/disk-io.c
index 8db0335..e447af8 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -1186,22 +1186,25 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
 {
 	u8 fsid[BTRFS_FSID_SIZE];
 	int fsid_is_initialized = 0;
-	struct btrfs_super_block buf;
+	u8 data[BTRFS_SUPER_INFO_SIZE];
+	struct btrfs_super_block *buf = (struct btrfs_super_block *) data;
 	int i;
 	int ret;
 	u64 transid = 0;
 	u64 bytenr;
+	u32 crc;
+	char crc_result[BTRFS_CSUM_SIZE];
 
 	if (sb_bytenr != BTRFS_SUPER_INFO_OFFSET) {
-		ret = pread64(fd, &buf, sizeof(buf), sb_bytenr);
-		if (ret < sizeof(buf))
+		ret = pread64(fd, data, sizeof(data), sb_bytenr);
+		if (ret < sizeof(data))
 			return -1;
 
-		if (btrfs_super_bytenr(&buf) != sb_bytenr ||
-		    btrfs_super_magic(&buf) != BTRFS_MAGIC)
+		if (btrfs_super_bytenr(buf) != sb_bytenr ||
+		    btrfs_super_magic(buf) != BTRFS_MAGIC)
 			return -1;
 
-		memcpy(sb, &buf, sizeof(*sb));
+		memcpy(sb, data, sizeof(data));
 		return 0;
 	}
 
@@ -1214,22 +1217,31 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
 
 	for (i = 0; i < 1; i++) {
 		bytenr = btrfs_sb_offset(i);
-		ret = pread64(fd, &buf, sizeof(buf), bytenr);
-		if (ret < sizeof(buf))
+		ret = pread64(fd, data, sizeof(data), bytenr);
+		if (ret < sizeof(data))
 			break;
 
-		if (btrfs_super_bytenr(&buf) != bytenr )
+		if (btrfs_super_bytenr(buf) != bytenr)
 			continue;
-		/* if magic is NULL, the device was removed */
-		if (btrfs_super_magic(&buf) == 0 && i == 0)
+		/* if first super block is not btrfs, the device was removed */
+		if (btrfs_super_magic(buf) != BTRFS_MAGIC && i == 0)
 			return -1;
-		if (btrfs_super_magic(&buf) != BTRFS_MAGIC)
+		if (btrfs_super_magic(buf) != BTRFS_MAGIC)
+			continue;
+
+		/* check if the superblock is damaged */
+		crc = ~(u32)0;
+		crc = btrfs_csum_data(NULL, (char *)buf + BTRFS_CSUM_SIZE,
+				      crc, BTRFS_SUPER_INFO_SIZE -
+				      BTRFS_CSUM_SIZE);
+		btrfs_csum_final(crc, crc_result);
+		if (memcmp(crc_result, buf, btrfs_super_csum_size(buf)))
 			continue;
 
 		if (!fsid_is_initialized) {
-			memcpy(fsid, buf.fsid, sizeof(fsid));
+			memcpy(fsid, buf->fsid, sizeof(fsid));
 			fsid_is_initialized = 1;
-		} else if (memcmp(fsid, buf.fsid, sizeof(fsid))) {
+		} else if (memcmp(fsid, buf->fsid, sizeof(fsid))) {
 			/*
 			 * the superblocks (the original one and
 			 * its backups) contain data of different
@@ -1238,9 +1250,9 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
 			continue;
 		}
 
-		if (btrfs_super_generation(&buf) > transid) {
-			memcpy(sb, &buf, sizeof(*sb));
-			transid = btrfs_super_generation(&buf);
+		if (btrfs_super_generation(buf) > transid) {
+			memcpy(sb, data, sizeof(data));
+			transid = btrfs_super_generation(buf);
 		}
 	}
 
-- 
2.0.1

Re: [PATCH v2 1/4] btrfs-progs: Check superblock's checsum when read dev super

[David Sterba]

Hi,

sorry for late reply.

On Thu, Jul 03, 2014 at 05:36:35PM +0800, Qu Wenruo wrote:
> Also fix a bug that btrfs_read_dev_super() only reads sizeof(struct
> btrfs_super_block), corrent size should be BTRFS_SUPER_INFO_SIZE.

Actually it's correct to read only sizeof(super_block), the kernel does
it the same way. The expected in-memory format is to store the
super_block bytes and pad by zeros up to INFO_SIZE.

The checksum is calculated from that. The on-disk 4k block may contain
garbage after the super_block bytes, but it never affects the cheksum
due to the zeroing and padding.

We've got a report of mismatched checksum and bisected to this patch.

> --- a/disk-io.c
> +++ b/disk-io.c
> @@ -1186,22 +1186,25 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
>  {
>  	u8 fsid[BTRFS_FSID_SIZE];
>  	int fsid_is_initialized = 0;
> -	struct btrfs_super_block buf;
> +	u8 data[BTRFS_SUPER_INFO_SIZE];
> +	struct btrfs_super_block *buf = (struct btrfs_super_block *) data;
>  	int i;
>  	int ret;
>  	u64 transid = 0;
>  	u64 bytenr;
> +	u32 crc;
> +	char crc_result[BTRFS_CSUM_SIZE];
>  
>  	if (sb_bytenr != BTRFS_SUPER_INFO_OFFSET) {
> -		ret = pread64(fd, &buf, sizeof(buf), sb_bytenr);
> -		if (ret < sizeof(buf))
> +		ret = pread64(fd, data, sizeof(data), sb_bytenr);
> +		if (ret < sizeof(data))
>  			return -1;
>  
> -		if (btrfs_super_bytenr(&buf) != sb_bytenr ||
> -		    btrfs_super_magic(&buf) != BTRFS_MAGIC)
> +		if (btrfs_super_bytenr(buf) != sb_bytenr ||
> +		    btrfs_super_magic(buf) != BTRFS_MAGIC)
>  			return -1;
>  
> -		memcpy(sb, &buf, sizeof(*sb));
> +		memcpy(sb, data, sizeof(data));

Using a temporary buffer (data) of SIZE_INFO would be ok, untill you try
to memcpy it to the 'sb' which is sizeof(struct btrfs_super_block). So
the memcpy is overwring the memory after 'sb', the correct length of
memcpy is sizeof(*sb).

But then I don't see the point of the patch, the code would just use a
bigger temprary buffer for no apparent reason, the outcom is the same.

Re: [PATCH v2 1/4] btrfs-progs: Check superblock's checsum when read dev super

[David Sterba]

On Mon, Sep 15, 2014 at 01:36:12PM +0200, David Sterba wrote:
> We've got a report of mismatched checksum and bisected to this patch.
...
> But then I don't see the point of the patch, the code would just use a
> bigger temprary buffer for no apparent reason, the outcom is the same.

Scatch that, the patch seems in line with what kernel does and there
must be another reason of the mentioned bug.

Re: [PATCH v2 1/4] btrfs-progs: Check superblock's checsum when read dev super

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH v2 1/4] btrfs-progs: Check superblock's checsum when 
read dev super
From: David Sterba <dsterba@suse.cz>
To: <dsterba@suse.cz>, Qu Wenruo <quwenruo@cn.fujitsu.com>, 
<linux-btrfs@vger.kernel.org>
Date: 2014年09月15日 19:44
> On Mon, Sep 15, 2014 at 01:36:12PM +0200, David Sterba wrote:
>> We've got a report of mismatched checksum and bisected to this patch.
> ...
>> But then I don't see the point of the patch, the code would just use a
>> bigger temprary buffer for no apparent reason, the outcom is the same.
> Scatch that, the patch seems in line with what kernel does and there
> must be another reason of the mentioned bug.
That's OK。

But I am somewhat interesting about the report of mismatched csum.
Would you like to share some of the details?

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

[PATCH 2/4] btrfs-progs: Allow btrfs_read_dev_super() to read all 3 super for super_recover.

[Qu Wenruo]

Btrfs-progs superblock checksum check is somewhat too restricted for
super-recover, since current btrfs-progs will only read the 1st
superblock and if you need super-recover the 1st superblock is
possibly already damaged.

The fix is introducing super_recover parameter for
btrfs_read_dev_super() and callers to allow scan backup superblocks if
needed.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 btrfs-find-root.c |  4 ++--
 chunk-recover.c   |  6 +++---
 cmds-filesystem.c |  2 +-
 disk-io.c         | 17 ++++++++++-------
 disk-io.h         |  5 +++--
 super-recover.c   |  2 +-
 utils.c           | 11 ++++++-----
 volumes.c         |  4 ++--
 volumes.h         |  2 +-
 9 files changed, 29 insertions(+), 24 deletions(-)

diff --git a/btrfs-find-root.c b/btrfs-find-root.c
index 25d79f1..e31a9b5 100644
--- a/btrfs-find-root.c
+++ b/btrfs-find-root.c
@@ -82,7 +82,7 @@ static struct btrfs_root *open_ctree_broken(int fd, const char *device)
 		return NULL;
 	}
 
-	ret = btrfs_scan_fs_devices(fd, device, &fs_devices, 0, 1);
+	ret = btrfs_scan_fs_devices(fd, device, &fs_devices, 0, 1, 1);
 	if (ret)
 		goto out;
 
@@ -94,7 +94,7 @@ static struct btrfs_root *open_ctree_broken(int fd, const char *device)
 
 	disk_super = fs_info->super_copy;
 	ret = btrfs_read_dev_super(fs_devices->latest_bdev,
-				   disk_super, fs_info->super_bytenr);
+				   disk_super, fs_info->super_bytenr, 1);
 	if (ret) {
 		printk("No valid btrfs found\n");
 		goto out_devices;
diff --git a/chunk-recover.c b/chunk-recover.c
index 613d715..9baedd7 100644
--- a/chunk-recover.c
+++ b/chunk-recover.c
@@ -1283,7 +1283,7 @@ open_ctree_with_broken_chunk(struct recover_control *rc)
 
 	disk_super = fs_info->super_copy;
 	ret = btrfs_read_dev_super(fs_info->fs_devices->latest_bdev,
-				   disk_super, fs_info->super_bytenr);
+				   disk_super, fs_info->super_bytenr, 1);
 	if (ret) {
 		fprintf(stderr, "No valid btrfs found\n");
 		goto out_devices;
@@ -1349,7 +1349,7 @@ static int recover_prepare(struct recover_control *rc, char *path)
 		goto fail_close_fd;
 	}
 
-	ret = btrfs_read_dev_super(fd, sb, BTRFS_SUPER_INFO_OFFSET);
+	ret = btrfs_read_dev_super(fd, sb, BTRFS_SUPER_INFO_OFFSET, 1);
 	if (ret) {
 		fprintf(stderr, "read super block error\n");
 		goto fail_free_sb;
@@ -1368,7 +1368,7 @@ static int recover_prepare(struct recover_control *rc, char *path)
 		goto fail_free_sb;
 	}
 
-	ret = btrfs_scan_fs_devices(fd, path, &fs_devices, 0, 1);
+	ret = btrfs_scan_fs_devices(fd, path, &fs_devices, 0, 1, 1);
 	if (ret)
 		goto fail_free_sb;
 
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index 306f715..d2e46dc 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -513,7 +513,7 @@ static int dev_to_fsid(char *dev, __u8 *fsid)
 
 	disk_super = (struct btrfs_super_block *)buf;
 	ret = btrfs_read_dev_super(fd, disk_super,
-			BTRFS_SUPER_INFO_OFFSET);
+				   BTRFS_SUPER_INFO_OFFSET, 0);
 	if (ret)
 		goto out;
 
diff --git a/disk-io.c b/disk-io.c
index e447af8..1bd9fae 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -980,7 +980,7 @@ void btrfs_cleanup_all_caches(struct btrfs_fs_info *fs_info)
 
 int btrfs_scan_fs_devices(int fd, const char *path,
 			  struct btrfs_fs_devices **fs_devices,
-			  u64 sb_bytenr, int run_ioctl)
+			  u64 sb_bytenr, int run_ioctl, int super_recover)
 {
 	u64 total_devs;
 	int ret;
@@ -988,7 +988,7 @@ int btrfs_scan_fs_devices(int fd, const char *path,
 		sb_bytenr = BTRFS_SUPER_INFO_OFFSET;
 
 	ret = btrfs_scan_one_device(fd, path, fs_devices,
-				    &total_devs, sb_bytenr);
+				    &total_devs, sb_bytenr, super_recover);
 	if (ret) {
 		fprintf(stderr, "No valid Btrfs found on %s\n", path);
 		return ret;
@@ -1076,7 +1076,8 @@ static struct btrfs_fs_info *__open_ctree_fd(int fp, const char *path,
 		fs_info->on_restoring = 1;
 
 	ret = btrfs_scan_fs_devices(fp, path, &fs_devices, sb_bytenr,
-				    !(flags & OPEN_CTREE_RECOVER_SUPER));
+				    !(flags & OPEN_CTREE_RECOVER_SUPER),
+				    (flags & OPEN_CTREE_RECOVER_SUPER));
 	if (ret)
 		goto out;
 
@@ -1096,9 +1097,9 @@ static struct btrfs_fs_info *__open_ctree_fd(int fp, const char *path,
 	disk_super = fs_info->super_copy;
 	if (!(flags & OPEN_CTREE_RECOVER_SUPER))
 		ret = btrfs_read_dev_super(fs_devices->latest_bdev,
-					   disk_super, sb_bytenr);
+					   disk_super, sb_bytenr, 1);
 	else
-		ret = btrfs_read_dev_super(fp, disk_super, sb_bytenr);
+		ret = btrfs_read_dev_super(fp, disk_super, sb_bytenr, 0);
 	if (ret) {
 		printk("No valid btrfs found\n");
 		goto out_devices;
@@ -1182,7 +1183,8 @@ struct btrfs_root *open_ctree_fd(int fp, const char *path, u64 sb_bytenr,
 	return info->fs_root;
 }
 
-int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
+int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
+			 int recover_super)
 {
 	u8 fsid[BTRFS_FSID_SIZE];
 	int fsid_is_initialized = 0;
@@ -1190,6 +1192,7 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
 	struct btrfs_super_block *buf = (struct btrfs_super_block *) data;
 	int i;
 	int ret;
+	int max_super = recover_super ? BTRFS_SUPER_MIRROR_MAX : 1;
 	u64 transid = 0;
 	u64 bytenr;
 	u32 crc;
@@ -1215,7 +1218,7 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
 	* later supers, using BTRFS_SUPER_MIRROR_MAX instead
 	*/
 
-	for (i = 0; i < 1; i++) {
+	for (i = 0; i < max_super; i++) {
 		bytenr = btrfs_sb_offset(i);
 		ret = pread64(fd, data, sizeof(data), bytenr);
 		if (ret < sizeof(data))
diff --git a/disk-io.h b/disk-io.h
index 941a3da..13d4420 100644
--- a/disk-io.h
+++ b/disk-io.h
@@ -68,7 +68,7 @@ void btrfs_release_all_roots(struct btrfs_fs_info *fs_info);
 void btrfs_cleanup_all_caches(struct btrfs_fs_info *fs_info);
 int btrfs_scan_fs_devices(int fd, const char *path,
 			  struct btrfs_fs_devices **fs_devices, u64 sb_bytenr,
-			  int run_ioctl);
+			  int run_ioctl, int super_recover);
 int btrfs_setup_chunk_tree_and_device_map(struct btrfs_fs_info *fs_info);
 
 struct btrfs_root *open_ctree(const char *filename, u64 sb_bytenr,
@@ -82,7 +82,8 @@ int close_ctree(struct btrfs_root *root);
 int write_all_supers(struct btrfs_root *root);
 int write_ctree_super(struct btrfs_trans_handle *trans,
 		      struct btrfs_root *root);
-int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr);
+int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
+			 int super_recover);
 int btrfs_map_bh_to_logical(struct btrfs_root *root, struct extent_buffer *bh,
 			    u64 logical);
 struct extent_buffer *btrfs_find_tree_block(struct btrfs_root *root,
diff --git a/super-recover.c b/super-recover.c
index cd6ba37..767de4b 100644
--- a/super-recover.c
+++ b/super-recover.c
@@ -292,7 +292,7 @@ int btrfs_recover_superblocks(const char *dname,
 	}
 	init_recover_superblock(&recover);
 
-	ret = btrfs_scan_fs_devices(fd, dname, &recover.fs_devices, 0, 0);
+	ret = btrfs_scan_fs_devices(fd, dname, &recover.fs_devices, 0, 0, 1);
 	close(fd);
 	if (ret) {
 		ret = 1;
diff --git a/utils.c b/utils.c
index e130849..b0c24b1 100644
--- a/utils.c
+++ b/utils.c
@@ -1017,7 +1017,7 @@ int check_mounted_where(int fd, const char *file, char *where, int size,
 
 	/* scan the initial device */
 	ret = btrfs_scan_one_device(fd, file, &fs_devices_mnt,
-				    &total_devs, BTRFS_SUPER_INFO_OFFSET);
+				    &total_devs, BTRFS_SUPER_INFO_OFFSET, 0);
 	is_btrfs = (ret >= 0);
 
 	/* scan other devices */
@@ -1179,7 +1179,7 @@ again:
 		}
 		ret = btrfs_scan_one_device(fd, fullpath, &tmp_devices,
 					    &num_devices,
-					    BTRFS_SUPER_INFO_OFFSET);
+					    BTRFS_SUPER_INFO_OFFSET, 0);
 		if (ret == 0 && run_ioctl > 0) {
 			btrfs_register_one_device(fullpath);
 		}
@@ -1522,7 +1522,7 @@ scan_again:
 		}
 		ret = btrfs_scan_one_device(fd, fullpath, &tmp_devices,
 					    &num_devices,
-					    BTRFS_SUPER_INFO_OFFSET);
+					    BTRFS_SUPER_INFO_OFFSET, 0);
 		if (ret == 0 && run_ioctl > 0) {
 			btrfs_register_one_device(fullpath);
 		}
@@ -1696,7 +1696,8 @@ int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 		fi_args->num_devices = 1;
 
 		disk_super = (struct btrfs_super_block *)buf;
-		ret = btrfs_read_dev_super(fd, disk_super, BTRFS_SUPER_INFO_OFFSET);
+		ret = btrfs_read_dev_super(fd, disk_super,
+					   BTRFS_SUPER_INFO_OFFSET, 0);
 		if (ret < 0) {
 			ret = -EIO;
 			goto out;
@@ -2045,7 +2046,7 @@ int btrfs_scan_lblkid(int update_kernel)
 			continue;
 		}
 		ret = btrfs_scan_one_device(fd, path, &tmp_devices,
-				&num_devices, BTRFS_SUPER_INFO_OFFSET);
+				&num_devices, BTRFS_SUPER_INFO_OFFSET, 0);
 		if (ret) {
 			printf("ERROR: could not scan %s\n", path);
 			close (fd);
diff --git a/volumes.c b/volumes.c
index a61928c..fa17ce1 100644
--- a/volumes.c
+++ b/volumes.c
@@ -232,7 +232,7 @@ fail:
 
 int btrfs_scan_one_device(int fd, const char *path,
 			  struct btrfs_fs_devices **fs_devices_ret,
-			  u64 *total_devs, u64 super_offset)
+			  u64 *total_devs, u64 super_offset, int super_recover)
 {
 	struct btrfs_super_block *disk_super;
 	char *buf;
@@ -245,7 +245,7 @@ int btrfs_scan_one_device(int fd, const char *path,
 		goto error;
 	}
 	disk_super = (struct btrfs_super_block *)buf;
-	ret = btrfs_read_dev_super(fd, disk_super, super_offset);
+	ret = btrfs_read_dev_super(fd, disk_super, super_offset, super_recover);
 	if (ret < 0) {
 		ret = -EIO;
 		goto error_brelse;
diff --git a/volumes.h b/volumes.h
index b1ff3d0..70afa54 100644
--- a/volumes.h
+++ b/volumes.h
@@ -178,7 +178,7 @@ int btrfs_update_device(struct btrfs_trans_handle *trans,
 			struct btrfs_device *device);
 int btrfs_scan_one_device(int fd, const char *path,
 			  struct btrfs_fs_devices **fs_devices_ret,
-			  u64 *total_devs, u64 super_offset);
+			  u64 *total_devs, u64 super_offset, int super_recover);
 int btrfs_num_copies(struct btrfs_mapping_tree *map_tree, u64 logical, u64 len);
 struct list_head *btrfs_scanned_uuids(void);
 int btrfs_add_system_chunk(struct btrfs_trans_handle *trans,
-- 
2.0.1

Re: [PATCH 2/4] btrfs-progs: Allow btrfs_read_dev_super() to read all 3 super for super_recover.

[David Sterba]

On Thu, Jul 03, 2014 at 05:36:36PM +0800, Qu Wenruo wrote:
> @@ -1182,7 +1183,8 @@ struct btrfs_root *open_ctree_fd(int fp, const char *path, u64 sb_bytenr,
>  	return info->fs_root;
>  }
>  
> -int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
> +int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
> +			 int recover_super)

> +	int max_super = recover_super ? BTRFS_SUPER_MIRROR_MAX : 1;

Minor tweak, I've renamed it to super_recover as this is used everywhere
else. No need to resend the patch.

[PATCH 3/4] btrfs-progs: Add more meaningful return value for btrfs_read_dev_super() and corresponding error string.

[Qu Wenruo]

Since btrfs_read_dev_super() now can distinguish non-btrfs fs and
corrupted superblock thanks for the newly introduced super csum check,
the return value and corresponding error string should also be updated
to print more meaningful errors for end users.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 btrfs-find-root.c |  5 ++++-
 chunk-recover.c   | 10 ++++++++--
 cmds-filesystem.c |  7 ++++++-
 disk-io.c         | 36 ++++++++++++++++++++++++++----------
 utils.c           |  5 ++++-
 volumes.c         |  4 +---
 6 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/btrfs-find-root.c b/btrfs-find-root.c
index e31a9b5..f3bf452 100644
--- a/btrfs-find-root.c
+++ b/btrfs-find-root.c
@@ -96,7 +96,10 @@ static struct btrfs_root *open_ctree_broken(int fd, const char *device)
 	ret = btrfs_read_dev_super(fs_devices->latest_bdev,
 				   disk_super, fs_info->super_bytenr, 1);
 	if (ret) {
-		printk("No valid btrfs found\n");
+		if (ret == -ENOENT)
+			printk("No valid btrfs found\n");
+		if (ret == -EIO)
+			printk("Superblock is corrupted\n");
 		goto out_devices;
 	}
 
diff --git a/chunk-recover.c b/chunk-recover.c
index 9baedd7..c8badf9 100644
--- a/chunk-recover.c
+++ b/chunk-recover.c
@@ -1285,7 +1285,10 @@ open_ctree_with_broken_chunk(struct recover_control *rc)
 	ret = btrfs_read_dev_super(fs_info->fs_devices->latest_bdev,
 				   disk_super, fs_info->super_bytenr, 1);
 	if (ret) {
-		fprintf(stderr, "No valid btrfs found\n");
+		if (ret == -ENOENT)
+			printk("No valid btrfs found\n");
+		if (ret == -EIO)
+			printk("Superblock is corrupted\n");
 		goto out_devices;
 	}
 
@@ -1351,7 +1354,10 @@ static int recover_prepare(struct recover_control *rc, char *path)
 
 	ret = btrfs_read_dev_super(fd, sb, BTRFS_SUPER_INFO_OFFSET, 1);
 	if (ret) {
-		fprintf(stderr, "read super block error\n");
+		if (ret == -ENOENT)
+			printk("No valid btrfs found\n");
+		if (ret == -EIO)
+			printk("Superblock is corrupted\n");
 		goto fail_free_sb;
 	}
 
diff --git a/cmds-filesystem.c b/cmds-filesystem.c
index d2e46dc..d58397d 100644
--- a/cmds-filesystem.c
+++ b/cmds-filesystem.c
@@ -604,9 +604,14 @@ static int cmd_show(int argc, char **argv)
 				} else {
 					ret = dev_to_fsid(search, fsid);
 					if (ret) {
-						fprintf(stderr,
+						if (ret == -ENOENT)
+							fprintf(stderr,
 							"ERROR: No btrfs on %s\n",
 							search);
+						if (ret == -EIO)
+							fprintf(stderr,
+							"Superblock is corrupted on %s\n",
+							search);
 						return 1;
 					}
 					uuid_unparse(fsid, uuid_buf);
diff --git a/disk-io.c b/disk-io.c
index 1bd9fae..4cc831b 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -990,7 +990,11 @@ int btrfs_scan_fs_devices(int fd, const char *path,
 	ret = btrfs_scan_one_device(fd, path, fs_devices,
 				    &total_devs, sb_bytenr, super_recover);
 	if (ret) {
-		fprintf(stderr, "No valid Btrfs found on %s\n", path);
+		if (ret == -ENOENT)
+			fprintf(stderr, "No valid Btrfs found on %s\n", path);
+		if (ret == -EIO)
+			fprintf(stderr, "Superblock is corrupted on %s\n",
+				path);
 		return ret;
 	}
 
@@ -1101,7 +1105,10 @@ static struct btrfs_fs_info *__open_ctree_fd(int fp, const char *path,
 	else
 		ret = btrfs_read_dev_super(fp, disk_super, sb_bytenr, 0);
 	if (ret) {
-		printk("No valid btrfs found\n");
+		if (ret == -ENOENT)
+			printk("No valid btrfs found\n");
+		if (ret == -EIO)
+			printk("Superblock is corrupted\n");
 		goto out_devices;
 	}
 
@@ -1201,11 +1208,11 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
 	if (sb_bytenr != BTRFS_SUPER_INFO_OFFSET) {
 		ret = pread64(fd, data, sizeof(data), sb_bytenr);
 		if (ret < sizeof(data))
-			return -1;
+			return -EIO;
 
 		if (btrfs_super_bytenr(buf) != sb_bytenr ||
 		    btrfs_super_magic(buf) != BTRFS_MAGIC)
-			return -1;
+			return -ENOENT;
 
 		memcpy(sb, data, sizeof(data));
 		return 0;
@@ -1221,16 +1228,22 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
 	for (i = 0; i < max_super; i++) {
 		bytenr = btrfs_sb_offset(i);
 		ret = pread64(fd, data, sizeof(data), bytenr);
-		if (ret < sizeof(data))
+		if (ret < sizeof(data)) {
+			ret = -EIO;
 			break;
+		}
 
-		if (btrfs_super_bytenr(buf) != bytenr)
+		if (btrfs_super_bytenr(buf) != bytenr) {
+			ret = -EIO;
 			continue;
+		}
 		/* if first super block is not btrfs, the device was removed */
 		if (btrfs_super_magic(buf) != BTRFS_MAGIC && i == 0)
-			return -1;
-		if (btrfs_super_magic(buf) != BTRFS_MAGIC)
+			return -ENOENT;
+		if (btrfs_super_magic(buf) != BTRFS_MAGIC) {
+			ret = -ENOENT;
 			continue;
+		}
 
 		/* check if the superblock is damaged */
 		crc = ~(u32)0;
@@ -1238,8 +1251,10 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
 				      crc, BTRFS_SUPER_INFO_SIZE -
 				      BTRFS_CSUM_SIZE);
 		btrfs_csum_final(crc, crc_result);
-		if (memcmp(crc_result, buf, btrfs_super_csum_size(buf)))
+		if (memcmp(crc_result, buf, btrfs_super_csum_size(buf))) {
+			ret = -EIO;
 			continue;
+		}
 
 		if (!fsid_is_initialized) {
 			memcpy(fsid, buf->fsid, sizeof(fsid));
@@ -1250,6 +1265,7 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
 			 * its backups) contain data of different
 			 * filesystems -> the super cannot be trusted
 			 */
+			ret = -EIO;
 			continue;
 		}
 
@@ -1259,7 +1275,7 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr,
 		}
 	}
 
-	return transid > 0 ? 0 : -1;
+	return transid > 0 ? 0 : ret;
 }
 
 static int write_dev_supers(struct btrfs_root *root,
diff --git a/utils.c b/utils.c
index b0c24b1..9071be3 100644
--- a/utils.c
+++ b/utils.c
@@ -1699,7 +1699,10 @@ int get_fs_info(char *path, struct btrfs_ioctl_fs_info_args *fi_args,
 		ret = btrfs_read_dev_super(fd, disk_super,
 					   BTRFS_SUPER_INFO_OFFSET, 0);
 		if (ret < 0) {
-			ret = -EIO;
+			if (ret == -ENOENT)
+				fprintf(stderr, "No valid btrfs found\n");
+			if (ret == -EIO)
+				fprintf(stderr, "Superblock is corrupted\n");
 			goto out;
 		}
 		devid = btrfs_stack_device_id(&disk_super->dev_item);
diff --git a/volumes.c b/volumes.c
index fa17ce1..f10fc95 100644
--- a/volumes.c
+++ b/volumes.c
@@ -246,10 +246,8 @@ int btrfs_scan_one_device(int fd, const char *path,
 	}
 	disk_super = (struct btrfs_super_block *)buf;
 	ret = btrfs_read_dev_super(fd, disk_super, super_offset, super_recover);
-	if (ret < 0) {
-		ret = -EIO;
+	if (ret < 0)
 		goto error_brelse;
-	}
 	devid = btrfs_stack_device_id(&disk_super->dev_item);
 	if (btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_METADUMP)
 		*total_devs = 1;
-- 
2.0.1

Re: [PATCH 3/4] btrfs-progs: Add more meaningful return value for btrfs_read_dev_super() and corresponding error string.

[David Sterba]

On Thu, Jul 03, 2014 at 05:36:37PM +0800, Qu Wenruo wrote:
> --- a/btrfs-find-root.c
> +++ b/btrfs-find-root.c
> @@ -96,7 +96,10 @@ static struct btrfs_root *open_ctree_broken(int fd, const char *device)
>  	ret = btrfs_read_dev_super(fs_devices->latest_bdev,
>  				   disk_super, fs_info->super_bytenr, 1);
>  	if (ret) {
> -		printk("No valid btrfs found\n");
> +		if (ret == -ENOENT)
> +			printk("No valid btrfs found\n");

Please use fprintf(stderr, ...) for error messages, until we have a
better message logging helpers.

[PATCH 4/4] btrfs-progs: Fix malloc size for superblock.

[Qu Wenruo]

recover_prepare() in chunk-recover.c alloc memory which only contains
sizeof(struct btrfs_super_block). This will cause glibc malloc error
after superblock csum is calculated.

Use BTRFS_SUPER_INFO_SIZE to fix the bug.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
 chunk-recover.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/chunk-recover.c b/chunk-recover.c
index c8badf9..7dfaf82 100644
--- a/chunk-recover.c
+++ b/chunk-recover.c
@@ -1345,7 +1345,7 @@ static int recover_prepare(struct recover_control *rc, char *path)
 		return -1;
 	}
 
-	sb = malloc(sizeof(struct btrfs_super_block));
+	sb = malloc(BTRFS_SUPER_INFO_SIZE);
 	if (!sb) {
 		fprintf(stderr, "allocating memory for sb failed.\n");
 		ret = -ENOMEM;
-- 
2.0.1

Re: [PATCH 0/4] Add superblock checksum check for btrfs-progs

[David Sterba]

On Thu, Jul 03, 2014 at 05:36:34PM +0800, Qu Wenruo wrote:
> Before this patchset, btrfs-progs will overall ignore the superblock
> checksum and continue the routine.
> Sometimes this may cause disasters like checking a btrfs with corrupted
> superblock will lead to crash in btrfs-progs.
> 
> This patch introduces superblock checksum check into btrfs_read_dev_super(),
> making btrfs-progs much more restricted and robust.
> To allow super-recover to open devices, add options to scan all 3
> superblocks when using super-recover.
> Also updated the related error string and fix a bug in chunk-recover that
> will not be triggered until superblock csum is calculated.
> 
> Qu Wenruo (4):
>   btrfs-progs: Check superblock's checsum when read dev super
>   btrfs-progs: Allow btrfs_read_dev_super() to read all 3 super for    
>     super_recover.
>   btrfs-progs: Add more meaningful return value for
>     btrfs_read_dev_super()     and corresponding error string.
>   btrfs-progs: Fix size for malloc for superblock checksum.

Nice work. I've added 1, 2 and 4 it to integration. Please update the
patch 3 (printf/fprintf).

Re: [PATCH 0/4] Add superblock checksum check for btrfs-progs

[Qu Wenruo]

-------- Original Message --------
Subject: Re: [PATCH 0/4] Add superblock checksum check for btrfs-progs
From: David Sterba <dsterba@suse.cz>
To: Qu Wenruo <quwenruo@cn.fujitsu.com>
Date: 2014年07月04日 01:57
> On Thu, Jul 03, 2014 at 05:36:34PM +0800, Qu Wenruo wrote:
>> Before this patchset, btrfs-progs will overall ignore the superblock
>> checksum and continue the routine.
>> Sometimes this may cause disasters like checking a btrfs with corrupted
>> superblock will lead to crash in btrfs-progs.
>>
>> This patch introduces superblock checksum check into btrfs_read_dev_super(),
>> making btrfs-progs much more restricted and robust.
>> To allow super-recover to open devices, add options to scan all 3
>> superblocks when using super-recover.
>> Also updated the related error string and fix a bug in chunk-recover that
>> will not be triggered until superblock csum is calculated.
>>
>> Qu Wenruo (4):
>>    btrfs-progs: Check superblock's checsum when read dev super
>>    btrfs-progs: Allow btrfs_read_dev_super() to read all 3 super for
>>      super_recover.
>>    btrfs-progs: Add more meaningful return value for
>>      btrfs_read_dev_super()     and corresponding error string.
>>    btrfs-progs: Fix size for malloc for superblock checksum.
> Nice work. I've added 1, 2 and 4 it to integration. Please update the
> patch 3 (printf/fprintf).
Thanks for the review and minor tweak for patch 2.

I'll send v2 version of patch 3 soon.

Thank,
Qu

[PATCH 0/4] Add superblock checksum check for btrfs-progs

[Qu Wenruo]

Before this patchset, btrfs-progs will overall ignore the superblock
checksum and continue the routine.
Sometimes this may cause disasters like checking a btrfs with corrupted
superblock will lead to crash in btrfs-progs.

This patch introduces superblock checksum check into btrfs_read_dev_super(),
making btrfs-progs much more restricted and robust.
To allow super-recover to open devices, add options to scan all 3
superblocks when using super-recover.
Also updated the related error string and fix a bug in chunk-recover that
will not be triggered until superblock csum is calculated.

Qu Wenruo (4):
  btrfs-progs: Check superblock's checsum when read dev super
  btrfs-progs: Allow btrfs_read_dev_super() to read all 3 super for    
    super_recover.
  btrfs-progs: Add more meaningful return value for
    btrfs_read_dev_super()     and corresponding error string.
  btrfs-progs: Fix size for malloc for superblock checksum.

 btrfs-find-root.c |  9 ++++--
 chunk-recover.c   | 18 +++++++----
 cmds-filesystem.c |  9 ++++--
 disk-io.c         | 91 +++++++++++++++++++++++++++++++++++++------------------
 disk-io.h         |  5 +--
 super-recover.c   |  2 +-
 utils.c           | 16 ++++++----
 volumes.c         |  8 ++---
 volumes.h         |  2 +-
 9 files changed, 104 insertions(+), 56 deletions(-)

-- 
2.0.1

[PATCH v2 1/4] btrfs-progs: Check superblock's checsum when read dev super

[Qu Wenruo]

Btrfs-progs will read the superblock without checking the checksum.
When all superblocks are corrupted, continuing will cause disaster.

So this patch will add checksum check for btrfs-progs when reading
superblocks.

Also fix a bug that btrfs_read_dev_super() only reads sizeof(struct
btrfs_super_block), corrent size should be BTRFS_SUPER_INFO_SIZE.

Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
---
v2:
  Use corrent memcmp src.
  Read the whole supblock size(sectorsize) other than
  sizeof(btrfs_super_block).
---
 disk-io.c | 46 +++++++++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 17 deletions(-)

diff --git a/disk-io.c b/disk-io.c
index 8db0335..e447af8 100644
--- a/disk-io.c
+++ b/disk-io.c
@@ -1186,22 +1186,25 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
 {
 	u8 fsid[BTRFS_FSID_SIZE];
 	int fsid_is_initialized = 0;
-	struct btrfs_super_block buf;
+	u8 data[BTRFS_SUPER_INFO_SIZE];
+	struct btrfs_super_block *buf = (struct btrfs_super_block *) data;
 	int i;
 	int ret;
 	u64 transid = 0;
 	u64 bytenr;
+	u32 crc;
+	char crc_result[BTRFS_CSUM_SIZE];
 
 	if (sb_bytenr != BTRFS_SUPER_INFO_OFFSET) {
-		ret = pread64(fd, &buf, sizeof(buf), sb_bytenr);
-		if (ret < sizeof(buf))
+		ret = pread64(fd, data, sizeof(data), sb_bytenr);
+		if (ret < sizeof(data))
 			return -1;
 
-		if (btrfs_super_bytenr(&buf) != sb_bytenr ||
-		    btrfs_super_magic(&buf) != BTRFS_MAGIC)
+		if (btrfs_super_bytenr(buf) != sb_bytenr ||
+		    btrfs_super_magic(buf) != BTRFS_MAGIC)
 			return -1;
 
-		memcpy(sb, &buf, sizeof(*sb));
+		memcpy(sb, data, sizeof(data));
 		return 0;
 	}
 
@@ -1214,22 +1217,31 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
 
 	for (i = 0; i < 1; i++) {
 		bytenr = btrfs_sb_offset(i);
-		ret = pread64(fd, &buf, sizeof(buf), bytenr);
-		if (ret < sizeof(buf))
+		ret = pread64(fd, data, sizeof(data), bytenr);
+		if (ret < sizeof(data))
 			break;
 
-		if (btrfs_super_bytenr(&buf) != bytenr )
+		if (btrfs_super_bytenr(buf) != bytenr)
 			continue;
-		/* if magic is NULL, the device was removed */
-		if (btrfs_super_magic(&buf) == 0 && i == 0)
+		/* if first super block is not btrfs, the device was removed */
+		if (btrfs_super_magic(buf) != BTRFS_MAGIC && i == 0)
 			return -1;
-		if (btrfs_super_magic(&buf) != BTRFS_MAGIC)
+		if (btrfs_super_magic(buf) != BTRFS_MAGIC)
+			continue;
+
+		/* check if the superblock is damaged */
+		crc = ~(u32)0;
+		crc = btrfs_csum_data(NULL, (char *)buf + BTRFS_CSUM_SIZE,
+				      crc, BTRFS_SUPER_INFO_SIZE -
+				      BTRFS_CSUM_SIZE);
+		btrfs_csum_final(crc, crc_result);
+		if (memcmp(crc_result, buf, btrfs_super_csum_size(buf)))
 			continue;
 
 		if (!fsid_is_initialized) {
-			memcpy(fsid, buf.fsid, sizeof(fsid));
+			memcpy(fsid, buf->fsid, sizeof(fsid));
 			fsid_is_initialized = 1;
-		} else if (memcmp(fsid, buf.fsid, sizeof(fsid))) {
+		} else if (memcmp(fsid, buf->fsid, sizeof(fsid))) {
 			/*
 			 * the superblocks (the original one and
 			 * its backups) contain data of different
@@ -1238,9 +1250,9 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr)
 			continue;
 		}
 
-		if (btrfs_super_generation(&buf) > transid) {
-			memcpy(sb, &buf, sizeof(*sb));
-			transid = btrfs_super_generation(&buf);
+		if (btrfs_super_generation(buf) > transid) {
+			memcpy(sb, data, sizeof(data));
+			transid = btrfs_super_generation(buf);
 		}
 	}
 
-- 
2.0.1