btrfs: Add write time super block validation

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.

Quoted 3 lines · 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.

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 3 lines

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?

Quoted 22 lines · Qu Wenruo

Quoted 3 lines

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.

Quoted 5 lines

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) {
		  ...
	  }

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

+ */

fs/btrfs/disk-io.c +3 -1

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

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

Quoted 17 lines · 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.

Quoted 5 lines

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html