| Message ID | 20180424044809.29838-3-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. -- 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
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 >
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) { ... } -- 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
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 >
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.
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(+)