Re: [PATCH] generic: check if one fs can detect damage at/after fs thaw

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2022/10/20 11:02, Theodore Ts'o wrote:
> On Wed, Oct 19, 2022 at 08:36:55AM -0700, Darrick J. Wong wrote:
>> On Wed, Oct 19, 2022 at 01:29:55PM +0800, Qu Wenruo wrote:
>>> [BACKGROUND]
>>> There is bug report from btrfs mailing list that, hiberation can allow
>>> one to modify the frozen filesystem unexpectedly (using another OS).
>>> (https://lore.kernel.org/linux-btrfs/83bf3b4b-7f4c-387a-b286-9251e3991e34@bluemole.com/)
>>>
>>> Later btrfs adds the check to make sure the fs is not changed
>>> unexpectedly, to prevent corruption from happening.
>>>
>>> [TESTCASE]
>>> Here the new test case will create a basic filesystem, fill it with
>>> something by using fsstress, then sync the fs, and finally freeze the fs.
>>>
>>> Then corrupt the whole fs by overwriting the block device with 0xcd
>>> (default seed from xfs_io pwrite command).
>
> It seems to me that the test case is testing something very different
> from the originally stated concern, and what btrfs is testing.
>
> The original concern is "something else modified the file system",
> which btrfs is testing by checking whether the file system generation
> number is different from the last recorded transaction id.
>
> The test is "something has trashed the file system by filling the
> block device by 0xcd; let's see how quickly the file system notices"
> which is quite different from the scenario described in the link and
> the commit description a05d3c915314 ("btrfs: check superblock to
> ensure the fs was not modified at thaw time").

Yes, you're right.

The problem here is I have no good way to just mount the frozen fs and
update it.

One of the btrfs specific problem is, btrfs will reject fs with the same
fsid.
Thus even if we do a binary copy of the original fs, we can not mount it
to modify, at least not for btrfs.

Any good ideas for this? Maybe relying on btrfs-progs to do the write?

>
>> What /does/ btrfs check, specifically?  Reading this makes me wonder if
>> xfs shouldn't re-read its primary super on thaw to check that nobody ran
>> us over with a backhoe, though that wouldn't help us in the hibernation
>> case.  (Or does it?  Is userspace/systemd finally smart enough to freeze
>> filesystems?)
>
>  From looking at the commit described below, it appears to do some
> basic superblock sanity checks, and then it checks to see if the last
> commited transaction has changed from what has been recorded in the
> superblock.
>
> The simple stupid thing I could add in ext4 is to simply make a full
> copy of the ext4 superblock, and if *anything* in that 1k set of bytes
> has changed between the freeze and the thaw, call ext4_error(), and mark
> the file system corrupted.

Yes, for EXT4/XFS it can be a simple memcmp(), but for btrfs we have
multiple devices, and super block for each device has something
different (related to that device).

Thus at least for btrfs, we can not do a full memcmp() level check, but
you get the idea correctly.

>
> We've been talking about changing the default for ext4 to remount the
> file system read-only, and if we did this then the behavior would be
> the same as btrfs.  Or maybe in the specific case of the superblock
> has changed between freeze and thaw, we will always remount the file
> system read-only.
>
>>> For XFS, it will detect the corruption at touch time, return -EUCLEAN.
>>> (Without the cache drop, XFS seems to be very happy using the cache info
>>> to do the work without any error though.)
>>
>> Yep.
>
> I would suggest not putting this test in generic/NNN, but put it in
> shared, and to let each file system opt-in to this test.  There are a
> whole bunch of file systems such such as jfs, reiserfs, vfat, exfat,
> etc., which could run this test, and depending on the specifics of how
> a file system might behave to determine whether the test "passes" or
> "fails" seems wrong.
>
> After all, what you're really doing is protecting against a specific
> form of "stupid user trick", and other Linux file systems happen to do
> something different when you completely trash the file system by
> overwriting the block device with 0xcd, callign what some other file
> system, whether it be f2fs, exfat, overlayfs, nfs, as a "failure"
> doesn't seem right.
>
> Moving it into shared also means you don't have to add extra checks to
> make sure the test gets skipped if there is no block device to trash
> (for example, if you are testing overlayfs, nfs, tmpfs, etc.)

Thanks for the advice, would move it to shared in next update.

Thanks,
Qu

>
> Cheers,
>
> 					- Ted