linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] btrfs: Validate generation of btrfs super blocks before writing it to disk
@ 2018-12-25  4:50 Qu Wenruo
  2018-12-25  5:36 ` Qu Wenruo
  2018-12-26  2:08 ` Qu Wenruo
  0 siblings, 2 replies; 3+ messages in thread
From: Qu Wenruo @ 2018-12-25  4:50 UTC (permalink / raw)
  To: linux-btrfs; +Cc: Peter Chant

Even we did super block verification in commit 75cb857d2618
("btrfs: Do super block verification before writing it to disk"), it
doesn't check generation against transaction as generation corruption is
not that obvious.
This allows super block generation corruption sneak in.

So in this patch, btrfs will check super block generation by:
- Check it against expected transid
  We have 2 different callers who writes all super blocks:
  * btrfs_commit_transaction()
    The expected generation is trans->transid.
  * btrfs_sync_log()
    The expected generation is trans->transid - 1, since we only
    update log_root, while not updating generation.

- Chech it against backup roots
  For backup roots, we should not have any backup roots newer than our
  current expected generation.

  NOTE: Normally we should have a backup root matching the expected
        generation, but btrfs-progs doesn't update backup roots, which
        makes a valid repaired fs undergoing fsync() to trigger false
        alerts. So we don't require backup roots to match generation
        yet.

With above solution, at least we should be able to catch potential
generation corruption early on.

Reported-by: Peter Chant <pete@petezilla.co.uk>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c     | 40 +++++++++++++++++++++++++++++++++++++---
 fs/btrfs/disk-io.h     |  2 +-
 fs/btrfs/transaction.c |  2 +-
 fs/btrfs/tree-log.c    |  2 +-
 4 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 5228320030a5..e9caf199e579 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2573,9 +2573,12 @@ static int btrfs_validate_mount_super(struct btrfs_fs_info *fs_info)
  * Extra checks like csum type and incompat flags will be done here.
  */
 static int btrfs_validate_write_super(struct btrfs_fs_info *fs_info,
-				      struct btrfs_super_block *sb)
+				      struct btrfs_super_block *sb,
+				      u64 expected_gen)
 {
+	u64 sb_gen = btrfs_super_generation(sb);
 	int ret;
+	int i;
 
 	ret = validate_super(fs_info, sb, -1);
 	if (ret < 0)
@@ -2594,6 +2597,37 @@ static int btrfs_validate_write_super(struct btrfs_fs_info *fs_info,
 			  (unsigned long long)BTRFS_FEATURE_INCOMPAT_SUPP);
 		goto out;
 	}
+
+	/* super block generation check */
+	if (sb_gen != expected_gen) {
+		ret = -EUCLEAN;
+		btrfs_err(fs_info,
+			"invalid generation detected, has %llu expect %llu",
+			sb_gen, expected_gen);
+		goto out;
+	}
+
+	/*
+	 * check against backup roots
+	 *
+	 * NOTE: Normally we should have a backup root matching the expected
+	 * generation, however for fsync() call on a btrfs-progs modified fs,
+	 * its backup roots aren't updated and could cause false alerts.
+	 * So here we only check obvious corrupted backup roots.
+	 */
+	for (i = 0; i < BTRFS_NUM_BACKUP_ROOTS; i++) {
+		struct btrfs_root_backup *root_backup = sb->super_roots + i;
+		u64 backup_gen = btrfs_backup_tree_root_gen(root_backup);
+
+		if (backup_gen > sb_gen) {
+			ret = -EUCLEAN;
+			btrfs_err(fs_info,
+	"invalid backup root generation detected, slot %u has %llu expect <%llu",
+				  i, backup_gen, sb_gen);
+			goto out;
+		}
+	}
+
 out:
 	if (ret < 0)
 		btrfs_err(fs_info,
@@ -3721,7 +3755,7 @@ int btrfs_get_num_tolerated_disk_barrier_failures(u64 flags)
 	return min_tolerated;
 }
 
