From fc1e1aeef9e152f5db4b2b9fce7aab16df03d6cd Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Tue, 7 Jul 2020 13:23:07 -0700 Subject: [PATCH 1/2] Try locking after mounting metadata In rescue mode, if /metadata is mounted but /metadata/ota does not exist, immeidately unmount /metadata and fallback to the code path when /metadata is not mounted; that is, old partitions are overwritten. Test: in recovery, select wipe then immediately sideload Bug: 160457903 Change-Id: I412d62b7005c81a7126106edc471622e6a7ef813 Merged-In: I412d62b7005c81a7126106edc471622e6a7ef813 --- fs_mgr/libsnapshot/snapshot.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 55214f545..688b65820 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -2520,7 +2520,19 @@ std::unique_ptr SnapshotManager::EnsureMetadataMounted() { LOG(INFO) << "EnsureMetadataMounted does nothing in Android mode."; return std::unique_ptr(new AutoUnmountDevice()); } - return AutoUnmountDevice::New(device_->GetMetadataDir()); + auto ret = AutoUnmountDevice::New(device_->GetMetadataDir()); + if (ret == nullptr) return nullptr; + + // In rescue mode, it is possible to erase and format metadata, but /metadata/ota is not + // created to execute snapshot updates. Hence, subsequent calls is likely to fail because + // Lock*() fails. By failing early and returning nullptr here, update_engine_sideload can + // treat this case as if /metadata is not mounted. + if (!LockShared()) { + LOG(WARNING) << "/metadata is mounted, but errors occur when acquiring a shared lock. " + "Subsequent calls to SnapshotManager will fail. Unmounting /metadata now."; + return nullptr; + } + return ret; } bool SnapshotManager::HandleImminentDataWipe(const std::function& callback) { From c96ab99136499d7b123f3745b85b53829c44e9be Mon Sep 17 00:00:00 2001 From: Yifan Hong Date: Tue, 7 Jul 2020 14:28:41 -0700 Subject: [PATCH 2/2] Wrap flock with TEMP_FAILURE_RETRY. flock may return EINTR. There are code using LockShared() to test existance of the directory. Don't fail spuriously. Test: pass Bug: 160457903 Change-Id: I51628abe05599422eb3f344781d8f3acd653c822 Merged-In: I51628abe05599422eb3f344781d8f3acd653c822 --- fs_mgr/libsnapshot/snapshot.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fs_mgr/libsnapshot/snapshot.cpp b/fs_mgr/libsnapshot/snapshot.cpp index 688b65820..7488bdaca 100644 --- a/fs_mgr/libsnapshot/snapshot.cpp +++ b/fs_mgr/libsnapshot/snapshot.cpp @@ -1846,7 +1846,7 @@ auto SnapshotManager::OpenFile(const std::string& file, int lock_flags) PLOG(ERROR) << "Open failed: " << file; return nullptr; } - if (lock_flags != 0 && flock(fd, lock_flags) < 0) { + if (lock_flags != 0 && TEMP_FAILURE_RETRY(flock(fd, lock_flags)) < 0) { PLOG(ERROR) << "Acquire flock failed: " << file; return nullptr; } @@ -1857,7 +1857,7 @@ auto SnapshotManager::OpenFile(const std::string& file, int lock_flags) } SnapshotManager::LockedFile::~LockedFile() { - if (flock(fd_, LOCK_UN) < 0) { + if (TEMP_FAILURE_RETRY(flock(fd_, LOCK_UN)) < 0) { PLOG(ERROR) << "Failed to unlock file: " << path_; } }