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

Theodore Ts'o
2022-10-20T03:03:15Z

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").

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

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

Cheers,

- Ted