-int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
+int write_all_supers(struct btrfs_fs_info *fs_info, u64 gen, int max_mirrors)
 {
 	struct list_head *head;
 	struct btrfs_device *dev;
@@ -3786,7 +3820,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
 		flags = btrfs_super_flags(sb);
 		btrfs_set_super_flags(sb, flags | BTRFS_HEADER_FLAG_WRITTEN);
 
-		ret = btrfs_validate_write_super(fs_info, sb);
+		ret = btrfs_validate_write_super(fs_info, sb, gen);
 		if (ret < 0) {
 			mutex_unlock(&fs_info->fs_devices->device_list_mutex);
 			btrfs_handle_fs_error(fs_info, -EUCLEAN,
diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
index 4cccba22640f..9f8fd0a9a3a4 100644
--- a/fs/btrfs/disk-io.h
+++ b/fs/btrfs/disk-io.h
@@ -53,7 +53,7 @@ int open_ctree(struct super_block *sb,
 	       struct btrfs_fs_devices *fs_devices,
 	       char *options);
 void close_ctree(struct btrfs_fs_info *fs_info);
-int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors);
+int write_all_supers(struct btrfs_fs_info *fs_info, u64 gen, int max_mirrors);
 struct buffer_head *btrfs_read_dev_super(struct block_device *bdev);
 int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num,
 			struct buffer_head **bh_ret);
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index d1eeef9ec5da..b61cec2780b9 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -2241,7 +2241,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
 		goto scrub_continue;
 	}
 
-	ret = write_all_supers(fs_info, 0);
+	ret = write_all_supers(fs_info, trans->transid, 0);
 	/*
 	 * the super is written, we can safely allow the tree-loggers
 	 * to go about their business
diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
index e07f3376b7df..4f84345fdbb6 100644
--- a/fs/btrfs/tree-log.c
+++ b/fs/btrfs/tree-log.c
@@ -3155,7 +3155,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
 	 * the running transaction open, so a full commit can't hop
 	 * in and cause problems either.
 	 */
-	ret = write_all_supers(fs_info, 1);
+	ret = write_all_supers(fs_info, trans->transid - 1, 1);
 	if (ret) {
 		btrfs_set_log_full_commit(fs_info, trans);
 		btrfs_abort_transaction(trans, ret);
-- 
2.20.1


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] btrfs: Validate generation of btrfs super blocks before writing it to disk
  2018-12-25  4:50 [PATCH v2] btrfs: Validate generation of btrfs super blocks before writing it to disk Qu Wenruo
@ 2018-12-25  5:36 ` Qu Wenruo
  2018-12-26  2:08 ` Qu Wenruo
  1 sibling, 0 replies; 3+ messages in thread
From: Qu Wenruo @ 2018-12-25  5:36 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs; +Cc: Peter Chant


