Re: [PATCH v2] btrfs: check the superblock to ensure the fs is not modified at thaw time

[Qu Wenruo <wqu@suse.com>]

On 2022/8/24 18:13, Anand Jain wrote:
> On 11/08/2022 14:47, Qu Wenruo wrote:
>> [BACKGROUND]
>> There is an incident report that, one user hibernated the system, with
>> one btrfs on removable device still mounted.
>>
>> Then by some incident, the btrfs got mounted and modified by another
>> system/OS, then back to the hibernated system.
>>
>> After resuming from the hibernation, new write happened into the 
>> victim btrfs.
>>
>> Now the fs is completely broken, since the underlying btrfs is no longer
>> the same one before the hibernation, and the user lost their data due to
>> various transid mismatch.
>>
>> [REPRODUCER]
>> We can emulate the situation using the following small script:
>>
>>   truncate -s 1G $dev
>>   mkfs.btrfs -f $dev
>>   mount $dev $mnt
>>   fsstress -w -d $mnt -n 500
>>   sync
>>   xfs_freeze -f $mnt
>>   cp $dev $dev.backup
>>
>>   # There is no way to mount the same cloned fs on the same system,
>>   # as the conflicting fsid will be rejected by btrfs.
>>   # Thus here we have to wipe the fs using a different btrfs.
>>   mkfs.btrfs -f $dev.backup
>>
>>   dd if=$dev.backup of=$dev bs=1M
>>   xfs_freeze -u $mnt
>>   fsstress -w -d $mnt -n 20
>>   umount $mnt
>>   btrfs check $dev
>>
>> The final fsck will fail due to some tree blocks has incorrect fsid.
>>
>> This is enough to emulate the problem hit by the unfortunate user.
>>
>> [ENHANCEMENT]
>> Although such case should not be that common, it can still happen from
>> time to time.
>>
>>  From the view of btrfs, we can detect any unexpected super block change,
>> and if there is any unexpected change, we just mark the fs RO, and thaw
>> the fs.
>>
>> By this we can limit the damage to minimal, and I hope no one would lose
>> their data by this anymore.
>>
>> Suggested-by: Goffredo Baroncelli <kreijack@libero.it>
>> Link: 
>> https://lore.kernel.org/linux-btrfs/83bf3b4b-7f4c-387a-b286-9251e3991e34@bluemole.com/ 
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>> Changelog:
>> v2:
>> - Remove one unrelated debug pr_info()
>> - Slightly re-word some comments
>> - Add suggested-by tag
>> ---
>>   fs/btrfs/disk-io.c |  9 +++++--
>>   fs/btrfs/disk-io.h |  2 +-
>>   fs/btrfs/super.c   | 58 ++++++++++++++++++++++++++++++++++++++++++++++
>>   fs/btrfs/volumes.c |  2 +-
>>   4 files changed, 67 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 6268dafeeb2d..7d99c42bdc51 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -3849,7 +3849,7 @@ static void btrfs_end_super_write(struct bio *bio)
>>   }
>>   struct btrfs_super_block *btrfs_read_dev_one_super(struct 
>> block_device *bdev,
>> -                           int copy_num)
>> +                           int copy_num, bool drop_cache)
>>   {
>>       struct btrfs_super_block *super;
>>       struct page *page;
>> @@ -3867,6 +3867,11 @@ struct btrfs_super_block 
>> *btrfs_read_dev_one_super(struct block_device *bdev,
>>       if (bytenr + BTRFS_SUPER_INFO_SIZE >= bdev_nr_bytes(bdev))
>>           return ERR_PTR(-EINVAL);
>> +    if (drop_cache)
> 
> 
>> +        truncate_inode_pages_range(bdev->bd_inode->i_mapping,
>> +                round_down(bytenr, PAGE_SIZE),
>> +                round_up(bytenr + BTRFS_SUPER_INFO_SIZE,
>> +                     PAGE_SIZE) - 1);
> 
> The 3rd argument is the offset to which to truncate (inclusive), and it
> looks correct.
> 
> 
>>       page = read_cache_page_gfp(mapping, bytenr >> PAGE_SHIFT, 
>> GFP_NOFS);
>>       if (IS_ERR(page))
>>           return ERR_CAST(page);
>> @@ -3898,7 +3903,7 @@ struct btrfs_super_block 
>> *btrfs_read_dev_super(struct block_device *bdev)
>>        * later supers, using BTRFS_SUPER_MIRROR_MAX instead
>>        */
>>       for (i = 0; i < 1; i++) {
>> -        super = btrfs_read_dev_one_super(bdev, i);
>> +        super = btrfs_read_dev_one_super(bdev, i, false);
>>           if (IS_ERR(super))
>>               continue;
>> diff --git a/fs/btrfs/disk-io.h b/fs/btrfs/disk-io.h
>> index 47ad8e0a2d33..d0946f502f62 100644
>> --- a/fs/btrfs/disk-io.h
>> +++ b/fs/btrfs/disk-io.h
>> @@ -49,7 +49,7 @@ void __cold close_ctree(struct btrfs_fs_info *fs_info);
>>   int write_all_supers(struct btrfs_fs_info *fs_info, int max_mirrors);
>>   struct btrfs_super_block *btrfs_read_dev_super(struct block_device 
>> *bdev);
>>   struct btrfs_super_block *btrfs_read_dev_one_super(struct 
>> block_device *bdev,
>> -                           int copy_num);
>> +                           int copy_num, bool drop_cache);
>>   int btrfs_commit_super(struct btrfs_fs_info *fs_info);
>>   struct btrfs_root *btrfs_read_tree_root(struct btrfs_root *tree_root,
>>                       struct btrfs_key *key);
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 4c7089b1681b..913b951981a9 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -2548,11 +2548,69 @@ static int btrfs_freeze(struct super_block *sb)
>>       return btrfs_commit_transaction(trans);
>>   }
>> +static int check_dev_super(struct btrfs_device *dev)
>> +{
>> +    struct btrfs_fs_info *fs_info = dev->fs_info;
>> +    struct btrfs_super_block *sb;
>> +    int ret = 0;
>> +
>> +    /* This should be called with fs still frozen. */
>> +    ASSERT(test_bit(BTRFS_FS_FROZEN, &fs_info->flags));
>> +
>> +    /* Missing dev,  no need to check. */
>> +    if (!dev->bdev)
>> +        return 0;
>> +
>> +    /* Only need to check the primary super block. */
>> +    sb = btrfs_read_dev_one_super(dev->bdev, 0, true);
>> +    if (IS_ERR(sb))
>> +        return PTR_ERR(sb);
>> +
> 
>> +    if (memcmp(sb->fsid, dev->fs_devices->fsid, BTRFS_FSID_SIZE)) {
>> +        btrfs_err(fs_info, "fsid doesn't match, has %pU expect %pU",
>> +              sb->fsid, dev->fs_devices->fsid);
>> +        ret = -EUCLEAN;
>> +        goto out;
>> +    }
> 
>   Just a fallthrough is fine.

If the fsid is changed, the generation check are almost ensured to fail.

Thus I don't think we should even try continue checking.

> 
>> +    if (btrfs_super_generation(sb) != fs_info->last_trans_committed) {
>> +        btrfs_err(fs_info, "transid mismatch, has %llu expect %llu",
>> +            btrfs_super_generation(sb),
>> +            fs_info->last_trans_committed);
>> +        ret = -EUCLEAN;
>> +        goto out;
>> +    }
> 
>   Here also. >
>> +out:
> 
>   And the out label can be removed.

As David mentioned, we may want to do a full super block check,
and as I mentioned above, I don't think any failed check should continue 
the verification, thus I'm afraid I would keep the tag.

> 
>> +    btrfs_release_disk_super(sb);
>> +    return ret;
>> +}
>> +
>>   static int btrfs_unfreeze(struct super_block *sb)
>>   {
>>       struct btrfs_fs_info *fs_info = btrfs_sb(sb);
>> +    struct btrfs_device *device;
>> +    int ret = 0;
> 
> 
>> +    /*
>> +     * Make sure the fs is not changed by accident (like hibernation 
>> then
>> +     * modified by other OS).
>> +     * If we found anything wrong, we mark the fs error immediately.
>> +     */
>> +    list_for_each_entry(device, &fs_info->fs_devices->devices, 
>> dev_list) {
>> +        ret = check_dev_super(device);
>> +        if (ret < 0) {
>> +            btrfs_handle_fs_error(fs_info, ret,
>> +                "filesystem got modified unexpectedly");
> 
> 
>   btrfs_read_dev_one_super() may return -EINVAL and the error log will
>   miss lead.

In that case, it still means the fs is incorrect.

Unless we have some unexposed bugs, shouldn't every super block, which 
we committed to disk, is valid?

> 
>> +            break;
>> +        }
>> +    }
> 
>   I checked if device_list_mutex is required, but as we are in a frozen
>   state, you are correct no device_list_mutex is required here.

I can definitely add a comment for this.

Thanks,
Qu

> 
> 
> Thanks, Anand
> 
>>       clear_bit(BTRFS_FS_FROZEN, &fs_info->flags);
>> +
>> +    /*
>> +     * We still return 0, to allow VFS layer to unfreeze the fs even 
>> above
>> +     * checks failed. Since the fs is either fine or RO, we're safe to
>> +     * continue, without causing further damage.
>> +     */
>>       return 0;
>>   }
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 8c64dda69404..a02066ae5812 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -2017,7 +2017,7 @@ void btrfs_scratch_superblocks(struct 
>> btrfs_fs_info *fs_info,
>>           struct page *page;
>>           int ret;
>> -        disk_super = btrfs_read_dev_one_super(bdev, copy_num);
>> +        disk_super = btrfs_read_dev_one_super(bdev, copy_num, false);
>>           if (IS_ERR(disk_super))
>>               continue;
>