From f2f5edd9d526ff91f03a0bee0997b660cc8b4f34 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 Merged-In: Ief6dea3ba76323044e61307272dda320a4494aea --- fastboot/device/commands.cpp | 2 +- .../include/libsnapshot/snapshot.h | 5 ++ fs_mgr/libsnapshot/snapshot.cpp | 55 ++++++++++++++++++- fs_mgr/libsnapshot/snapshot_test.cpp | 46 ++++++++++++++++ 4 files changed, 106 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/snapshot.h b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h index 1daa83b8a..5065508c7 100644 --- a/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h +++ b/fs_mgr/libsnapshot/include/libsnapshot/snapshot.h @@ -223,6 +223,10 @@ class SnapshotManager final { // optional callback fires periodically to query progress via GetUpdateState. bool HandleImminentDataWipe(const std::function& callback = {}); + // Force a merge to complete in recovery. This is similar to HandleImminentDataWipe + // but does not expect a data wipe after. + bool FinishMergeInRecovery(); + // 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. @@ -541,6 +545,7 @@ class SnapshotManager final { 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/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index c9fa28e9f..73efcfd80 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_test.cpp b/fs_mgr/libsnapshot/snapshot_test.cpp index f82c082bf..9ca2412df 100644 --- a/fs_mgr/libsnapshot/snapshot_test.cpp +++ b/fs_mgr/libsnapshot/snapshot_test.cpp @@ -1454,6 +1454,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.