[-- Attachment #1.1: Type: text/plain, Size: 6297 bytes --]



On 2018/12/25 下午12:50, Qu Wenruo wrote:
> Even we did super block verification in commit 75cb857d2618
> ("btrfs: Do super block verification before writing it to disk"), it
> doesn't check generation against transaction as generation corruption is
> not that obvious.
> This allows super block generation corruption sneak in.
> 
> So in this patch, btrfs will check super block generation by:
> - Check it against expected transid
>   We have 2 different callers who writes all super blocks:
>   * btrfs_commit_transaction()
>     The expected generation is trans->transid.
>   * btrfs_sync_log()
>     The expected generation is trans->transid - 1, since we only
>     update log_root, while not updating generation.
> 
> - Chech it against backup roots
>   For backup roots, we should not have any backup roots newer than our
>   current expected generation.
> 
>   NOTE: Normally we should have a backup root matching the expected
>         generation, but btrfs-progs doesn't update backup roots, which
>         makes a valid repaired fs undergoing fsync() to trigger false
>         alerts. So we don't require backup roots to match generation
>         yet.
> 
> With above solution, at least we should be able to catch potential
> generation corruption early on.
> 
> Reported-by: Peter Chant <pete@petezilla.co.uk>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
Sorry I missed the changelog.

Changelog:
v2:
- Remove the requirement for backup root to match expected generation.
  This is for btrfs-progs modified fs with fsync() call.

Thanks,
Qu

>  fs/btrfs/disk-io.c     | 40 +++++++++++++++++++++++++++++++++++++---
>  fs/btrfs/disk-io.h     |  2 +-
>  fs/btrfs/transaction.c |  2 +-
>  fs/btrfs/tree-log.c    |  2 +-
>  4 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 5228320030a5..e9caf199e579 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2573,9 +2573,12 @@ static int btrfs_validate_mount_super(struct btrfs_fs_info *fs_info)
>   * Extra checks like csum type and incompat flags will be done here.
>   */
>  static int btrfs_validate_write_super(struct btrfs_fs_info *fs_info,
> -				      struct btrfs_super_block *sb)
> +				      struct btrfs_super_block *sb,
> +				      u64 expected_gen)
>  {
> +	u64 sb_gen = btrfs_super_generation(sb);
>  	int ret;
> +	int i;
>  
>  	ret = validate_super(fs_info, sb, -1);
>  	if (ret < 0)
> @@ -2594,6 +2597,37 @@ static int btrfs_validate_write_super(struct btrfs_fs_info *fs_info,
>  			  (unsigned long long)BTRFS_FEATURE_INCOMPAT_SUPP);
>  		goto out;
>  	}
> +
> +	/* super block generation check */
> +	if (sb_gen != expected_gen) {
> +		ret = -EUCLEAN;
> +		btrfs_err(fs_info,
> +			"invalid generation detected, has %llu expect %llu",
> +			sb_gen, expected_gen);
> +		goto out;
> +	}
> +
> +	/*
> +	 * check against backup roots
> +	 *
> +	 * NOTE: Normally we should have a backup root matching the expected
> +	 * generation, however for fsync() call on a btrfs-progs modified fs,
> +	 * its backup roots aren't updated and could cause false alerts.
> +	 * So here we only check obvious corrupted backup roots.
> +	 */
> +	for (i = 0; i < BTRFS_NUM_BACKUP_ROOTS; i++) {
> +		struct btrfs_root_backup *root_backup = sb->super_roots + i;
> +		u64 backup_gen = btrfs_backup_tree_root_gen(root_backup);
> +
> +		if (backup_gen > sb_gen) {
> +			ret = -EUCLEAN;
> +			btrfs_err(fs_info,
> +	"invalid backup root generation detected, slot %u has %llu expect <%llu",
> +				  i, backup_gen, sb_gen);
> +			goto out;
> +		}
> +	}
> +
>  out:
>  	if (ret < 0)
>  		btrfs_err(fs_info,
> @@ -3721,7 +3755,7 @@ int btrfs_get_num_tolerated_disk_barrier_failures(u64 flags)
>  	return min_tolerated;
>  }
>  
> -int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
> +int write_all_supers(struct btrfs_fs_info *fs_info, u64 gen, int max_mirrors)
>  {
>  	struct list_head *head;
>  	struct btrfs_device *dev;
> @@ -3786,7 +3820,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
>  		flags = btrfs_super_flags(sb);
>  		btrfs_set_super_flags(sb, flags | BTRFS_HEADER_FLAG_WRITTEN);
>  
> -		ret = btrfs_validate_write_super(fs_info, sb);
> +		ret = btrfs_validate_write_super(fs_info, sb, gen);
>  		if (ret < 0) {
>  			mutex_unlock(&fs_info->fs_devices->device_list_mutex);
>  			btrfs_handle_fs_error(fs_info, -EUCLEAN,
> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
> index 4cccba22640f..9f8fd0a9a3a4 100644
> --- a/fs/btrfs/disk-io.h
> +++ b/fs/btrfs/disk-io.h
> @@ -53,7 +53,7 @@ int open_ctree(struct super_block *sb,
>  	       struct btrfs_fs_devices *fs_devices,
>  	       char *options);
>  void close_ctree(struct btrfs_fs_info *fs_info);
> -int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors);
> +int write_all_supers(struct btrfs_fs_info *fs_info, u64 gen, int max_mirrors);
>  struct buffer_head *btrfs_read_dev_super(struct block_device *bdev);
>  int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num,
>  			struct buffer_head **bh_ret);
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index d1eeef9ec5da..b61cec2780b9 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -2241,7 +2241,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  		goto scrub_continue;
>  	}
>  
> -	ret = write_all_supers(fs_info, 0);
> +	ret = write_all_supers(fs_info, trans->transid, 0);
>  	/*
>  	 * the super is written, we can safely allow the tree-loggers
>  	 * to go about their business
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index e07f3376b7df..4f84345fdbb6 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3155,7 +3155,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
>  	 * the running transaction open, so a full commit can't hop
>  	 * in and cause problems either.
>  	 */
> -	ret = write_all_supers(fs_info, 1);
> +	ret = write_all_supers(fs_info, trans->transid - 1, 1);
>  	if (ret) {
>  		btrfs_set_log_full_commit(fs_info, trans);
>  		btrfs_abort_transaction(trans, ret);
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH v2] btrfs: Validate generation of btrfs super blocks before writing it to disk
  2018-12-25  4:50 [PATCH v2] btrfs: Validate generation of btrfs super blocks before writing it to disk Qu Wenruo
  2018-12-25  5:36 ` Qu Wenruo
@ 2018-12-26  2:08 ` Qu Wenruo
  1 sibling, 0 replies; 3+ messages in thread
