[PATCH v2 0/4] btrfs: Add write time super block validation

[PATCH v2 0/4] btrfs: Add write time super block validation

[Qu Wenruo]

This patchset can be fetched from github:
https://github.com/adam900710/linux/tree/write_time_sb_check

We have 2 reports about corrupted btrfs super block, which has some garbage
in its super block, but otherwise it's completely fine and its csum even
matches.

This means we develop memory corruption during btrfs mount time.
It's not clear whether it's caused by btrfs or some other kernel module,
but at least let's do write time verification to catch such corruption
early.

Changelog:
v2:
  Rename btrfs_check_super_valid() to btrfs_validate_super() suggested
  by Nikolay and David.

Qu Wenruo (4):
  btrfs: Rename btrfs_check_super_valid() to btrfs_validate_super()
  btrfs: Add incompat flags check for btrfs_check_super_valid()
  btrfs: Add csum type check for btrfs_check_super_valid()
  btrfs: Do super block verification before writing it to disk

 fs/btrfs/disk-io.c | 58 ++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 51 insertions(+), 7 deletions(-)

-- 
2.17.0

[PATCH v2 1/4] btrfs: Rename btrfs_check_super_valid() to btrfs_validate_super()

[Qu Wenruo]

Makes the function name a little shorter and easier to read.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 60caa68c3618..23b5c90cdbb2 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -55,7 +55,7 @@
 static const struct extent_io_ops btree_extent_io_ops;
 static void end_workqueue_fn(struct btrfs_work *work);
 static void free_fs_root(struct btrfs_root *root);
-static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info);
+static int btrfs_validate_super(struct btrfs_fs_info *fs_info);
 static void btrfs_destroy_ordered_extents(struct btrfs_root *root);
 static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
 				      struct btrfs_fs_info *fs_info);
