update_engine: Do not cancel external powerwash in invalidations Do not cancel powerwash initiated with reason != update_engine during update invalidations. Also keep the powerwash marker file in case if there is an error when fetching the powerwash reason. BUG=None TEST=cros_run_unit_tests --board=BOARD --packages update_engine TEST=tast run -var=tlwAddress=<DUT> autoupdate.OmahaInvalidation.* Change-Id: I6d1d5c5a68476643674bebf7d9a4f0fd9cde7ede Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/4957016 Commit-Queue: Artyom Chen <artyomchen@google.com> Reviewed-by: John Admanski <jadmanski@chromium.org> Reviewed-by: Jae Hoon Kim <kimjae@chromium.org> Tested-by: Artyom Chen <artyomchen@google.com>
diff --git a/common/fake_hardware.h b/common/fake_hardware.h index 65a6897..64b7c11 100644 --- a/common/fake_hardware.h +++ b/common/fake_hardware.h
@@ -19,6 +19,7 @@ #include <map> #include <memory> +#include <optional> #include <string> #include <utility> @@ -299,6 +300,17 @@ } bool IsFWTryNextSlotReset() const { return reset_fw_try_next_slot_; } + std::optional<bool> IsPowerwashScheduledByUpdateEngine() const override { + return is_powerwash_scheduled_by_update_engine_; + } + void SetIsPowerwashScheduledByUpdateEngine(std::optional<bool> value) { + is_powerwash_scheduled_by_update_engine_ = value; + } + + base::FilePath GetPowerwashMarkerFullPath() const override { + return base::FilePath(); + }; + private: bool is_official_build_{true}; bool is_normal_boot_mode_{true}; @@ -318,6 +330,7 @@ int kernel_max_rollforward_{kKernelMaxRollforward}; int firmware_max_rollforward_{kFirmwareMaxRollforward}; int powerwash_count_{kPowerwashCountNotSet}; + std::optional<bool> is_powerwash_scheduled_by_update_engine_{true}; bool powerwash_scheduled_{false}; bool save_rollback_data_{false}; int64_t build_timestamp_{0};
diff --git a/common/hardware_interface.h b/common/hardware_interface.h index 9d7be40..ebae384 100644 --- a/common/hardware_interface.h +++ b/common/hardware_interface.h
@@ -20,6 +20,7 @@ #include <stdint.h> #include <memory> +#include <optional> #include <string> #include <vector> @@ -115,6 +116,12 @@ // recovery don't have this value set. virtual int GetPowerwashCount() const = 0; + // Parses the powerwash marker file and returns if powerwash is + // initiated by update engine. If the file is not found, returns + // std::nullopt. If parsing failed, returns false. + // Otherwise returns if a marker file contains reason=update_engine. + virtual std::optional<bool> IsPowerwashScheduledByUpdateEngine() const = 0; + // Signals that a powerwash (stateful partition wipe) should be performed // after reboot. If |save_rollback_data| is true additional state is // preserved during shutdown that can be restored after the powerwash. @@ -199,6 +206,9 @@ // Resets a RW firmware partition slot to try on next boot to a current slot. // Returns false on failure, true on success. virtual bool ResetFWTryNextSlot() = 0; + + // Returns an absolute path to a powerwash marker file. + virtual base::FilePath GetPowerwashMarkerFullPath() const = 0; }; } // namespace chromeos_update_engine
diff --git a/cros/hardware_chromeos.cc b/cros/hardware_chromeos.cc index c7f72d8..24f4bed 100644 --- a/cros/hardware_chromeos.cc +++ b/cros/hardware_chromeos.cc
@@ -16,6 +16,7 @@ #include "update_engine/cros/hardware_chromeos.h" +#include <optional> #include <utility> #include <base/files/file_path.h> @@ -64,10 +65,14 @@ // a powerwash is performed. const char kPowerwashCountMarker[] = "powerwash_count"; -// The name of the marker file used to trigger powerwash when post-install +// The path of the marker file used to trigger powerwash when post-install // completes successfully so that the device is powerwashed on next reboot. -const char kPowerwashMarkerFile[] = - "/mnt/stateful_partition/factory_install_reset"; +constexpr char kPowerwashMarkerPath[] = + "mnt/stateful_partition/factory_install_reset"; + +// Expected tag in the powerwash marker file that indicates that +// powerwash is initiated by the update engine. +constexpr char kPowerwashReasonUpdateEngineTag[] = "reason=update_engine"; // The name of the marker file used to trigger a save of rollback data // during the next shutdown. @@ -326,29 +331,52 @@ } auto powerwash_command = GeneratePowerwashCommand(save_rollback_data); - bool result = utils::WriteFile( - kPowerwashMarkerFile, powerwash_command.data(), powerwash_command.size()); + const std::string powerwash_marker_full_path = + GetPowerwashMarkerFullPath().value(); + bool result = utils::WriteFile(powerwash_marker_full_path.c_str(), + powerwash_command.data(), + powerwash_command.size()); if (result) { - LOG(INFO) << "Created " << kPowerwashMarkerFile + LOG(INFO) << "Created " << powerwash_marker_full_path << " to powerwash on next reboot (" << "save_rollback_data=" << save_rollback_data << ")"; } else { PLOG(ERROR) << "Error in creating powerwash marker file: " - << kPowerwashMarkerFile; + << powerwash_marker_full_path; } return result; } +std::optional<bool> HardwareChromeOS::IsPowerwashScheduledByUpdateEngine() + const { + const std::string powerwash_marker_full_path = + GetPowerwashMarkerFullPath().value(); + + if (!utils::FileExists(powerwash_marker_full_path.c_str())) { + return std::nullopt; + } + + std::string contents; + if (!utils::ReadFile(powerwash_marker_full_path, &contents)) { + LOG(ERROR) << "Failed to read the powerwash marker file."; + return false; + } + + return contents.find(kPowerwashReasonUpdateEngineTag) != std::string::npos; +} + bool HardwareChromeOS::CancelPowerwash() { - bool result = base::DeleteFile(base::FilePath(kPowerwashMarkerFile)); + const base::FilePath powerwash_marker_full_path = + GetPowerwashMarkerFullPath(); + bool result = base::DeleteFile(powerwash_marker_full_path); if (result) { LOG(INFO) << "Successfully deleted the powerwash marker file : " - << kPowerwashMarkerFile; + << powerwash_marker_full_path; } else { PLOG(ERROR) << "Could not delete the powerwash marker file : " - << kPowerwashMarkerFile; + << powerwash_marker_full_path; } // Delete the rollback save marker file if it existed. @@ -661,4 +689,8 @@ return true; } +base::FilePath HardwareChromeOS::GetPowerwashMarkerFullPath() const { + return root_.Append(kPowerwashMarkerPath); +} + } // namespace chromeos_update_engine
diff --git a/cros/hardware_chromeos.h b/cros/hardware_chromeos.h index f68431e..bce8bc2 100644 --- a/cros/hardware_chromeos.h +++ b/cros/hardware_chromeos.h
@@ -18,6 +18,7 @@ #define UPDATE_ENGINE_CROS_HARDWARE_CHROMEOS_H_ #include <memory> +#include <optional> #include <string> #include <vector> @@ -62,6 +63,7 @@ bool SetMaxFirmwareKeyRollforward(int firmware_max_rollforward) override; bool SetMaxKernelKeyRollforward(int kernel_max_rollforward) override; int GetPowerwashCount() const override; + std::optional<bool> IsPowerwashScheduledByUpdateEngine() const override; // Must not be called prior to boot control initialization. bool SchedulePowerwash(bool save_rollback_data) override; bool CancelPowerwash() override; @@ -91,6 +93,8 @@ bool ResetFWTryNextSlot() override; + base::FilePath GetPowerwashMarkerFullPath() const override; + private: friend class HardwareChromeOSTest; FRIEND_TEST(HardwareChromeOSTest, GeneratePowerwashCommandCheck);
diff --git a/cros/hardware_chromeos_unittest.cc b/cros/hardware_chromeos_unittest.cc index f60337c..1cd58ee 100644 --- a/cros/hardware_chromeos_unittest.cc +++ b/cros/hardware_chromeos_unittest.cc
@@ -17,6 +17,7 @@ #include "update_engine/cros/hardware_chromeos.h" #include <memory> +#include <optional> #include <utility> #include <base/files/file_util.h> @@ -467,4 +468,57 @@ ASSERT_FALSE(result); } +TEST_F(HardwareChromeOSTest, IsPowerwashScheduledByUpdateEngineValidReason) { + base::FilePath root_path = root_dir_.GetPath(); + hardware_.SetRootForTest(root_path); + base::FilePath marker_path = hardware_.GetPowerwashMarkerFullPath(); + std::string marker_contents = "safe reason=update_engine\n test_key"; + ASSERT_TRUE(brillo::TouchFile(marker_path)); + ASSERT_TRUE(base::WriteFile(marker_path, marker_contents)); + + std::optional<bool> result = hardware_.IsPowerwashScheduledByUpdateEngine(); + + EXPECT_TRUE(result); + EXPECT_TRUE(result.value()); +} + +TEST_F(HardwareChromeOSTest, IsPowerwashScheduledByUpdateEngineNoMarker) { + base::FilePath root_path = root_dir_.GetPath(); + hardware_.SetRootForTest(root_path); + base::FilePath marker_path = hardware_.GetPowerwashMarkerFullPath(); + ASSERT_FALSE(base::PathExists(marker_path)); + + std::optional<bool> result = hardware_.IsPowerwashScheduledByUpdateEngine(); + + EXPECT_FALSE(result); +} + +TEST_F(HardwareChromeOSTest, IsPowerwashScheduledByUpdateEngineNoReasonKey) { + base::FilePath root_path = root_dir_.GetPath(); + hardware_.SetRootForTest(root_path); + base::FilePath marker_path = hardware_.GetPowerwashMarkerFullPath(); + std::string marker_contents = "safe reason test_key"; + ASSERT_TRUE(brillo::TouchFile(marker_path)); + ASSERT_TRUE(base::WriteFile(marker_path, marker_contents)); + + std::optional<bool> result = hardware_.IsPowerwashScheduledByUpdateEngine(); + + EXPECT_TRUE(result); + EXPECT_FALSE(result.value()); +} + +TEST_F(HardwareChromeOSTest, IsPowerwashScheduledByUpdateEngineEmptyReason) { + base::FilePath root_path = root_dir_.GetPath(); + hardware_.SetRootForTest(root_path); + base::FilePath marker_path = hardware_.GetPowerwashMarkerFullPath(); + std::string marker_contents = "safe reason=\n test_key"; + ASSERT_TRUE(brillo::TouchFile(marker_path)); + ASSERT_TRUE(base::WriteFile(marker_path, marker_contents)); + + std::optional<bool> result = hardware_.IsPowerwashScheduledByUpdateEngine(); + + EXPECT_TRUE(result); + EXPECT_FALSE(result.value()); +} + } // namespace chromeos_update_engine
diff --git a/cros/update_attempter.cc b/cros/update_attempter.cc index d78eef9..d0eb4a0 100644 --- a/cros/update_attempter.cc +++ b/cros/update_attempter.cc
@@ -1779,9 +1779,19 @@ } LOG(INFO) << "Clearing powerwash and rollback flags, if any."; - if (!SystemState::Get()->hardware()->CancelPowerwash()) { - LOG(WARNING) << "Failed to cancel powerwash. Continuing anyway."; - success = false; + const std::optional<bool> is_powerwash_scheduled_by_update_engine = + SystemState::Get()->hardware()->IsPowerwashScheduledByUpdateEngine(); + if (!is_powerwash_scheduled_by_update_engine) { + LOG(INFO) << "Powerwash is not scheduled, continuing."; + } else if (!is_powerwash_scheduled_by_update_engine.value()) { + LOG(INFO) << "Not cancelling powerwash. Either not initiated by update " + "engine or there was a parsing error."; + } else { + LOG(INFO) << "Cancelling powerwash that was initiated by update engine."; + if (!SystemState::Get()->hardware()->CancelPowerwash()) { + LOG(WARNING) << "Failed to cancel powerwash. Continuing anyway."; + success = false; + } } SystemState::Get()->payload_state()->SetRollbackHappened(false);
diff --git a/cros/update_attempter.h b/cros/update_attempter.h index 5da200e..7df9d81 100644 --- a/cros/update_attempter.h +++ b/cros/update_attempter.h
@@ -374,6 +374,8 @@ FRIEND_TEST(UpdateAttempterTest, FirstUpdateBeforeReboot); FRIEND_TEST(UpdateAttempterTest, InvalidateLastUpdate); FRIEND_TEST(UpdateAttempterTest, InvalidateLastPowerwashUpdate); + FRIEND_TEST(UpdateAttempterTest, InvalidateLastUpdateNoPowerwashFile); + FRIEND_TEST(UpdateAttempterTest, InvalidateLastUpdateExternalPowerwash); FRIEND_TEST(UpdateAttempterTest, ConsecutiveUpdateBeforeRebootSuccess); FRIEND_TEST(UpdateAttempterTest, ConsecutiveUpdateBeforeRebootLimited); FRIEND_TEST(UpdateAttempterTest, ConsecutiveUpdateFailureMetric);
diff --git a/cros/update_attempter_unittest.cc b/cros/update_attempter_unittest.cc index 41da85d..6d0a186 100644 --- a/cros/update_attempter_unittest.cc +++ b/cros/update_attempter_unittest.cc
@@ -22,6 +22,7 @@ #include <limits> #include <map> #include <memory> +#include <optional> #include <string> #include <unordered_set> @@ -2597,6 +2598,9 @@ bool save_rollback_data = true; FakeSystemState::Get()->fake_hardware()->SchedulePowerwash( save_rollback_data); + FakeSystemState::Get() + ->fake_hardware() + ->SetIsPowerwashScheduledByUpdateEngine(true); EXPECT_TRUE(FakeSystemState::Get()->fake_hardware()->IsPowerwashScheduled()); FakeSystemState::Get()->fake_clock()->SetBootTime(Time::FromTimeT(42)); attempter_.WriteUpdateCompletedMarker(); @@ -2616,6 +2620,60 @@ EXPECT_TRUE(FakeSystemState::Get()->fake_hardware()->IsFWTryNextSlotReset()); } +TEST_F(UpdateAttempterTest, InvalidateLastUpdateNoPowerwashFile) { + // Mock a previous update. + bool save_rollback_data = true; + FakeSystemState::Get()->fake_hardware()->SchedulePowerwash( + save_rollback_data); + EXPECT_TRUE(FakeSystemState::Get()->fake_hardware()->IsPowerwashScheduled()); + FakeSystemState::Get() + ->fake_hardware() + ->SetIsPowerwashScheduledByUpdateEngine(std::nullopt); + FakeSystemState::Get()->fake_clock()->SetBootTime(Time::FromTimeT(42)); + attempter_.WriteUpdateCompletedMarker(); + EXPECT_TRUE(attempter_.GetBootTimeAtUpdate(nullptr)); + attempter_.status_ = UpdateStatus::UPDATED_NEED_REBOOT; + EXPECT_CALL(*FakeSystemState::Get()->mock_payload_state(), + SetRollbackHappened(false)) + .Times(1); + + // Invalidate an update. + MockAction action; + attempter_.ActionCompleted( + nullptr, &action, ErrorCode::kInvalidateLastUpdate); + + EXPECT_FALSE(attempter_.GetBootTimeAtUpdate(nullptr)); + EXPECT_TRUE(FakeSystemState::Get()->fake_hardware()->IsPowerwashScheduled()); + EXPECT_TRUE(FakeSystemState::Get()->fake_hardware()->IsFWTryNextSlotReset()); +} + +TEST_F(UpdateAttempterTest, InvalidateLastUpdateExternalPowerwash) { + // Mock a previous update. + bool save_rollback_data = true; + FakeSystemState::Get()->fake_hardware()->SchedulePowerwash( + save_rollback_data); + EXPECT_TRUE(FakeSystemState::Get()->fake_hardware()->IsPowerwashScheduled()); + FakeSystemState::Get() + ->fake_hardware() + ->SetIsPowerwashScheduledByUpdateEngine(false); + FakeSystemState::Get()->fake_clock()->SetBootTime(Time::FromTimeT(42)); + attempter_.WriteUpdateCompletedMarker(); + EXPECT_TRUE(attempter_.GetBootTimeAtUpdate(nullptr)); + attempter_.status_ = UpdateStatus::UPDATED_NEED_REBOOT; + EXPECT_CALL(*FakeSystemState::Get()->mock_payload_state(), + SetRollbackHappened(false)) + .Times(1); + + // Invalidate an update. + MockAction action; + attempter_.ActionCompleted( + nullptr, &action, ErrorCode::kInvalidateLastUpdate); + + EXPECT_FALSE(attempter_.GetBootTimeAtUpdate(nullptr)); + EXPECT_TRUE(FakeSystemState::Get()->fake_hardware()->IsPowerwashScheduled()); + EXPECT_TRUE(FakeSystemState::Get()->fake_hardware()->IsFWTryNextSlotReset()); +} + TEST_F(UpdateAttempterTest, CalculateDlcParamsRemoveStaleMetadata) { string dlc_id = "dlc0"; auto active_key =