From 5a0177d9458ddd7dccef612ee3a52597a26db928 Mon Sep 17 00:00:00 2001 From: David Anderson Date: Thu, 30 Apr 2020 18:53:23 -0700 Subject: [PATCH] fastboot: Fix snapshot-update merge behavior. When merging in recovery, the "imminent data wipe" code was used, which made the assumption the /metadata and /data state would be zapped. This caused future OTAs to error because the old snapshots were detected. This CL allows OTAs to proceed even if unexpected snapshots are present. It also forces the state to "MergeCompleted" after a merge in recovery, so that the next normal boot can perform cleanup. Bug: 155339165 Test: fastboot snapshot-update merge, then take another OTA vts_libsnapshot_test Change-Id: Ief6dea3ba76323044e61307272dda320a4494aea --- fastboot/device/commands.cpp | 2 +- .../include/libsnapshot/mock_snapshot.h | 1 + .../include/libsnapshot/snapshot.h | 6 ++ .../include/libsnapshot/snapshot_stub.h | 1 + fs_mgr/libsnapshot/snapshot.cpp | 55 ++++++++++++++++++- fs_mgr/libsnapshot/snapshot_stub.cpp | 5 ++ fs_mgr/libsnapshot/snapshot_test.cpp | 46 ++++++++++++++++ 7 files changed, 114 insertions(+), 2 deletions(-) diff --git a/fastboot/device/commands.cpp b/fastboot/device/commands.cpp index b8eee4ac1..25533535b 100644 --- a/fastboot/device/commands.cpp +++ b/fastboot/device/commands.cpp @@ -625,7 +625,7 @@ bool SnapshotUpdateHandler(FastbootDevice* device, const std::vectorWriteFail("Unable to create SnapshotManager"); } - if (!sm->HandleImminentDataWipe()) { + if (!sm->FinishMergeInRecovery()) { return device->WriteFail("Unable to finish snapshot merge"); } } else { diff --git a/fs_mgr/libsnapshot/include/libsnapshot/mock_snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/mock_snapshot.h index 758d66cc2..cf0b08519 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/mock_snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/mock_snapshot.h @@ -43,6 +43,7 @@ class MockSnapshotManager : public ISnapshotManager { (const std::string& super_device, const std::chrono::milliseconds& timeout_ms), (override)); MOCK_METHOD(bool, HandleImminentDataWipe, (const std::function& callback), (override)); + MOCK_METHOD(bool, FinishMergeInRecovery, (), (override)); MOCK_METHOD(CreateResult, RecoveryCreateSnapshotDevices, (), (override)); MOCK_METHOD(CreateResult, RecoveryCreateSnapshotDevices, (const std::unique_ptr& metadata_device), (override)); diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h index 4658fb432..5acd039ca 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -202,6 +202,10 @@ class ISnapshotManager { // optional callback fires periodically to query progress via GetUpdateState. virtual bool HandleImminentDataWipe(const std::function& callback = {}) = 0; + // Force a merge to complete in recovery. This is similar to HandleImminentDataWipe + // but does not expect a data wipe after. + virtual bool FinishMergeInRecovery() = 0; + // This method is only allowed in recovery and is used as a helper to // initialize the snapshot devices as a requirement to mount a snapshotted // /system in recovery. @@ -290,6 +294,7 @@ class SnapshotManager final : public ISnapshotManager { const std::string& super_device, const std::chrono::milliseconds& timeout_ms = {}) override; bool HandleImminentDataWipe(const std::function& callback = {}) override; + bool FinishMergeInRecovery() override; CreateResult RecoveryCreateSnapshotDevices() override; CreateResult RecoveryCreateSnapshotDevices( const std::unique_ptr& metadata_device) override; @@ -580,6 +585,7 @@ class SnapshotManager final : public ISnapshotManager { std::unique_ptr device_; std::unique_ptr images_; bool has_local_image_manager_ = false; + bool in_factory_data_reset_ = false; }; } // namespace snapshot diff --git a/fs_mgr/libsnapshot/include/libsnapshot/snapshot_stub.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot_stub.h index 9b2590ccd..9c8290697 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot_stub.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot_stub.h @@ -41,6 +41,7 @@ class SnapshotManagerStub : public ISnapshotManager { const std::string& super_device, const std::chrono::milliseconds& timeout_ms = {}) override; bool HandleImminentDataWipe(const std::function& callback = {}) override; + bool FinishMergeInRecovery() override; CreateResult RecoveryCreateSnapshotDevices() override; CreateResult RecoveryCreateSnapshotDevices( const std::unique_ptr& metadata_device) override; diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index eafeea215..03efd688e 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -577,8 +577,16 @@ bool SnapshotManager::InitiateMerge() { return false; } + auto other_suffix = device_->GetOtherSlotSuffix(); + auto& dm = DeviceMapper::Instance(); for (const auto& snapshot : snapshots) { + if (android::base::EndsWith(snapshot, other_suffix)) { + // Allow the merge to continue, but log this unexpected case. + LOG(ERROR) << "Unexpected snapshot found during merge: " << snapshot; + continue; + } + // The device has to be mapped, since everything should be merged at // the same time. This is a fairly serious error. We could forcefully // map everything here, but it should have been mapped during first- @@ -1008,6 +1016,15 @@ std::string SnapshotManager::GetForwardMergeIndicatorPath() { } void SnapshotManager::AcknowledgeMergeSuccess(LockedFile* lock) { + // It's not possible to remove update state in recovery, so write an + // indicator that cleanup is needed on reboot. If a factory data reset + // was requested, it doesn't matter, everything will get wiped anyway. + // To make testing easier we consider a /data wipe as cleaned up. + if (device_->IsRecovery() && !in_factory_data_reset_) { + WriteUpdateState(lock, UpdateState::MergeCompleted); + return; + } + RemoveAllUpdateState(lock); } @@ -2528,7 +2545,43 @@ bool SnapshotManager::HandleImminentDataWipe(const std::function& callba } return true; }; - if (!ProcessUpdateStateOnDataWipe(true /* allow_forward_merge */, process_callback)) { + + in_factory_data_reset_ = true; + bool ok = ProcessUpdateStateOnDataWipe(true /* allow_forward_merge */, process_callback); + in_factory_data_reset_ = false; + + if (!ok) { + return false; + } + + // Nothing should be depending on partitions now, so unmap them all. + if (!UnmapAllPartitions()) { + LOG(ERROR) << "Unable to unmap all partitions; fastboot may fail to flash."; + } + return true; +} + +bool SnapshotManager::FinishMergeInRecovery() { + if (!device_->IsRecovery()) { + LOG(ERROR) << "Data wipes are only allowed in recovery."; + return false; + } + + auto mount = EnsureMetadataMounted(); + if (!mount || !mount->HasDevice()) { + return false; + } + + auto slot_number = SlotNumberForSlotSuffix(device_->GetSlotSuffix()); + auto super_path = device_->GetSuperDevice(slot_number); + if (!CreateLogicalAndSnapshotPartitions(super_path)) { + LOG(ERROR) << "Unable to map partitions to complete merge."; + return false; + } + + UpdateState state = ProcessUpdateState(); + if (state != UpdateState::MergeCompleted) { + LOG(ERROR) << "Merge returned unexpected status: " << state; return false; } diff --git a/fs_mgr/libsnapshot/snapshot_stub.cpp b/fs_mgr/libsnapshot/snapshot_stub.cpp index 2747b037f..2aaa78c85 100644 --- a/fs_mgr/libsnapshot/snapshot_stub.cpp +++ b/fs_mgr/libsnapshot/snapshot_stub.cpp @@ -89,6 +89,11 @@ bool SnapshotManagerStub::HandleImminentDataWipe(const std::function&) { return false; } +bool SnapshotManagerStub::FinishMergeInRecovery() { + LOG(ERROR) << __FUNCTION__ << " should never be called."; + return false; +} + CreateResult SnapshotManagerStub::RecoveryCreateSnapshotDevices() { LOG(ERROR) << __FUNCTION__ << " should never be called."; return CreateResult::ERROR; diff --git a/fs_mgr/libsnapshot/snapshot_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index 53a37bebd..2bd013599 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -1459,6 +1459,52 @@ TEST_F(SnapshotUpdateTest, MergeInRecovery) { ASSERT_EQ(new_sm->GetUpdateState(), UpdateState::None); } +// Test that a merge does not clear the snapshot state in fastboot. +TEST_F(SnapshotUpdateTest, MergeInFastboot) { + // Execute the first update. + ASSERT_TRUE(sm->BeginUpdate()); + ASSERT_TRUE(sm->CreateUpdateSnapshots(manifest_)); + ASSERT_TRUE(MapUpdateSnapshots()); + ASSERT_TRUE(sm->FinishedSnapshotWrites(false)); + + // Simulate shutting down the device. + ASSERT_TRUE(UnmapAll()); + + // After reboot, init does first stage mount. + auto init = SnapshotManager::NewForFirstStageMount(new TestDeviceInfo(fake_super, "_b")); + ASSERT_NE(init, nullptr); + ASSERT_TRUE(init->NeedSnapshotsInFirstStageMount()); + ASSERT_TRUE(init->CreateLogicalAndSnapshotPartitions("super", snapshot_timeout_)); + init = nullptr; + + // Initiate the merge and then immediately stop it to simulate a reboot. + auto new_sm = SnapshotManager::New(new TestDeviceInfo(fake_super, "_b")); + ASSERT_TRUE(new_sm->InitiateMerge()); + ASSERT_TRUE(UnmapAll()); + + // Simulate a reboot into recovery. + auto test_device = std::make_unique(fake_super, "_b"); + test_device->set_recovery(true); + new_sm = SnapshotManager::NewForFirstStageMount(test_device.release()); + + ASSERT_TRUE(new_sm->FinishMergeInRecovery()); + + auto mount = new_sm->EnsureMetadataMounted(); + ASSERT_TRUE(mount && mount->HasDevice()); + ASSERT_EQ(new_sm->ProcessUpdateState(), UpdateState::MergeCompleted); + + // Finish the merge in a normal boot. + test_device = std::make_unique(fake_super, "_b"); + init = SnapshotManager::NewForFirstStageMount(test_device.release()); + ASSERT_TRUE(init->CreateLogicalAndSnapshotPartitions("super", snapshot_timeout_)); + init = nullptr; + + test_device = std::make_unique(fake_super, "_b"); + new_sm = SnapshotManager::NewForFirstStageMount(test_device.release()); + ASSERT_EQ(new_sm->ProcessUpdateState(), UpdateState::MergeCompleted); + ASSERT_EQ(new_sm->ProcessUpdateState(), UpdateState::None); +} + // Test that after an OTA, before a merge, we can wipe data in recovery. TEST_F(SnapshotUpdateTest, DataWipeRollbackInRecovery) { // Execute the first update.