From 9e8c41c5113b965d3d96ed46c39da0929dddee75 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Wed, 3 Aug 2022 17:58:33 -0700 Subject: [PATCH 1/3] remount: Move more stuff out of do_remount. This moves more logic out of do_remount and into main(). This eliminates some redundant code. Bug: 241179247 Test: remount Change-Id: I212bdb0e97016dec50618962d7c24f46d35764c7 --- fs_mgr/fs_mgr_remount.cpp | 218 +++++++++++++++++++------------------- 1 file changed, 109 insertions(+), 109 deletions(-) diff --git a/fs_mgr/fs_mgr_remount.cpp b/fs_mgr/fs_mgr_remount.cpp index c47d11048..8bc5681fc 100644 --- a/fs_mgr/fs_mgr_remount.cpp +++ b/fs_mgr/fs_mgr_remount.cpp @@ -91,12 +91,8 @@ void MyLogger(android::base::LogId id, android::base::LogSeverity severity, cons logd(id, severity, tag, file, line, message); } -[[noreturn]] void reboot(bool overlayfs = false) { - if (overlayfs) { - LOG(INFO) << "Successfully setup overlayfs\nrebooting device"; - } else { - LOG(INFO) << "Successfully disabled verity\nrebooting device"; - } +[[noreturn]] void reboot() { + LOG(INFO) << "Rebooting device for new settings to take effect"; ::sync(); android::base::SetProperty(ANDROID_RB_PROPERTY, "reboot,remount"); ::sleep(60); @@ -136,7 +132,6 @@ enum RemountStatus { BINDER_ERROR, CHECKPOINTING, GSID_ERROR, - CLEAN_SCRATCH_FILES, }; static bool ReadFstab(const char* fstab_file, android::fs_mgr::Fstab* fstab) { @@ -266,6 +261,7 @@ struct RemountCheckResult { bool setup_overlayfs = false; bool disabled_verity = false; bool verity_error = false; + bool remounted_anything = false; }; static RemountStatus CheckVerity(const FstabEntry& entry, RemountCheckResult* result) { @@ -422,75 +418,8 @@ static RemountStatus RemountPartition(Fstab& fstab, Fstab& mounts, FstabEntry& e return REMOUNT_FAILED; } -static int do_remount(int argc, char* argv[]) { - // If somehow this executable is delivered on a "user" build, it can - // not function, so providing a clear message to the caller rather than - // letting if fall through and provide a lot of confusing failure messages. - if (!ALLOW_ADBD_DISABLE_VERITY || (android::base::GetProperty("ro.debuggable", "0") != "1")) { - LOG(ERROR) << "only functions on userdebug or eng builds"; - return NOT_USERDEBUG; - } - - const char* fstab_file = nullptr; - auto can_reboot = false; - std::vector partition_args; - - struct option longopts[] = { - {"fstab", required_argument, nullptr, 'T'}, - {"help", no_argument, nullptr, 'h'}, - {"reboot", no_argument, nullptr, 'R'}, - {"verbose", no_argument, nullptr, 'v'}, - {"clean_scratch_files", no_argument, nullptr, 'C'}, - {0, 0, nullptr, 0}, - }; - for (int opt; (opt = ::getopt_long(argc, argv, "hRT:v", longopts, nullptr)) != -1;) { - switch (opt) { - case 'h': - usage(SUCCESS); - break; - case 'R': - can_reboot = true; - break; - case 'T': - if (fstab_file) { - LOG(ERROR) << "Cannot supply two fstabs: -T " << fstab_file << " -T" << optarg; - usage(BADARG); - } - fstab_file = optarg; - break; - case 'v': - verbose = true; - break; - case 'C': - return CLEAN_SCRATCH_FILES; - default: - LOG(ERROR) << "Bad Argument -" << char(opt); - usage(BADARG); - break; - } - } - - for (; argc > optind; ++optind) { - partition_args.emplace_back(argv[optind]); - } - - // Make sure we are root. - if (::getuid() != 0) { - LOG(ERROR) << "Not running as root. Try \"adb root\" first."; - return NOT_ROOT; - } - - // Read the selected fstab. - Fstab fstab; - if (!ReadFstab(fstab_file, &fstab) || fstab.empty()) { - PLOG(ERROR) << "Failed to read fstab"; - return NO_FSTAB; - } - - if (auto rv = VerifyCheckpointing(); rv != REMOUNT_SUCCESS) { - return rv; - } - +static int do_remount(Fstab& fstab, const std::vector& partition_args, + RemountCheckResult* check_result) { Fstab partitions; if (partition_args.empty()) { partitions = GetAllRemountablePartitions(fstab); @@ -501,29 +430,12 @@ static int do_remount(int argc, char* argv[]) { } // Check verity and optionally setup overlayfs backing. - RemountCheckResult check_result; - auto retval = CheckVerityAndOverlayfs(&partitions, &check_result); + auto retval = CheckVerityAndOverlayfs(&partitions, check_result); - bool auto_reboot = check_result.reboot_later && can_reboot; - - // If (1) remount requires a reboot to take effect, (2) system is currently - // running a DSU guest and (3) DSU is disabled, then enable DSU so that the - // next reboot would not take us back to the host system but stay within - // the guest system. - if (auto_reboot) { - if (auto rv = EnableDsuIfNeeded(); rv != REMOUNT_SUCCESS) { - return rv; + if (partitions.empty() || check_result->disabled_verity) { + if (partitions.empty()) { + LOG(WARNING) << "No remountable partitions were found."; } - } - - if (partitions.empty() || check_result.disabled_verity) { - if (auto_reboot) { - reboot(check_result.setup_overlayfs); - } - if (check_result.reboot_later) { - return MUST_REBOOT; - } - LOG(WARNING) << "No partitions to remount"; return retval; } @@ -545,13 +457,10 @@ static int do_remount(int argc, char* argv[]) { for (auto& entry : partitions) { if (auto rv = RemountPartition(fstab, mounts, entry); rv != REMOUNT_SUCCESS) { retval = rv; + } else { + check_result->remounted_anything = true; } } - - if (auto_reboot) reboot(check_result.setup_overlayfs); - if (check_result.reboot_later) { - LOG(INFO) << "Now reboot your device for settings to take effect"; - } return retval; } @@ -565,14 +474,105 @@ int main(int argc, char* argv[]) { if (argc > 0 && android::base::Basename(argv[0]) == "clean_scratch_files"s) { return do_clean_scratch_files(); } - int result = do_remount(argc, argv); - if (result == MUST_REBOOT) { - LOG(INFO) << "Now reboot your device for settings to take effect"; - result = 0; - } else if (result == REMOUNT_SUCCESS) { + + // Make sure we are root. + if (::getuid() != 0) { + LOG(ERROR) << "Not running as root. Try \"adb root\" first."; + return NOT_ROOT; + } + + // If somehow this executable is delivered on a "user" build, it can + // not function, so providing a clear message to the caller rather than + // letting if fall through and provide a lot of confusing failure messages. + if (!ALLOW_ADBD_DISABLE_VERITY || (android::base::GetProperty("ro.debuggable", "0") != "1")) { + LOG(ERROR) << "only functions on userdebug or eng builds"; + return NOT_USERDEBUG; + } + + const char* fstab_file = nullptr; + auto auto_reboot = false; + std::vector partition_args; + + struct option longopts[] = { + {"fstab", required_argument, nullptr, 'T'}, + {"help", no_argument, nullptr, 'h'}, + {"reboot", no_argument, nullptr, 'R'}, + {"verbose", no_argument, nullptr, 'v'}, + {"clean_scratch_files", no_argument, nullptr, 'C'}, + {0, 0, nullptr, 0}, + }; + for (int opt; (opt = ::getopt_long(argc, argv, "hRT:v", longopts, nullptr)) != -1;) { + switch (opt) { + case 'h': + usage(SUCCESS); + break; + case 'R': + auto_reboot = true; + break; + case 'T': + if (fstab_file) { + LOG(ERROR) << "Cannot supply two fstabs: -T " << fstab_file << " -T" << optarg; + usage(BADARG); + } + fstab_file = optarg; + break; + case 'v': + verbose = true; + break; + case 'C': + return do_clean_scratch_files(); + default: + LOG(ERROR) << "Bad Argument -" << char(opt); + usage(BADARG); + break; + } + } + + for (; argc > optind; ++optind) { + partition_args.emplace_back(argv[optind]); + } + + // Make sure checkpointing is disabled if necessary. + if (auto rv = VerifyCheckpointing(); rv != REMOUNT_SUCCESS) { + return rv; + } + + // Read the selected fstab. + Fstab fstab; + if (!ReadFstab(fstab_file, &fstab) || fstab.empty()) { + PLOG(ERROR) << "Failed to read fstab"; + return NO_FSTAB; + } + + RemountCheckResult check_result; + int result = do_remount(fstab, partition_args, &check_result); + + if (check_result.disabled_verity && check_result.setup_overlayfs) { + LOG(INFO) << "Verity disabled; overlayfs enabled."; + } else if (check_result.disabled_verity) { + LOG(INFO) << "Verity disabled."; + } else if (check_result.setup_overlayfs) { + LOG(INFO) << "Overlayfs enabled."; + } + + if (check_result.reboot_later) { + if (auto_reboot) { + // If (1) remount requires a reboot to take effect, (2) system is currently + // running a DSU guest and (3) DSU is disabled, then enable DSU so that the + // next reboot would not take us back to the host system but stay within + // the guest system. + if (auto rv = EnableDsuIfNeeded(); rv != REMOUNT_SUCCESS) { + LOG(ERROR) << "Unable to automatically enable DSU"; + return rv; + } + reboot(); + } else { + LOG(INFO) << "Now reboot your device for settings to take effect"; + } + return MUST_REBOOT; + } + if (result == REMOUNT_SUCCESS) { printf("remount succeeded\n"); - } else if (result == CLEAN_SCRATCH_FILES) { - return do_clean_scratch_files(); } else { printf("remount failed\n"); } From 63432cd3178a70a692bf594094b522112990c6c3 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Wed, 3 Aug 2022 22:08:08 -0700 Subject: [PATCH 2/3] remount: Prevent error spam when remounting fails. Cuttlefish's combined fstab has two entries for every partition, which causes a lot of error spam when remount fails. Fix this by only remounting entries that match an actual mount point (if such a mount point exists). Bug: 241179247 Test: remount on broken kernel Change-Id: I3ddab553706f98b45f83221fd195f481dfdcc5c0 --- fs_mgr/fs_mgr_overlayfs.cpp | 59 +++++++++++++++++++------------ fs_mgr/fs_mgr_remount.cpp | 28 ++++++++++----- fs_mgr/include/fs_mgr_overlayfs.h | 1 + 3 files changed, 58 insertions(+), 30 deletions(-) diff --git a/fs_mgr/fs_mgr_overlayfs.cpp b/fs_mgr/fs_mgr_overlayfs.cpp index b1606d92f..4fe00d1e3 100644 --- a/fs_mgr/fs_mgr_overlayfs.cpp +++ b/fs_mgr/fs_mgr_overlayfs.cpp @@ -370,28 +370,6 @@ bool fs_mgr_rw_access(const std::string& path) { return ret; } -bool fs_mgr_overlayfs_already_mounted(const std::string& mount_point, bool overlay_only = true) { - Fstab fstab; - auto save_errno = errno; - if (!ReadFstabFromFile("/proc/mounts", &fstab)) { - return false; - } - errno = save_errno; - const auto lowerdir = kLowerdirOption + mount_point; - for (const auto& entry : fstab) { - if (overlay_only && "overlay" != entry.fs_type && "overlayfs" != entry.fs_type) continue; - if (mount_point != entry.mount_point) continue; - if (!overlay_only) return true; - const auto options = android::base::Split(entry.fs_options, ","); - for (const auto& opt : options) { - if (opt == lowerdir) { - return true; - } - } - } - return false; -} - constexpr char kOverlayfsFileContext[] = "u:object_r:overlayfs_file:s0"; bool fs_mgr_overlayfs_setup_dir(const std::string& dir, std::string* overlay, bool* change) { @@ -1290,8 +1268,23 @@ bool fs_mgr_wants_overlayfs(FstabEntry* entry) { } Fstab fs_mgr_overlayfs_candidate_list(const Fstab& fstab) { + android::fs_mgr::Fstab mounts; + if (!android::fs_mgr::ReadFstabFromFile("/proc/mounts", &mounts)) { + PLOG(ERROR) << "Failed to read /proc/mounts"; + return {}; + } + Fstab candidates; for (const auto& entry : fstab) { + // Filter out partitions whose type doesn't match what's mounted. + // This avoids spammy behavior on devices which can mount different + // filesystems for each partition. + auto proc_mount_point = (entry.mount_point == "/system") ? "/" : entry.mount_point; + auto mounted = GetEntryForMountPoint(&mounts, proc_mount_point); + if (!mounted || mounted->fs_type != entry.fs_type) { + continue; + } + FstabEntry new_entry = entry; if (!fs_mgr_overlayfs_already_mounted(entry.mount_point) && !fs_mgr_wants_overlayfs(&new_entry)) { @@ -1698,6 +1691,28 @@ void TeardownAllOverlayForMountPoint(const std::string& mount_point) { #endif // ALLOW_ADBD_DISABLE_VERITY != 0 +bool fs_mgr_overlayfs_already_mounted(const std::string& mount_point, bool overlay_only) { + Fstab fstab; + auto save_errno = errno; + if (!ReadFstabFromFile("/proc/mounts", &fstab)) { + return false; + } + errno = save_errno; + const auto lowerdir = kLowerdirOption + mount_point; + for (const auto& entry : fstab) { + if (overlay_only && "overlay" != entry.fs_type && "overlayfs" != entry.fs_type) continue; + if (mount_point != entry.mount_point) continue; + if (!overlay_only) return true; + const auto options = android::base::Split(entry.fs_options, ","); + for (const auto& opt : options) { + if (opt == lowerdir) { + return true; + } + } + } + return false; +} + bool fs_mgr_has_shared_blocks(const std::string& mount_point, const std::string& dev) { struct statfs fs; if ((statfs((mount_point + "/lost+found").c_str(), &fs) == -1) || diff --git a/fs_mgr/fs_mgr_remount.cpp b/fs_mgr/fs_mgr_remount.cpp index 8bc5681fc..d17e4f77b 100644 --- a/fs_mgr/fs_mgr_remount.cpp +++ b/fs_mgr/fs_mgr_remount.cpp @@ -186,8 +186,8 @@ static bool IsRemountable(Fstab& candidates, const FstabEntry& entry) { if (entry.fs_type == "vfat") { return false; } - if (GetEntryForMountPoint(&candidates, entry.mount_point)) { - return true; + if (auto candidate_entry = GetEntryForMountPoint(&candidates, entry.mount_point)) { + return candidate_entry->fs_type == entry.fs_type; } if (GetWrappedEntry(candidates, entry)) { return false; @@ -196,13 +196,22 @@ static bool IsRemountable(Fstab& candidates, const FstabEntry& entry) { } static Fstab::const_iterator FindPartition(const Fstab& fstab, const std::string& partition) { + Fstab mounts; + if (!android::fs_mgr::ReadFstabFromFile("/proc/mounts", &mounts)) { + LOG(ERROR) << "Failed to read /proc/mounts"; + return fstab.end(); + } + for (auto iter = fstab.begin(); iter != fstab.end(); iter++) { const auto mount_point = system_mount_point(*iter); - if (partition == mount_point) { - return iter; - } - if (partition == android::base::Basename(mount_point)) { - return iter; + if (partition == mount_point || partition == android::base::Basename(mount_point)) { + // In case fstab has multiple entries, pick the one that matches the + // actual mounted filesystem type. + auto proc_mount_point = (iter->mount_point == "/system") ? "/" : iter->mount_point; + auto mounted = GetEntryForMountPoint(&mounts, proc_mount_point); + if (mounted && mounted->fs_type == iter->fs_type) { + return iter; + } } } return fstab.end(); @@ -243,7 +252,10 @@ static RemountStatus GetRemountList(const Fstab& fstab, const std::vectormount_point) && + !IsRemountable(candidates, *entry)) { LOG(ERROR) << "Invalid partition " << arg; return INVALID_PARTITION; } diff --git a/fs_mgr/include/fs_mgr_overlayfs.h b/fs_mgr/include/fs_mgr_overlayfs.h index 21d7cd9fa..b328052d7 100644 --- a/fs_mgr/include/fs_mgr_overlayfs.h +++ b/fs_mgr/include/fs_mgr_overlayfs.h @@ -33,6 +33,7 @@ bool fs_mgr_overlayfs_setup(const char* backing = nullptr, const char* mount_poi bool fs_mgr_overlayfs_teardown(const char* mount_point = nullptr, bool* change = nullptr); bool fs_mgr_overlayfs_is_setup(); bool fs_mgr_has_shared_blocks(const std::string& mount_point, const std::string& dev); +bool fs_mgr_overlayfs_already_mounted(const std::string& mount_point, bool overlay_only = true); std::string fs_mgr_get_context(const std::string& mount_point); enum class OverlayfsValidResult { From e9e3f6e01bdfab0c1e7edfe28cadb0421d297c4f Mon Sep 17 00:00:00 2001 From: David Anderson Date: Thu, 4 Aug 2022 11:18:01 -0700 Subject: [PATCH 3/3] remount: Remove the "backing" parameter to fs_mgr_overlayfs_setup. This is unused. Bug: 241179247 Test: remount Change-Id: I7a9e07a4cf397c6fc8909a9959e08d1aefa3216a --- fs_mgr/fs_mgr_overlayfs.cpp | 6 ++---- fs_mgr/fs_mgr_remount.cpp | 2 +- fs_mgr/include/fs_mgr_overlayfs.h | 4 ++-- set-verity-state/set-verity-state.cpp | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/fs_mgr/fs_mgr_overlayfs.cpp b/fs_mgr/fs_mgr_overlayfs.cpp index 4fe00d1e3..0b1eb399e 100644 --- a/fs_mgr/fs_mgr_overlayfs.cpp +++ b/fs_mgr/fs_mgr_overlayfs.cpp @@ -97,7 +97,7 @@ bool fs_mgr_overlayfs_mount_all(Fstab*) { return false; } -bool fs_mgr_overlayfs_setup(const char*, const char*, bool* change, bool) { +bool fs_mgr_overlayfs_setup(const char*, bool* change, bool) { if (change) *change = false; return false; } @@ -1357,8 +1357,7 @@ bool fs_mgr_overlayfs_mount_all(Fstab* fstab) { // Returns false if setup not permitted, errno set to last error. // If something is altered, set *change. -bool fs_mgr_overlayfs_setup(const char* backing, const char* mount_point, bool* change, - bool force) { +bool fs_mgr_overlayfs_setup(const char* mount_point, bool* change, bool force) { if (change) *change = false; auto ret = false; if (fs_mgr_overlayfs_valid() == OverlayfsValidResult::kNotSupported) return ret; @@ -1395,7 +1394,6 @@ bool fs_mgr_overlayfs_setup(const char* backing, const char* mount_point, bool* std::string dir; for (const auto& overlay_mount_point : OverlayMountPoints()) { - if (backing && backing[0] && (overlay_mount_point != backing)) continue; if (overlay_mount_point == kScratchMountPoint) { if (!fs_mgr_overlayfs_setup_scratch(fstab, change)) continue; } else { diff --git a/fs_mgr/fs_mgr_remount.cpp b/fs_mgr/fs_mgr_remount.cpp index d17e4f77b..86d095ec4 100644 --- a/fs_mgr/fs_mgr_remount.cpp +++ b/fs_mgr/fs_mgr_remount.cpp @@ -320,7 +320,7 @@ static RemountStatus CheckVerityAndOverlayfs(Fstab* partitions, RemountCheckResu if (fs_mgr_wants_overlayfs(&entry)) { bool change = false; bool force = result->disabled_verity; - if (!fs_mgr_overlayfs_setup(nullptr, mount_point.c_str(), &change, force)) { + if (!fs_mgr_overlayfs_setup(mount_point.c_str(), &change, force)) { LOG(ERROR) << "Overlayfs setup for " << mount_point << " failed, skipping"; status = BAD_OVERLAY; it = partitions->erase(it); diff --git a/fs_mgr/include/fs_mgr_overlayfs.h b/fs_mgr/include/fs_mgr_overlayfs.h index b328052d7..ec1d78f2f 100644 --- a/fs_mgr/include/fs_mgr_overlayfs.h +++ b/fs_mgr/include/fs_mgr_overlayfs.h @@ -28,8 +28,8 @@ android::fs_mgr::Fstab fs_mgr_overlayfs_candidate_list(const android::fs_mgr::Fs bool fs_mgr_wants_overlayfs(android::fs_mgr::FstabEntry* entry); bool fs_mgr_overlayfs_mount_all(android::fs_mgr::Fstab* fstab); -bool fs_mgr_overlayfs_setup(const char* backing = nullptr, const char* mount_point = nullptr, - bool* change = nullptr, bool force = true); +bool fs_mgr_overlayfs_setup(const char* mount_point = nullptr, bool* change = nullptr, + bool force = true); bool fs_mgr_overlayfs_teardown(const char* mount_point = nullptr, bool* change = nullptr); bool fs_mgr_overlayfs_is_setup(); bool fs_mgr_has_shared_blocks(const std::string& mount_point, const std::string& dev); diff --git a/set-verity-state/set-verity-state.cpp b/set-verity-state/set-verity-state.cpp index d8e3d315c..ac91ca08d 100644 --- a/set-verity-state/set-verity-state.cpp +++ b/set-verity-state/set-verity-state.cpp @@ -52,7 +52,7 @@ static bool overlayfs_setup(bool enable) { auto change = false; errno = 0; if (enable ? fs_mgr_overlayfs_teardown(nullptr, &change) - : fs_mgr_overlayfs_setup(nullptr, nullptr, &change)) { + : fs_mgr_overlayfs_setup(nullptr, &change)) { if (change) { printf("%s overlayfs\n", enable ? "disabling" : "using"); }