From: Qu Wenruo @ 2018-12-26  2:08 UTC (permalink / raw)
  To: Qu Wenruo, linux-btrfs; +Cc: Peter Chant


[-- Attachment #1.1: Type: text/plain, Size: 6340 bytes --]

Please discard this patch, such generation check can't handle
concurrency in snapshot creation.

And since the original report on generation mismatch is completely
caused by btrfs-progs, such check has no real-world need.

Thanks,
Qu

On 2018/12/25 下午12:50, Qu Wenruo wrote:
> Even we did super block verification in commit 75cb857d2618
> ("btrfs: Do super block verification before writing it to disk"), it
> doesn't check generation against transaction as generation corruption is
> not that obvious.
> This allows super block generation corruption sneak in.
> 
> So in this patch, btrfs will check super block generation by:
> - Check it against expected transid
>   We have 2 different callers who writes all super blocks:
>   * btrfs_commit_transaction()
>     The expected generation is trans->transid.
>   * btrfs_sync_log()
>     The expected generation is trans->transid - 1, since we only
>     update log_root, while not updating generation.
> 
> - Chech it against backup roots
>   For backup roots, we should not have any backup roots newer than our
>   current expected generation.
> 
>   NOTE: Normally we should have a backup root matching the expected
>         generation, but btrfs-progs doesn't update backup roots, which
>         makes a valid repaired fs undergoing fsync() to trigger false
>         alerts. So we don't require backup roots to match generation
>         yet.
> 
> With above solution, at least we should be able to catch potential
> generation corruption early on.
> 
> Reported-by: Peter Chant <pete@petezilla.co.uk>
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/disk-io.c     | 40 +++++++++++++++++++++++++++++++++++++---
>  fs/btrfs/disk-io.h     |  2 +-
>  fs/btrfs/transaction.c |  2 +-
>  fs/btrfs/tree-log.c    |  2 +-
>  4 files changed, 40 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 5228320030a5..e9caf199e579 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2573,9 +2573,12 @@ static int btrfs_validate_mount_super(struct btrfs_fs_info *fs_info)
>   * Extra checks like csum type and incompat flags will be done here.
>   */
>  static int btrfs_validate_write_super(struct btrfs_fs_info *fs_info,
> -				      struct btrfs_super_block *sb)
> +				      struct btrfs_super_block *sb,
> +				      u64 expected_gen)
>  {
> +	u64 sb_gen = btrfs_super_generation(sb);
>  	int ret;
> +	int i;
>  
>  	ret = validate_super(fs_info, sb, -1);
>  	if (ret < 0)
> @@ -2594,6 +2597,37 @@ static int btrfs_validate_write_super(struct btrfs_fs_info *fs_info,
>  			  (unsigned long long)BTRFS_FEATURE_INCOMPAT_SUPP);
>  		goto out;
>  	}
> +
> +	/* super block generation check */
> +	if (sb_gen != expected_gen) {
> +		ret = -EUCLEAN;
> +		btrfs_err(fs_info,
> +			"invalid generation detected, has %llu expect %llu",
> +			sb_gen, expected_gen);
> +		goto out;
> +	}
> +
> +	/*
> +	 * check against backup roots
> +	 *
> +	 * NOTE: Normally we should have a backup root matching the expected
> +	 * generation, however for fsync() call on a btrfs-progs modified fs,
> +	 * its backup roots aren't updated and could cause false alerts.
> +	 * So here we only check obvious corrupted backup roots.
> +	 */
> +	for (i = 0; i < BTRFS_NUM_BACKUP_ROOTS; i++) {
> +		struct btrfs_root_backup *root_backup = sb->super_roots + i;
> +		u64 backup_gen = btrfs_backup_tree_root_gen(root_backup);
> +
> +		if (backup_gen > sb_gen) {
> +			ret = -EUCLEAN;
> +			btrfs_err(fs_info,
> +	"invalid backup root generation detected, slot %u has %llu expect <%llu",
> +				  i, backup_gen, sb_gen);
> +			goto out;
> +		}
> +	}
> +
>  out:
>  	if (ret < 0)
>  		btrfs_err(fs_info,
> @@ -3721,7 +3755,7 @@ int btrfs_get_num_tolerated_disk_barrier_failures(u64 flags)
>  	return min_tolerated;
>  }
>  
> -int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
> +int write_all_supers(struct btrfs_fs_info *fs_info, u64 gen, int max_mirrors)
>  {
>  	struct list_head *head;
>  	struct btrfs_device *dev;
> @@ -3786,7 +3820,7 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
>  		flags = btrfs_super_flags(sb);
>  		btrfs_set_super_flags(sb, flags | BTRFS_HEADER_FLAG_WRITTEN);
>  
> -		ret = btrfs_validate_write_super(fs_info, sb);
> +		ret = btrfs_validate_write_super(fs_info, sb, gen);
>  		if (ret < 0) {
>  			mutex_unlock(&fs_info->fs_devices->device_list_mutex);
>  			btrfs_handle_fs_error(fs_info, -EUCLEAN,
> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
> index 4cccba22640f..9f8fd0a9a3a4 100644
> --- a/fs/btrfs/disk-io.h
> +++ b/fs/btrfs/disk-io.h
> @@ -53,7 +53,7 @@ int open_ctree(struct super_block *sb,
>  	       struct btrfs_fs_devices *fs_devices,
>  	       char *options);
>  void close_ctree(struct btrfs_fs_info *fs_info);
> -int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors);
> +int write_all_supers(struct btrfs_fs_info *fs_info, u64 gen, int max_mirrors);
>  struct buffer_head *btrfs_read_dev_super(struct block_device *bdev);
>  int btrfs_read_dev_one_super(struct block_device *bdev, int copy_num,
>  			struct buffer_head **bh_ret);
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index d1eeef9ec5da..b61cec2780b9 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -2241,7 +2241,7 @@ int btrfs_commit_transaction(struct btrfs_trans_handle *trans)
>  		goto scrub_continue;
>  	}
>  
> -	ret = write_all_supers(fs_info, 0);
> +	ret = write_all_supers(fs_info, trans->transid, 0);
>  	/*
>  	 * the super is written, we can safely allow the tree-loggers
>  	 * to go about their business
> diff --git a/fs/btrfs/tree-log.c b/fs/btrfs/tree-log.c
> index e07f3376b7df..4f84345fdbb6 100644
> --- a/fs/btrfs/tree-log.c
> +++ b/fs/btrfs/tree-log.c
> @@ -3155,7 +3155,7 @@ int btrfs_sync_log(struct btrfs_trans_handle *trans,
>  	 * the running transaction open, so a full commit can't hop
>  	 * in and cause problems either.
>  	 */
> -	ret = write_all_supers(fs_info, 1);
> +	ret = write_all_supers(fs_info, trans->transid - 1, 1);
>  	if (ret) {
>  		btrfs_set_log_full_commit(fs_info, trans);
>  		btrfs_abort_transaction(trans, ret);
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-12-26  2:08 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-25  4:50 [PATCH v2] btrfs: Validate generation of btrfs super blocks before writing it to disk Qu Wenruo
2018-12-25  5:36 ` Qu Wenruo
2018-12-26  2:08 ` Qu Wenruo

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).