From 41c8da6a22ad4c8f552be202c1f9eb7ade3ddb95 Mon Sep 17 00:00:00 2001
From: Sean Klein <smklein@google.com>
Date: Sat, 27 Apr 2019 00:23:24 +0000
Subject: [PATCH] [fshost][minfs] Add fshost tests for reformatting, verifying
filesystems
Additionally, some minor related changes:
- Remove unused "FormatMinfs" function.
- Plumb the "Should we check filesystems?" boolean through explicit
arguments instead of environment variables.
- Make minfs superblock validation more strict. This validates that
clients can't format a filesystem with N blocks, and either mount
of check the consistency of a filesystem with M blocks, for M < N.
This change, along with the fshost tests, acts as a regression for
a bug where the consistency checker only inspected a subsection of the
filesystem.
Change-Id: I15470e9529553526011c8fba2d8a6f5987b8e1a6
---
.../devmgr/fshost/block-device-interface.h | 4 +
.../core/devmgr/fshost/block-device-test.cpp | 85 +++++++++++++++++--
.../core/devmgr/fshost/block-device.cpp | 26 ++----
.../system/core/devmgr/fshost/block-device.h | 1 +
.../core/devmgr/fshost/block-watcher-test.cpp | 3 +
.../core/devmgr/fshost/block-watcher.cpp | 4 +-
.../system/core/devmgr/fshost/block-watcher.h | 2 +-
.../core/devmgr/fshost/filesystem-mounter.h | 9 +-
zircon/system/core/devmgr/fshost/fs-manager.h | 2 +-
zircon/system/core/devmgr/fshost/main.cpp | 5 +-
zircon/system/ulib/minfs/minfs.cpp | 2 +-
11 files changed, 106 insertions(+), 37 deletions(-)
diff --git a/zircon/system/core/devmgr/fshost/block-device-interface.h b/zircon/system/core/devmgr/fshost/block-device-interface.h
index f774fd681fc..99c6302b893 100644
--- a/zircon/system/core/devmgr/fshost/block-device-interface.h
+++ b/zircon/system/core/devmgr/fshost/block-device-interface.h
@@ -58,6 +58,10 @@ private:
// Unseals the underlying zxcrypt volume.
virtual zx_status_t UnsealZxcrypt() = 0;
+ // Returns true if the consistency of filesystems should be validated before
+ // mounting.
+ virtual bool ShouldCheckFilesystems() = 0;
+
// Validates the state of the filesystem, and returns ZX_OK if it appears
// consistent (or if the consistency check should be skipped).
virtual zx_status_t CheckFilesystem() = 0;
diff --git a/zircon/system/core/devmgr/fshost/block-device-test.cpp b/zircon/system/core/devmgr/fshost/block-device-test.cpp
index 641ef04590f..292367e4948 100644
--- a/zircon/system/core/devmgr/fshost/block-device-test.cpp
+++ b/zircon/system/core/devmgr/fshost/block-device-test.cpp
@@ -56,7 +56,8 @@ private:
TEST_F(BlockDeviceHarness, TestBadHandleDevice) {
std::unique_ptr<FsManager> manager = TakeManager();
bool netboot = false;
- FilesystemMounter mounter(std::move(manager), netboot);
+ bool check_filesystems = false;
+ FilesystemMounter mounter(std::move(manager), netboot, check_filesystems);
fbl::unique_fd fd;
BlockDevice device(&mounter, std::move(fd));
EXPECT_EQ(device.Netbooting(), netboot);
@@ -71,7 +72,7 @@ TEST_F(BlockDeviceHarness, TestBadHandleDevice) {
// thread without observing the results.
EXPECT_OK(device.UnsealZxcrypt());
- // Returns ZX_OK because filesystem checks are only enabled via environment variables.
+ // Returns ZX_OK because filesystem checks are disabled.
EXPECT_OK(device.CheckFilesystem());
EXPECT_EQ(device.FormatFilesystem(), ZX_ERR_BAD_HANDLE);
@@ -81,7 +82,8 @@ TEST_F(BlockDeviceHarness, TestBadHandleDevice) {
TEST_F(BlockDeviceHarness, TestEmptyDevice) {
std::unique_ptr<FsManager> manager = TakeManager();
bool netboot = false;
- FilesystemMounter mounter(std::move(manager), netboot);
+ bool check_filesystems = false;
+ FilesystemMounter mounter(std::move(manager), netboot, check_filesystems);
// Initialize Ramdisk.
constexpr uint64_t kBlockSize = 512;
@@ -118,7 +120,8 @@ TEST_F(BlockDeviceHarness, TestEmptyDevice) {
TEST_F(BlockDeviceHarness, TestMinfsBadGUID) {
std::unique_ptr<FsManager> manager = TakeManager();
bool netboot = false;
- FilesystemMounter mounter(std::move(manager), netboot);
+ bool check_filesystems = false;
+ FilesystemMounter mounter(std::move(manager), netboot, check_filesystems);
// Initialize Ramdisk with an empty GUID.
constexpr uint64_t kBlockSize = 512;
@@ -147,7 +150,8 @@ TEST_F(BlockDeviceHarness, TestMinfsGoodGUID) {
std::unique_ptr<FsManager> manager = TakeManager();
bool netboot = false;
- FilesystemMounter mounter(std::move(manager), netboot);
+ bool check_filesystems = false;
+ FilesystemMounter mounter(std::move(manager), netboot, check_filesystems);
// Initialize Ramdisk with a data GUID.
constexpr uint64_t kBlockSize = 512;
@@ -171,8 +175,75 @@ TEST_F(BlockDeviceHarness, TestMinfsGoodGUID) {
ASSERT_OK(ramdisk_destroy(ramdisk));
}
-// TODO(smklein): Plumb through the "check filesystem" decision using
-// something other than environment variables, and add a test for it.
+TEST_F(BlockDeviceHarness, TestMinfsReformat) {
+ std::unique_ptr<FsManager> manager = TakeManager();
+
+ bool netboot = false;
+ bool check_filesystems = true;
+ FilesystemMounter mounter(std::move(manager), netboot, check_filesystems);
+
+ // Initialize Ramdisk with a data GUID.
+ constexpr uint64_t kBlockSize = 512;
+ constexpr uint64_t kBlockCount = 1 << 20;
+ ramdisk_client_t* ramdisk;
+ const uint8_t data_guid[GPT_GUID_LEN] = GUID_DATA_VALUE;
+ ASSERT_OK(ramdisk_create_with_guid(kBlockSize, kBlockCount, data_guid, sizeof(data_guid),
+ &ramdisk));
+ ASSERT_OK(wait_for_device(ramdisk_get_path(ramdisk), zx::sec(5).get()));
+ fbl::unique_fd fd(open(ramdisk_get_path(ramdisk), O_RDWR));
+ ASSERT_TRUE(fd);
+
+ BlockDevice device(&mounter, std::move(fd));
+ device.SetFormat(DISK_FORMAT_MINFS);
+ EXPECT_EQ(device.GetFormat(), DISK_FORMAT_MINFS);
+
+ // Before formatting the device, this isn't a valid minfs partition.
+ EXPECT_NOT_OK(device.CheckFilesystem());
+ EXPECT_NOT_OK(device.MountFilesystem());
+
+ // After formatting the device, it is a valid partition. We can check the device,
+ // and also mount it.
+ EXPECT_OK(device.FormatFilesystem());
+ EXPECT_OK(device.CheckFilesystem());
+ EXPECT_OK(device.MountFilesystem());
+
+ ASSERT_OK(ramdisk_destroy(ramdisk));
+}
+
+TEST_F(BlockDeviceHarness, TestBlobfs) {
+ std::unique_ptr<FsManager> manager = TakeManager();
+
+ bool netboot = false;
+ bool check_filesystems = true;
+ FilesystemMounter mounter(std::move(manager), netboot, check_filesystems);
+
+ // Initialize Ramdisk with a data GUID.
+ constexpr uint64_t kBlockSize = 512;
+ constexpr uint64_t kBlockCount = 1 << 20;
+ ramdisk_client_t* ramdisk;
+ const uint8_t data_guid[GPT_GUID_LEN] = GUID_BLOB_VALUE;
+ ASSERT_OK(ramdisk_create_with_guid(kBlockSize, kBlockCount, data_guid, sizeof(data_guid),
+ &ramdisk));
+ ASSERT_OK(wait_for_device(ramdisk_get_path(ramdisk), zx::sec(5).get()));
+ fbl::unique_fd fd(open(ramdisk_get_path(ramdisk), O_RDWR));
+ ASSERT_TRUE(fd);
+
+ BlockDevice device(&mounter, std::move(fd));
+ device.SetFormat(DISK_FORMAT_BLOBFS);
+ EXPECT_EQ(device.GetFormat(), DISK_FORMAT_BLOBFS);
+
+ // Before formatting the device, this isn't a valid blobfs partition.
+ // However, as implemented, we always validate the consistency of the filesystem.
+ EXPECT_OK(device.CheckFilesystem());
+ EXPECT_NOT_OK(device.MountFilesystem());
+
+ // Additionally, blobfs does not yet support reformatting within fshost.
+ EXPECT_NOT_OK(device.FormatFilesystem());
+ EXPECT_OK(device.CheckFilesystem());
+ EXPECT_NOT_OK(device.MountFilesystem());
+
+ ASSERT_OK(ramdisk_destroy(ramdisk));
+}
// TODO: Add tests for Zxcrypt binding.
diff --git a/zircon/system/core/devmgr/fshost/block-device.cpp b/zircon/system/core/devmgr/fshost/block-device.cpp
index e4388d73824..c5e64c85fc9 100644
--- a/zircon/system/core/devmgr/fshost/block-device.cpp
+++ b/zircon/system/core/devmgr/fshost/block-device.cpp
@@ -108,26 +108,6 @@ int UnsealZxcryptThread(void* arg) {
return 0;
}
-zx_status_t FormatMinfs(const fbl::unique_fd& block_device,
- const fuchsia_hardware_block_BlockInfo& info) {
- fprintf(stderr, "fshost: Formatting minfs.\n");
- uint64_t device_size = info.block_size * info.block_count / minfs::kMinfsBlockSize;
- std::unique_ptr<minfs::Bcache> bc;
- zx_status_t status;
- if ((status = minfs::Bcache::Create(&bc, block_device.duplicate(),
- static_cast<uint32_t>(device_size))) != ZX_OK) {
- fprintf(stderr, "fshost: Could not initialize minfs bcache.\n");
- return status;
- }
- minfs::MountOptions options = {};
- if ((status = Mkfs(options, std::move(bc))) != ZX_OK) {
- fprintf(stderr, "fshost: Could not format minfs filesystem.\n");
- return status;
- }
- printf("fshost: Minfs filesystem re-formatted. Expect data loss.\n");
- return ZX_OK;
-}
-
} // namespace
BlockDevice::BlockDevice(FilesystemMounter* mounter, fbl::unique_fd fd)
@@ -209,8 +189,12 @@ zx_status_t BlockDevice::UnsealZxcrypt() {
return ZX_OK;
}
+bool BlockDevice::ShouldCheckFilesystems() {
+ return mounter_->ShouldCheckFilesystems();
+}
+
zx_status_t BlockDevice::CheckFilesystem() {
- if (!getenv_bool("zircon.system.filesystem-check", false)) {
+ if (!ShouldCheckFilesystems()) {
return ZX_OK;
}
diff --git a/zircon/system/core/devmgr/fshost/block-device.h b/zircon/system/core/devmgr/fshost/block-device.h
index c12e9df57f3..980973641b5 100644
--- a/zircon/system/core/devmgr/fshost/block-device.h
+++ b/zircon/system/core/devmgr/fshost/block-device.h
@@ -30,6 +30,7 @@ public:
zx_status_t GetTypeGUID(fuchsia_hardware_block_partition_GUID* out_guid) final;
zx_status_t AttachDriver(const fbl::StringPiece& driver) final;
zx_status_t UnsealZxcrypt() final;
+ bool ShouldCheckFilesystems() final;
zx_status_t CheckFilesystem() final;
zx_status_t FormatFilesystem() final;
zx_status_t MountFilesystem() final;
diff --git a/zircon/system/core/devmgr/fshost/block-watcher-test.cpp b/zircon/system/core/devmgr/fshost/block-watcher-test.cpp
index b2a6c475ff8..b971e9a571d 100644
--- a/zircon/system/core/devmgr/fshost/block-watcher-test.cpp
+++ b/zircon/system/core/devmgr/fshost/block-watcher-test.cpp
@@ -34,6 +34,9 @@ public:
zx_status_t UnsealZxcrypt() override {
ZX_PANIC("Test should not invoke function %s\n", __FUNCTION__);
}
+ bool ShouldCheckFilesystems() override {
+ ZX_PANIC("Test should not invoke function %s\n", __FUNCTION__);
+ }
zx_status_t CheckFilesystem() override {
ZX_PANIC("Test should not invoke function %s\n", __FUNCTION__);
}
diff --git a/zircon/system/core/devmgr/fshost/block-watcher.cpp b/zircon/system/core/devmgr/fshost/block-watcher.cpp
index 4266d5d4f3f..ac5b37547bd 100644
--- a/zircon/system/core/devmgr/fshost/block-watcher.cpp
+++ b/zircon/system/core/devmgr/fshost/block-watcher.cpp
@@ -66,8 +66,8 @@ zx_status_t BlockDeviceCallback(int dirfd, int event, const char* name, void* co
} // namespace
-void BlockDeviceWatcher(std::unique_ptr<FsManager> fshost, bool netboot) {
- FilesystemMounter mounter(std::move(fshost), netboot);
+void BlockDeviceWatcher(std::unique_ptr<FsManager> fshost, bool netboot, bool check_filesystems) {
+ FilesystemMounter mounter(std::move(fshost), netboot, check_filesystems);
fbl::unique_fd dirfd(open(kPathBlockDeviceRoot, O_DIRECTORY | O_RDONLY));
if (dirfd) {
diff --git a/zircon/system/core/devmgr/fshost/block-watcher.h b/zircon/system/core/devmgr/fshost/block-watcher.h
index 115a2603629..015946e675e 100644
--- a/zircon/system/core/devmgr/fshost/block-watcher.h
+++ b/zircon/system/core/devmgr/fshost/block-watcher.h
@@ -11,6 +11,6 @@
namespace devmgr {
// Monitors "/dev/class/block" for new devices indefinitely.
-void BlockDeviceWatcher(std::unique_ptr<FsManager> fshost, bool netboot);
+void BlockDeviceWatcher(std::unique_ptr<FsManager> fshost, bool netboot, bool check_filesystems);
} // namespace devmgr
diff --git a/zircon/system/core/devmgr/fshost/filesystem-mounter.h b/zircon/system/core/devmgr/fshost/filesystem-mounter.h
index 656ab6a420b..092a5eb3c17 100644
--- a/zircon/system/core/devmgr/fshost/filesystem-mounter.h
+++ b/zircon/system/core/devmgr/fshost/filesystem-mounter.h
@@ -19,8 +19,9 @@ namespace devmgr {
// and helps clients mount filesystems within the fshost namespace.
class FilesystemMounter {
public:
- FilesystemMounter(std::unique_ptr<FsManager> fshost, bool netboot)
- : fshost_(std::move(fshost)), netboot_(netboot) {}
+ FilesystemMounter(std::unique_ptr<FsManager> fshost, bool netboot,
+ bool check_filesystems)
+ : fshost_(std::move(fshost)), netboot_(netboot), check_filesystems_(check_filesystems) {}
void FuchsiaStart() const { fshost_->FuchsiaStart(); }
@@ -29,6 +30,7 @@ public:
}
bool Netbooting() const { return netboot_; }
+ bool ShouldCheckFilesystems() const { return check_filesystems_; }
// Attempts to mount a block device backed by |fd| to "/data".
// Fails if already mounted.
@@ -44,7 +46,8 @@ public:
private:
std::unique_ptr<FsManager> fshost_;
- bool netboot_ = false;
+ const bool netboot_ = false;
+ const bool check_filesystems_ = false;
bool data_mounted_ = false;
bool install_mounted_ = false;
bool blob_mounted_ = false;
diff --git a/zircon/system/core/devmgr/fshost/fs-manager.h b/zircon/system/core/devmgr/fshost/fs-manager.h
index cca2ddeb3c0..bc9ec5a8295 100644
--- a/zircon/system/core/devmgr/fshost/fs-manager.h
+++ b/zircon/system/core/devmgr/fshost/fs-manager.h
@@ -14,7 +14,7 @@
#include <zircon/thread_annotations.h>
#include <zircon/types.h>
-#include "../shared/env.h"
+// Used for fshost signals.
#include "../shared/fdio.h"
#include "registry.h"
diff --git a/zircon/system/core/devmgr/fshost/main.cpp b/zircon/system/core/devmgr/fshost/main.cpp
index b4391dd29fd..f89984a6ee7 100644
--- a/zircon/system/core/devmgr/fshost/main.cpp
+++ b/zircon/system/core/devmgr/fshost/main.cpp
@@ -22,6 +22,8 @@
#include <zircon/processargs.h>
#include <zircon/status.h>
+#include "../shared/env.h"
+
#include "block-watcher.h"
#include "fs-manager.h"
@@ -218,7 +220,8 @@ int main(int argc, char** argv) {
}
if (!disable_block_watcher) {
- BlockDeviceWatcher(std::move(fs_manager), netboot);
+ bool check_filesystems = devmgr::getenv_bool("zircon.system.filesystem-check", false);
+ BlockDeviceWatcher(std::move(fs_manager), netboot, check_filesystems);
} else {
// Keep the process alive so that the loader service continues to be supplied
// to the devmgr. Otherwise the devmgr will segfault.
diff --git a/zircon/system/ulib/minfs/minfs.cpp b/zircon/system/ulib/minfs/minfs.cpp
index 0bd712f4479..300099d9c0c 100644
--- a/zircon/system/ulib/minfs/minfs.cpp
+++ b/zircon/system/ulib/minfs/minfs.cpp
@@ -126,7 +126,7 @@ zx_status_t CheckSuperblock(const Superblock* info, Bcache* bc) {
TransactionLimits limits(*info);
if ((info->flags & kMinfsFlagFVM) == 0) {
- if (info->dat_block + info->block_count > max) {
+ if (info->dat_block + info->block_count != max) {
FS_TRACE_ERROR("minfs: too large for device\n");
return ZX_ERR_INVALID_ARGS;
}
--
GitLab