@@ -2668,7 +2668,7 @@ int open_ctree(struct super_block *sb,
 
 	memcpy(fs_info->fsid, fs_info->super_copy->fsid, BTRFS_FSID_SIZE);
 
-	ret = btrfs_check_super_valid(fs_info);
+	ret = btrfs_validate_super(fs_info);
 	if (ret) {
 		btrfs_err(fs_info, "superblock contains fatal errors");
 		err = -EINVAL;
@@ -3974,7 +3974,7 @@ int btrfs_read_buffer(struct extent_buffer *buf, u64 parent_transid, int level,
 					      level, first_key);
 }
 
-static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
+static int btrfs_validate_super(struct btrfs_fs_info *fs_info)
 {
 	struct btrfs_super_block *sb = fs_info->super_copy;
 	u64 nodesize = btrfs_super_nodesize(sb);
-- 
2.17.0

[PATCH v2 2/4] btrfs: Add incompat flags check for btrfs_check_super_valid()

[Qu Wenruo]

Although we have already checked incompat flags manually before really
mounting it, we could still enhance btrfs_check_super_valid() to check
incompat flags for later write time super block validation check.

This patch adds such incompat flags check for btrfs_check_super_valid(),
currently it won't be triggered, but provides the basis for later write
time check.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 23b5c90cdbb2..3968f7ff8de2 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4104,6 +4104,19 @@ static int btrfs_validate_super(struct btrfs_fs_info *fs_info)
 		ret = -EINVAL;
 	}
 
+	/*
+	 * Before calling btrfs_check_super_valid() we have already checked
+	 * incompat flags. So if we developr new incompat flags, it's must be
+	 * some corruption.
+	 */
+	if (btrfs_super_incompat_flags(sb) & ~BTRFS_FEATURE_INCOMPAT_SUPP) {
+		btrfs_err(fs_info,
+		"corrupted incompat flags detected 0x%llx, supported 0x%llx",
+			  btrfs_super_incompat_flags(sb),
+			  BTRFS_FEATURE_INCOMPAT_SUPP);
+		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.
-- 
2.17.0

Re: [PATCH v2 2/4] btrfs: Add incompat flags check for btrfs_check_super_valid()

[David Sterba]

On Tue, Apr 24, 2018 at 12:48:07PM +0800, Qu Wenruo wrote:
> Although we have already checked incompat flags manually before really
> mounting it, we could still enhance btrfs_check_super_valid() to check
> incompat flags for later write time super block validation check.

But the calls are in reverse. First the validation is called and then
the incompat bits are verified if the filesystem is mountable.

With change in this patch, any uknonwn incompat bit at mount time will
be reported as corruption. This does not make sense.

I've read the discussion under previous version again, IMHO the best way
to report what's going on is to use 2 functions for mount ant pre-commit
time.

We can't expect that random user will understand that new unsupported
flags actually mean corruption or that unsupported bits at mount time
are not really a corruption.

Re: [PATCH v2 2/4] btrfs: Add incompat flags check for btrfs_check_super_valid()

[Qu Wenruo]

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



On 2018年04月24日 18:48, David Sterba wrote:
> On Tue, Apr 24, 2018 at 12:48:07PM +0800, Qu Wenruo wrote:
>> Although we have already checked incompat flags manually before really
>> mounting it, we could still enhance btrfs_check_super_valid() to check
>> incompat flags for later write time super block validation check.
> 
> But the calls are in reverse. First the validation is called and then
> the incompat bits are verified if the filesystem is mountable.
> 
> With change in this patch, any uknonwn incompat bit at mount time will
> be reported as corruption. This does not make sense.

Oh sorry, I just though incompat flags is checked before calling
btrfs_validate_super().

> 
> I've read the discussion under previous version again, IMHO the best way
> to report what's going on is to use 2 functions for mount ant pre-commit
> time.

OK, next version will go that direction.

Although it may still be a little tricky to split what we need in
btrfs_validate_super() and btrfs_precheck_super().

What about this idea:
- btrfs_precheck_super() only checks the very basis:
  * magic
  * incompat flags
  * csum type
  Any mismatch will do friendly prompt ("no btrfs detected" or
  "unsupported flags/csum type" respectively)

- btrfs_validate_super() do the comprehensive check:
  * Everything we did in this patchset
  * including magic, incompat flags and csum type
  Any mismatch will be considered as corruption.

For mount time, we call btrfs_precheck_super() then btrfs_validate_super().

For commit time, only btrfs_validate_super().

How about this solution?

Thanks,
Qu
> 
> We can't expect that random user will understand that new unsupported
> flags actually mean corruption or that unsupported bits at mount time
> are not really a corruption.
> --
> 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
> 


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

Re: [PATCH v2 2/4] btrfs: Add incompat flags check for btrfs_check_super_valid()

[David Sterba]

On Tue, Apr 24, 2018 at 07:28:27PM +0800, Qu Wenruo wrote:
> > I've read the discussion under previous version again, IMHO the best way
> > to report what's going on is to use 2 functions for mount ant pre-commit
> > time.
> 
> OK, next version will go that direction.
> 
> Although it may still be a little tricky to split what we need in
> btrfs_validate_super() and btrfs_precheck_super().
> 
> What about this idea:
> - btrfs_precheck_super() only checks the very basis:
>   * magic
>   * incompat flags
>   * csum type
>   Any mismatch will do friendly prompt ("no btrfs detected" or
>   "unsupported flags/csum type" respectively)
> 
> - btrfs_validate_super() do the comprehensive check:
>   * Everything we did in this patchset
>   * including magic, incompat flags and csum type
>   Any mismatch will be considered as corruption.
> 
> For mount time, we call btrfs_precheck_super() then btrfs_validate_super().
> 
> For commit time, only btrfs_validate_super().
> 
> How about this solution?

I'd do that the other way around:

* mount checks: btrfs_validate_super - the superblock on disk must be
  valid before we mount it

  btrfs_validate_super()

* pre-commit checks: the superblock must be valid and we can also do
  some additional checks to detect in-memory corruption

  btrfs_super_precommit_check()
	  btrfs_validate_super()
	  if (incompat) {
		  ...
	  }

Re: [PATCH v2 2/4] btrfs: Add incompat flags check for btrfs_check_super_valid()

[Qu Wenruo]

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



On 2018年04月24日 19:30, David Sterba wrote:
> On Tue, Apr 24, 2018 at 07:28:27PM +0800, Qu Wenruo wrote:
>>> I've read the discussion under previous version again, IMHO the best way
>>> to report what's going on is to use 2 functions for mount ant pre-commit
>>> time.
>>
>> OK, next version will go that direction.
>>
>> Although it may still be a little tricky to split what we need in
>> btrfs_validate_super() and btrfs_precheck_super().
>>
>> What about this idea:
>> - btrfs_precheck_super() only checks the very basis:
>>   * magic
>>   * incompat flags
>>   * csum type
>>   Any mismatch will do friendly prompt ("no btrfs detected" or
>>   "unsupported flags/csum type" respectively)
>>
>> - btrfs_validate_super() do the comprehensive check:
>>   * Everything we did in this patchset
>>   * including magic, incompat flags and csum type
>>   Any mismatch will be considered as corruption.
>>
>> For mount time, we call btrfs_precheck_super() then btrfs_validate_super().
>>
>> For commit time, only btrfs_validate_super().
>>
>> How about this solution?
> 
> I'd do that the other way around:
> 
> * mount checks: btrfs_validate_super - the superblock on disk must be
>   valid before we mount it
> 
>   btrfs_validate_super()
> 
> * pre-commit checks: the superblock must be valid and we can also do
>   some additional checks to detect in-memory corruption
> 
>   btrfs_super_precommit_check()
> 	  btrfs_validate_super()
> 	  if (incompat) {
> 		  ...
> 	  }

Understood.

I'll try this one in next one.

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
> 


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

[PATCH v2 3/4] btrfs: Add csum type check for btrfs_check_super_valid()

[Qu Wenruo]

Just like incompat flags check, although we have already done super csum
type check before calling btrfs_check_super_valid(), we can still add
such check for later write time check.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 3968f7ff8de2..9282a6ac91db 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3985,6 +3985,15 @@ static int btrfs_validate_super(struct btrfs_fs_info *fs_info)
 		btrfs_err(fs_info, "no valid FS found");
 		ret = -EINVAL;
 	}
+	/*
+	 * For write time check, as for mount time we have checked csum before
+	 * calling btrfs_check_super_valid(), so it must be a corruption
+	 */
+	if (btrfs_super_csum_type(sb) >= ARRAY_SIZE(btrfs_csum_sizes)) {
+		btrfs_err(fs_info, "corrupted csum type %u",
+			  btrfs_super_csum_type(sb));
+		ret = -EINVAL;
+	}
 	if (btrfs_super_flags(sb) & ~BTRFS_SUPER_FLAG_SUPP) {
 		btrfs_err(fs_info, "unrecognized or unsupported super flag: %llu",
 				btrfs_super_flags(sb) & ~BTRFS_SUPER_FLAG_SUPP);
-- 
2.17.0

[PATCH v2 4/4] btrfs: Do super block verification before writing it to disk

[Qu Wenruo]

There are already 2 reports about strangely corrupted super blocks,
where csum still matches but extra garbage gets slipped into super block.

The corruption would looks like:
------
superblock: bytenr=65536, device=/dev/sdc1
---------------------------------------------------------
csum_type               41700 (INVALID)
csum                    0x3b252d3a [match]
bytenr                  65536
flags                   0x1
                        ( WRITTEN )
magic                   _BHRfS_M [match]
...
incompat_flags          0x5b22400000000169
                        ( MIXED_BACKREF |
                          COMPRESS_LZO |
                          BIG_METADATA |
                          EXTENDED_IREF |
                          SKINNY_METADATA |
                          unknown flag: 0x5b22400000000000 )
...
------
Or
------
superblock: bytenr=65536, device=/dev/mapper/x
---------------------------------------------------------
csum_type              35355 (INVALID)
csum_size              32
csum                   0xf0dbeddd [match]
bytenr                 65536
flags                  0x1
                       ( WRITTEN )
magic                  _BHRfS_M [match]
...
incompat_flags         0x176d200000000169
                       ( MIXED_BACKREF |
                         COMPRESS_LZO |
                         BIG_METADATA |
                         EXTENDED_IREF |
                         SKINNY_METADATA |
                         unknown flag: 0x176d200000000000 )
------

Obviously, csum_type and incompat_flags get some garbage, but its csum
still matches, which means kernel calculates the csum based on corrupted
super block memory.
And after manually fixing these values, the filesystem is completely
healthy without any problem exposed by btrfs check.

Although the cause is still unknown, at least detect it and prevent further
corruption.

Reported-by: Ken Swenson <flat@imo.uto.moe>
Reported-by: Ben Parsons <9parsonsb@gmail.com>
Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/disk-io.c | 36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 9282a6ac91db..0f5771244641 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -55,7 +55,9 @@
 static const struct extent_io_ops btree_extent_io_ops;
 static void end_workqueue_fn(struct btrfs_work *work);
 static void free_fs_root(struct btrfs_root *root);
-static int btrfs_validate_super(struct btrfs_fs_info *fs_info);
+static int btrfs_validate_super(struct btrfs_fs_info *fs_info,
+				   struct btrfs_super_block *sb,
+				   int super_mirror);
 static void btrfs_destroy_ordered_extents(struct btrfs_root *root);
 static int btrfs_destroy_delayed_refs(struct btrfs_transaction *trans,
 				      struct btrfs_fs_info *fs_info);
@@ -2668,7 +2670,7 @@ int open_ctree(struct super_block *sb,
 
 	memcpy(fs_info->fsid, fs_info->super_copy->fsid, BTRFS_FSID_SIZE);
 
-	ret = btrfs_validate_super(fs_info);
+	ret = btrfs_validate_super(fs_info, fs_info->super_copy, 0);
 	if (ret) {
 		btrfs_err(fs_info, "superblock contains fatal errors");
 		err = -EINVAL;
@@ -3563,6 +3565,16 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
 	sb = fs_info->super_for_commit;
 	dev_item = &sb->dev_item;
 
+	/*
+	 * super_bytenr will be updated in write_dev_supers(), even if it is
+	 * corrupted in current copy, it won't reach disk. So skip bytenr check.
+	 */
+	if (btrfs_validate_super(fs_info, sb, -1)) {
+		btrfs_err(fs_info,
+		"superblock corruption detected before transaction commit");
+		return -EUCLEAN;
+	}
+
 	mutex_lock(&fs_info->fs_devices->device_list_mutex);
 	head = &fs_info->fs_devices->devices;
 	max_errors = btrfs_super_num_devices(fs_info->super_copy) - 1;
@@ -3974,9 +3986,18 @@ int btrfs_read_buffer(struct extent_buffer *buf, u64 parent_transid, int level,
 					      level, first_key);
 }
 
-static int btrfs_validate_super(struct btrfs_fs_info *fs_info)
+/*
+ * Check the validation of btrfs super block.
+ *
+ * @sb:			super block to check
+ * @super_mirror:	the super block number to check its bytenr.
+ * 			0 means the primary (1st) sb, 1 and 2 means 2nd and
+ * 			3rd backup sb, while -1 means to skip bytenr check.
+ */
+static int btrfs_validate_super(struct btrfs_fs_info *fs_info,
+				struct btrfs_super_block *sb,
+				int super_mirror)
 {
-	struct btrfs_super_block *sb = fs_info->super_copy;
 	u64 nodesize = btrfs_super_nodesize(sb);
 	u64 sectorsize = btrfs_super_sectorsize(sb);
 	int ret = 0;
@@ -4088,9 +4109,10 @@ static int btrfs_validate_super(struct btrfs_fs_info *fs_info)
 		ret = -EINVAL;
 	}
 
-	if (btrfs_super_bytenr(sb) != BTRFS_SUPER_INFO_OFFSET) {
-		btrfs_err(fs_info, "super offset mismatch %llu != %u",
-			  btrfs_super_bytenr(sb), BTRFS_SUPER_INFO_OFFSET);
+	if (super_mirror >= 0 && btrfs_super_bytenr(sb) !=
+	    btrfs_sb_offset(super_mirror)) {
+		btrfs_err(fs_info, "super offset mismatch %llu != %llu",
+			btrfs_super_bytenr(sb), btrfs_sb_offset(super_mirror));
 		ret = -EINVAL;
 	}
 
-- 
2.17.0

Re: [PATCH v2 4/4] btrfs: Do super block verification before writing it to disk

[David Sterba]

On Tue, Apr 24, 2018 at 12:48:09PM +0800, Qu Wenruo wrote:
> -static int btrfs_validate_super(struct btrfs_fs_info *fs_info)
> +/*
> + * Check the validation of btrfs super block.
> + *
> + * @sb:			super block to check
> + * @super_mirror:	the super block number to check its bytenr.
> + * 			0 means the primary (1st) sb, 1 and 2 means 2nd and
> + * 			3rd backup sb, while -1 means to skip bytenr check.

Please format the values like:

/*
 * Check the validity of btrfs super block
 *
 * @sb:                        super block to check
 * @super_mirror:      the super block number to check its bytenr:
 *                     0 means the primary (1st) sb,
 *                     1 and 2 means 2nd and 3rd backup copy
 *                     -1 means to skip bytenr check
 */

> + */
> +static int btrfs_validate_super(struct btrfs_fs_info *fs_info,
> +				struct btrfs_super_block *sb,
> +				int super_mirror)
>  {
> -	struct btrfs_super_block *sb = fs_info->super_copy;
>  	u64 nodesize = btrfs_super_nodesize(sb);
>  	u64 sectorsize = btrfs_super_sectorsize(sb);
>  	int ret = 0;
> @@ -4088,9 +4109,10 @@ static int btrfs_validate_super(struct btrfs_fs_info *fs_info)
>  		ret = -EINVAL;
>  	}
>  
> -	if (btrfs_super_bytenr(sb) != BTRFS_SUPER_INFO_OFFSET) {
> -		btrfs_err(fs_info, "super offset mismatch %llu != %u",
> -			  btrfs_super_bytenr(sb), BTRFS_SUPER_INFO_OFFSET);
> +	if (super_mirror >= 0 && btrfs_super_bytenr(sb) !=
> +	    btrfs_sb_offset(super_mirror)) {

The preferred condition split is after the operators, not in the middle
of the expression. Like this:

	if (super_mirror >= 0 &&
	    btrfs_super_bytenr(sb) != btrfs_sb_offset(super_mirror)) {

Re: [PATCH v2 0/4] btrfs: Add write time super block validation

[David Sterba]

On Tue, Apr 24, 2018 at 12:48:05PM +0800, Qu Wenruo wrote:
> This patchset can be fetched from github:
> https://github.com/adam900710/linux/tree/write_time_sb_check
> 
> We have 2 reports about corrupted btrfs super block, which has some garbage
> in its super block, but otherwise it's completely fine and its csum even
> matches.
> 
> This means we develop memory corruption during btrfs mount time.
> It's not clear whether it's caused by btrfs or some other kernel module,
> but at least let's do write time verification to catch such corruption
> early.
> 
> Changelog:
> v2:
>   Rename btrfs_check_super_valid() to btrfs_validate_super() suggested
>   by Nikolay and David.
> 
> Qu Wenruo (4):
>   btrfs: Rename btrfs_check_super_valid() to btrfs_validate_super()
>   btrfs: Add incompat flags check for btrfs_check_super_valid()
>   btrfs: Add csum type check for btrfs_check_super_valid()
>   btrfs: Do super block verification before writing it to disk

IIRC there were some comments about the overal structure of the checks,
but I can't find V3 of the patchset. Can you please resend it or point
me to it in case I missed it? Thanks.

Re: [PATCH v2 0/4] btrfs: Add write time super block validation

[Qu Wenruo]

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



On 2018年05月10日 22:43, David Sterba wrote:
> On Tue, Apr 24, 2018 at 12:48:05PM +0800, Qu Wenruo wrote:
>> This patchset can be fetched from github:
>> https://github.com/adam900710/linux/tree/write_time_sb_check
>>
>> We have 2 reports about corrupted btrfs super block, which has some garbage
>> in its super block, but otherwise it's completely fine and its csum even
>> matches.
>>
>> This means we develop memory corruption during btrfs mount time.
>> It's not clear whether it's caused by btrfs or some other kernel module,
>> but at least let's do write time verification to catch such corruption
>> early.
>>
>> Changelog:
>> v2:
>>   Rename btrfs_check_super_valid() to btrfs_validate_super() suggested
>>   by Nikolay and David.
>>
>> Qu Wenruo (4):
>>   btrfs: Rename btrfs_check_super_valid() to btrfs_validate_super()
>>   btrfs: Add incompat flags check for btrfs_check_super_valid()
>>   btrfs: Add csum type check for btrfs_check_super_valid()
>>   btrfs: Do super block verification before writing it to disk
> 
> IIRC there were some comments about the overal structure of the checks,
> but I can't find V3 of the patchset. Can you please resend it or point
> me to it in case I missed it? Thanks.

Sorry, forgot to update the patchset, would send them out soon.

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
> 


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