[PATCH 1/3] btrfs: Add incompat flags check for btrfs_check_super_valid()

[PATCH 1/3] 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 60caa68c3618..ec123158f051 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(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

[PATCH 2/3] 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 ec123158f051..b189ec086bc2 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3981,6 +3981,15 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
 	u64 sectorsize = btrfs_super_sectorsize(sb);
 	int ret = 0;
 
+	/*
+	 * 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_magic(sb) != BTRFS_MAGIC) {
 		btrfs_err(fs_info, "no valid FS found");
 		ret = -EINVAL;
-- 
2.17.0

[PATCH 3/3] 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 | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b189ec086bc2..fc62f84c3613 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_check_super_valid(struct btrfs_fs_info *fs_info);
+static int btrfs_check_super_valid(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_check_super_valid(fs_info);
+	ret = btrfs_check_super_valid(fs_info, fs_info->super_copy, 0);
 	if (ret) {
 		btrfs_err(fs_info, "superblock contains fatal errors");
 		err = -EINVAL;
@@ -3563,6 +3565,12 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
 	sb = fs_info->super_for_commit;
 	dev_item = &sb->dev_item;
 
+	if (btrfs_check_super_valid(fs_info, sb, -1)) {
+		btrfs_err(fs_info,
+		"superblock corruption detected before transaction commitment");
+		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 +3982,18 @@ 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)
+/*
+ * 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_check_super_valid(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;
@@ -4019,7 +4036,7 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
 	 * Check sectorsize and nodesize first, other check will need it.
 	 * Check all possible sectorsize(4K, 8K, 16K, 32K, 64K) here.
 	 */
-	if (!is_power_of_2(sectorsize) || sectorsize < 4096 ||
+	if (!is_power_of_2(sectorsize) || sectorsize < SZ_4K ||
 	    sectorsize > BTRFS_MAX_METADATA_BLOCKSIZE) {
 		btrfs_err(fs_info, "invalid sectorsize %llu", sectorsize);
 		ret = -EINVAL;
@@ -4088,9 +4105,10 @@ static int btrfs_check_super_valid(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 2/3] btrfs: Add csum type check for btrfs_check_super_valid()

[David Sterba]

On Thu, Apr 19, 2018 at 05:38:15PM +0800, Qu Wenruo wrote:
> 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 ec123158f051..b189ec086bc2 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3981,6 +3981,15 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
>  	u64 sectorsize = btrfs_super_sectorsize(sb);
>  	int ret = 0;
>  
> +	/*
> +	 * 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_magic(sb) != BTRFS_MAGIC) {

The test for magic should be IMO the first one, that's how the
filesystem type is identified before anything else. Granted that the
magic must be correct before it can be even mounted, but the code should
follow that logic too. Otherwise ok.

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

[David Sterba]

Looks good, some minor comments below. I'm wondering how to test that.
We'd have to inject either the corruption or to provide a way to
forcibly fail the test. For the latter a debugfs should do, I'll send
something for comments.

On Thu, Apr 19, 2018 at 05:38:16PM +0800, Qu Wenruo wrote:
> @@ -3563,6 +3565,12 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
>  	sb = fs_info->super_for_commit;
>  	dev_item = &sb->dev_item;
>  
> +	if (btrfs_check_super_valid(fs_info, sb, -1)) {

A comment that this is skipping the bytenr check would be good.

> +		btrfs_err(fs_info,
> +		"superblock corruption detected before transaction commitment");

                                                                   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 +3982,18 @@ 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)
> +/*
> + * 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_check_super_valid(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;
> @@ -4019,7 +4036,7 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
>  	 * Check sectorsize and nodesize first, other check will need it.
>  	 * Check all possible sectorsize(4K, 8K, 16K, 32K, 64K) here.
>  	 */
> -	if (!is_power_of_2(sectorsize) || sectorsize < 4096 ||
> +	if (!is_power_of_2(sectorsize) || sectorsize < SZ_4K ||

No unrelated changes please. There are some remaining raw values, send a
separate patch if you want to convert them.

>  	    sectorsize > BTRFS_MAX_METADATA_BLOCKSIZE) {
>  		btrfs_err(fs_info, "invalid sectorsize %llu", sectorsize);
>  		ret = -EINVAL;
> @@ -4088,9 +4105,10 @@ static int btrfs_check_super_valid(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
> 
> --
> 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

Re: [PATCH 2/3] btrfs: Add csum type check for btrfs_check_super_valid()

[Qu Wenruo]

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



On 2018年04月19日 18:09, David Sterba wrote:
> On Thu, Apr 19, 2018 at 05:38:15PM +0800, Qu Wenruo wrote:
>> 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 ec123158f051..b189ec086bc2 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -3981,6 +3981,15 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
>>  	u64 sectorsize = btrfs_super_sectorsize(sb);
>>  	int ret = 0;
>>  
>> +	/*
>> +	 * 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_magic(sb) != BTRFS_MAGIC) {
> 
> The test for magic should be IMO the first one, that's how the
> filesystem type is identified before anything else. Granted that the
> magic must be correct before it can be even mounted, but the code should
> follow that logic too. Otherwise ok.

Makes sense, will update the patchset.

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 --]

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

[Qu Wenruo]

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



On 2018年04月19日 18:16, David Sterba wrote:
> Looks good, some minor comments below. I'm wondering how to test that.

Currently I'm using the most stupid way to test it, insert code randomly
modifies super blocks.

> We'd have to inject either the corruption or to provide a way to
> forcibly fail the test. For the latter a debugfs should do, I'll send
> something for comments.

What about a sysfs interface to modify super blocks?
Since we already have sectorsize and nodessize, allow it to be writeable
for CONFIG_BTRFS_DEBUG looks pretty good.

Thanks,
Qu

> 
> On Thu, Apr 19, 2018 at 05:38:16PM +0800, Qu Wenruo wrote:
>> @@ -3563,6 +3565,12 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
>>  	sb = fs_info->super_for_commit;
>>  	dev_item = &sb->dev_item;
>>  
>> +	if (btrfs_check_super_valid(fs_info, sb, -1)) {
> 
> A comment that this is skipping the bytenr check would be good.
> 
>> +		btrfs_err(fs_info,
>> +		"superblock corruption detected before transaction commitment");
> 
>                                                                    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 +3982,18 @@ 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)
>> +/*
>> + * 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_check_super_valid(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;
>> @@ -4019,7 +4036,7 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
>>  	 * Check sectorsize and nodesize first, other check will need it.
>>  	 * Check all possible sectorsize(4K, 8K, 16K, 32K, 64K) here.
>>  	 */
>> -	if (!is_power_of_2(sectorsize) || sectorsize < 4096 ||
>> +	if (!is_power_of_2(sectorsize) || sectorsize < SZ_4K ||
> 
> No unrelated changes please. There are some remaining raw values, send a
> separate patch if you want to convert them.
> 
>>  	    sectorsize > BTRFS_MAX_METADATA_BLOCKSIZE) {
>>  		btrfs_err(fs_info, "invalid sectorsize %llu", sectorsize);
>>  		ret = -EINVAL;
>> @@ -4088,9 +4105,10 @@ static int btrfs_check_super_valid(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
>>
>> --
>> 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
> --
> 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 1/3] btrfs: Add incompat flags check for btrfs_check_super_valid()

[Nikolay Borisov]

On 19.04.2018 12:38, 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.
> 
> 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>

Reviewed-by: Nikolay Borisov <nborisov@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 60caa68c3618..ec123158f051 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)

nit: Thinking out loud here - wouldn't it make more sense to name the
function btrfs_validate_super. check_super_valid sounds a bit cumbersome
to me. What do you think ?
>  		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
s/developr/detect ?
> +	 * 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.
> 

Re: [PATCH 1/3] btrfs: Add incompat flags check for btrfs_check_super_valid()

[Qu Wenruo]

On 2018年04月19日 18:59, Nikolay Borisov wrote:
> 
> 
> On 19.04.2018 12:38, 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.
>>
>> 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>
> 
> Reviewed-by: Nikolay Borisov <nborisov@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 60caa68c3618..ec123158f051 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
> 
> nit: Thinking out loud here - wouldn't it make more sense to name the
> function btrfs_validate_super. check_super_valid sounds a bit cumbersome
> to me. What do you think ?

Indeed, I also like to remove the btrfs_ prefix since it's a static
function.
validate_super() looks much better.

Thanks,
Qu

>>  		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
> s/developr/detect ?
>> +	 * 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.
>>
> --
> 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
> 

Re: [PATCH 1/3] btrfs: Add incompat flags check for btrfs_check_super_valid()

[David Sterba]

On Thu, Apr 19, 2018 at 07:10:30PM +0800, Qu Wenruo wrote:
> 
> 
> On 2018年04月19日 18:59, Nikolay Borisov wrote:
> > 
> > 
> > On 19.04.2018 12:38, 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.
> >>
> >> 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>
> > 
> > Reviewed-by: Nikolay Borisov <nborisov@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 60caa68c3618..ec123158f051 100644
> >> --- a/fs/btrfs/disk-io.c
> >> +++ b/fs/btrfs/disk-io.c
> >> @@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
> > 
> > nit: Thinking out loud here - wouldn't it make more sense to name the
> > function btrfs_validate_super. check_super_valid sounds a bit cumbersome
> > to me. What do you think ?
> 
> Indeed, I also like to remove the btrfs_ prefix since it's a static
> function.
> validate_super() looks much better.

It's not necessary to remove the btrfs_ prefix from all static
functions, sometimes the functions appear on stacks or mixed with other
subystem helpers that have generic names. The prefix makes it clear that
it's our function.

Re: [PATCH 1/3] btrfs: Add incompat flags check for btrfs_check_super_valid()

[Nikolay Borisov]

On 19.04.2018 18:31, David Sterba wrote:
> On Thu, Apr 19, 2018 at 07:10:30PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2018年04月19日 18:59, Nikolay Borisov wrote:
>>>
>>>
>>> On 19.04.2018 12:38, 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.
>>>>
>>>> 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>
>>>
>>> Reviewed-by: Nikolay Borisov <nborisov@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 60caa68c3618..ec123158f051 100644
>>>> --- a/fs/btrfs/disk-io.c
>>>> +++ b/fs/btrfs/disk-io.c
>>>> @@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(struct btrfs_fs_info *fs_info)
>>>
>>> nit: Thinking out loud here - wouldn't it make more sense to name the
>>> function btrfs_validate_super. check_super_valid sounds a bit cumbersome
>>> to me. What do you think ?
>>
>> Indeed, I also like to remove the btrfs_ prefix since it's a static
>> function.
>> validate_super() looks much better.
> 
> It's not necessary to remove the btrfs_ prefix from all static
> functions, sometimes the functions appear on stacks or mixed with other
> subystem helpers that have generic names. The prefix makes it clear that
> it's our function.

I agree with David, just make it btrfs_validate_super
> 

Re: [PATCH 1/3] btrfs: Add incompat flags check for btrfs_check_super_valid()

[David Sterba]

On Thu, Apr 19, 2018 at 07:24:48PM +0300, Nikolay Borisov wrote:
> I agree with David, just make it btrfs_validate_super

Ack.

Re: [PATCH 1/3] btrfs: Add incompat flags check for btrfs_check_super_valid()

[Anand Jain]

On 04/19/2018 05:38 PM, 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.
> 
> 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 60caa68c3618..ec123158f051 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(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.
> 

This patch should move the existing check in the open_ctree() line 2707
into the btrfs_check_super_valid(), we don't need duplicate checks.

disk-io.c

2707         features = btrfs_super_incompat_flags(disk_super) &
2708                 ~BTRFS_FEATURE_INCOMPAT_SUPP;
2709         if (features) {
2710                 btrfs_err(fs_info,
2711                     "cannot mount because of unsupported optional features (%llx)",
2712                     features);
2713                 err = -EINVAL;
2714                 goto fail_alloc;
2715         }


Thanks, Anand

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

[Anand Jain]

> @@ -3563,6 +3565,12 @@ int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors)
>   	sb = fs_info->super_for_commit;
>   	dev_item = &sb->dev_item;
>   
> +	if (btrfs_check_super_valid(fs_info, sb, -1)) {
> +		btrfs_err(fs_info,
> +		"superblock corruption detected before transaction commitment");
> +		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;

  With patch 1/3 (incompat feature checks) now this set as a
  whole address my concern that I explained earlier. I am ok with
  this approach.

Thanks, Anand

Re: [PATCH 1/3] btrfs: Add incompat flags check for btrfs_check_super_valid()

[David Sterba]

On Fri, Apr 20, 2018 at 10:32:03PM +0800, Anand Jain wrote:
> 
> 
> On 04/19/2018 05:38 PM, 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.
> > 
> > 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 60caa68c3618..ec123158f051 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(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",

> 2707         features = btrfs_super_incompat_flags(disk_super) &
> 2708                 ~BTRFS_FEATURE_INCOMPAT_SUPP;
> 2709         if (features) {
> 2710                 btrfs_err(fs_info,
> 2711                     "cannot mount because of unsupported optional features (%llx)",
> 2712                     features);
> 2713                 err = -EINVAL;
> 2714                 goto fail_alloc;
> 2715         }

So there's a "user experience" change, now that you pointed out the
other check. If a filesystem with incompat bits set will be mounted,
this will say 'you have corrupted filesystem', which is not IMO what we
want to tell.

Tough the extended btrfs_check_super_valid could catch the corrupted
incompat bits, what we need at mount time is wording from the 2nd
message ("cannot mount unsuppported features").

I think that there should be a separate function that does the
pre-commit checks, calls btrfs_check_super_valid and also validates the
incompat bit.

Re: [PATCH 1/3] btrfs: Add incompat flags check for btrfs_check_super_valid()

[Anand Jain]

On 04/20/2018 11:15 PM, David Sterba wrote:
> On Fri, Apr 20, 2018 at 10:32:03PM +0800, Anand Jain wrote:
>>
>>
>> On 04/19/2018 05:38 PM, 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.
>>>
>>> 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 60caa68c3618..ec123158f051 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(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",
> 
>> 2707         features = btrfs_super_incompat_flags(disk_super) &
>> 2708                 ~BTRFS_FEATURE_INCOMPAT_SUPP;
>> 2709         if (features) {
>> 2710                 btrfs_err(fs_info,
>> 2711                     "cannot mount because of unsupported optional features (%llx)",
>> 2712                     features);
>> 2713                 err = -EINVAL;
>> 2714                 goto fail_alloc;
>> 2715         }
> 
> So there's a "user experience" change, now that you pointed out the
> other check. 

  Its a regression.

> If a filesystem with incompat bits set will be mounted,
> this will say 'you have corrupted filesystem', which is not IMO what we
> want to tell.
> 
> Tough the extended btrfs_check_super_valid could catch the corrupted
> incompat bits, what we need at mount time is wording from the 2nd
> message ("cannot mount unsuppported features").
> 
> I think that there should be a separate function that does the
> pre-commit checks, calls btrfs_check_super_valid and also validates the
> incompat bit.

  We can still print it as 'unsupported optional features', which would
  imply corruption in the non-mount context.

Thanks, Anand

> --
> 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
> 

Re: [PATCH 1/3] btrfs: Add incompat flags check for btrfs_check_super_valid()

[Qu Wenruo]

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



On 2018年04月21日 10:38, Anand Jain wrote:
> 
> 
> On 04/20/2018 11:15 PM, David Sterba wrote:
>> On Fri, Apr 20, 2018 at 10:32:03PM +0800, Anand Jain wrote:
>>>
>>>
>>> On 04/19/2018 05:38 PM, 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.
>>>>
>>>> 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 60caa68c3618..ec123158f051 100644
>>>> --- a/fs/btrfs/disk-io.c
>>>> +++ b/fs/btrfs/disk-io.c
>>>> @@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(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",
>>
>>> 2707         features = btrfs_super_incompat_flags(disk_super) &
>>> 2708                 ~BTRFS_FEATURE_INCOMPAT_SUPP;
>>> 2709         if (features) {
>>> 2710                 btrfs_err(fs_info,
>>> 2711                     "cannot mount because of unsupported
>>> optional features (%llx)",
>>> 2712                     features);
>>> 2713                 err = -EINVAL;
>>> 2714                 goto fail_alloc;
>>> 2715         }
>>
>> So there's a "user experience" change, now that you pointed out the
>> other check. 
> 
>  Its a regression.
> 
>> If a filesystem with incompat bits set will be mounted,
>> this will say 'you have corrupted filesystem', which is not IMO what we
>> want to tell.
>>
>> Tough the extended btrfs_check_super_valid could catch the corrupted
>> incompat bits, what we need at mount time is wording from the 2nd
>> message ("cannot mount unsuppported features").
>>
>> I think that there should be a separate function that does the
>> pre-commit checks, calls btrfs_check_super_valid and also validates the
>> incompat bit.
> 
>  We can still print it as 'unsupported optional features', which would
>  imply corruption in the non-mount context.

In that case such wording is not obvious enough to info it's super block
corruption.

So I still prefer current way of checking incompact flags and csum types
manually at mount time before btrfs_validate_super().

David's idea of a separate function to do mount time check and gives
better prompt is pretty good.
Especially for incompat features, as incompat features will increase in
the future and for older kernel info such feature as corruption is not
acceptable.

But currently, there are only 2 members we need to check at mount time
(csum_type and incompat features), and only one caller, current code
looks good enough for its purpose.

Thanks,
Qu

> 
> Thanks, Anand
> 
>> -- 
>> 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
>>
> -- 
> 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 1/3] btrfs: Add incompat flags check for btrfs_check_super_valid()

[Anand Jain]

On 04/21/2018 10:43 AM, Qu Wenruo wrote:
> 
> 
> On 2018年04月21日 10:38, Anand Jain wrote:
>>
>>
>> On 04/20/2018 11:15 PM, David Sterba wrote:
>>> On Fri, Apr 20, 2018 at 10:32:03PM +0800, Anand Jain wrote:
>>>>
>>>>
>>>> On 04/19/2018 05:38 PM, 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.
>>>>>
>>>>> 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 60caa68c3618..ec123158f051 100644
>>>>> --- a/fs/btrfs/disk-io.c
>>>>> +++ b/fs/btrfs/disk-io.c
>>>>> @@ -4104,6 +4104,19 @@ static int btrfs_check_super_valid(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",
>>>
>>>> 2707         features = btrfs_super_incompat_flags(disk_super) &
>>>> 2708                 ~BTRFS_FEATURE_INCOMPAT_SUPP;
>>>> 2709         if (features) {
>>>> 2710                 btrfs_err(fs_info,
>>>> 2711                     "cannot mount because of unsupported
>>>> optional features (%llx)",
>>>> 2712                     features);
>>>> 2713                 err = -EINVAL;
>>>> 2714                 goto fail_alloc;
>>>> 2715         }
>>>
>>> So there's a "user experience" change, now that you pointed out the
>>> other check.
>>
>>   Its a regression.
>>
>>> If a filesystem with incompat bits set will be mounted,
>>> this will say 'you have corrupted filesystem', which is not IMO what we
>>> want to tell.
>>>
>>> Tough the extended btrfs_check_super_valid could catch the corrupted
>>> incompat bits, what we need at mount time is wording from the 2nd
>>> message ("cannot mount unsuppported features").
>>>
>>> I think that there should be a separate function that does the
>>> pre-commit checks, calls btrfs_check_super_valid and also validates the
>>> incompat bit.
>>
>>   We can still print it as 'unsupported optional features', which would
>>   imply corruption in the non-mount context.
> 
> In that case such wording is not obvious enough to info it's super block
> corruption.

  If the device is already mounted. Then its a corruption.

Thanks, Anand

> So I still prefer current way of checking incompact flags and csum types
> manually at mount time before btrfs_validate_super().
> 
> David's idea of a separate function to do mount time check and gives
> better prompt is pretty good.
> Especially for incompat features, as incompat features will increase in
> the future and for older kernel info such feature as corruption is not
> acceptable.
> 
> But currently, there are only 2 members we need to check at mount time
> (csum_type and incompat features), and only one caller, current code
> looks good enough for its purpose.
> 
> Thanks,
> Qu
> 
>>
>> Thanks, Anand
>>
>>> -- 
>>> 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
>>>
>> -- 
>> 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
>