update_engine: Synchronous calls into cgpt functions/scripts
There were frequent occurances of concurrent writes into the GPT
partition table that caused it to get corrupted.
During download action in CrOS, there would be instances where it kicked
off an asynchronous subprocess to run `chromeos-setgoodkernel` script
while in the code flow it also triggers a write into the GPT partition
table using the cgpt library. This change fixes those issues by calling
into the `chromeos-setgoodkernel` script synchronously, but leaving the
locations that used the asynchronous calls correctly as is.
BUG=b:189403353
TEST=FEATURES=test emerge-$B update_engine
Change-Id: I0b3b2aba092620fcb0ec1c7cc8fdf5bc82ac073c
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/2935000
Commit-Queue: Jae Hoon Kim <kimjae@chromium.org>
Tested-by: Jae Hoon Kim <kimjae@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>
(cherry picked from commit 03471df330fb07fa9d0347147e0116c2fc1d5e87)
Reviewed-on: https://chromium-review.googlesource.com/c/aosp/platform/system/update_engine/+/2938036
Tested-by: Amin Hassani <ahassani@chromium.org>
Auto-Submit: Jae Hoon Kim <kimjae@chromium.org>
Commit-Queue: Amin Hassani <ahassani@chromium.org>
diff --git a/common/boot_control_interface.h b/common/boot_control_interface.h
index c93de5c..c520945 100644
--- a/common/boot_control_interface.h
+++ b/common/boot_control_interface.h
@@ -89,6 +89,10 @@
// method doesn't change the value of GetCurrentSlot() on the current boot.
virtual bool SetActiveBootSlot(Slot slot) = 0;
+ // Mark the current slot as successfully booted synchronously. No other slot
+ // flags are modified. Returns false on failure.
+ virtual bool MarkBootSuccessful() = 0;
+
// Mark the current slot as successfully booted asynchronously. No other slot
// flags are modified. Returns false if it was not able to schedule the
// operation, otherwise, returns true and calls the |callback| with the result
diff --git a/common/boot_control_stub.cc b/common/boot_control_stub.cc
index 907f670..f4420b3 100644
--- a/common/boot_control_stub.cc
+++ b/common/boot_control_stub.cc
@@ -66,6 +66,10 @@
return false;
}
+bool BootControlStub::MarkBootSuccessful() {
+ return false;
+}
+
bool BootControlStub::MarkBootSuccessfulAsync(
base::Callback<void(bool)> callback) {
// This is expected to be called on update_engine startup.
diff --git a/common/boot_control_stub.h b/common/boot_control_stub.h
index a1bdb96..9096818 100644
--- a/common/boot_control_stub.h
+++ b/common/boot_control_stub.h
@@ -51,6 +51,7 @@
bool IsSlotBootable(BootControlInterface::Slot slot) const override;
bool MarkSlotUnbootable(BootControlInterface::Slot slot) override;
bool SetActiveBootSlot(BootControlInterface::Slot slot) override;
+ bool MarkBootSuccessful() override;
bool MarkBootSuccessfulAsync(base::Callback<void(bool)> callback) override;
bool IsSlotMarkedSuccessful(BootControlInterface::Slot slot) const override;
DynamicPartitionControlInterface* GetDynamicPartitionControl() override;
diff --git a/common/fake_boot_control.h b/common/fake_boot_control.h
index 98b93e6..d14ef22 100644
--- a/common/fake_boot_control.h
+++ b/common/fake_boot_control.h
@@ -82,6 +82,11 @@
bool SetActiveBootSlot(Slot slot) override { return true; }
+ bool MarkBootSuccessful() override {
+ is_marked_successful_[GetCurrentSlot()] = true;
+ return true;
+ }
+
bool MarkBootSuccessfulAsync(base::Callback<void(bool)> callback) override {
// We run the callback directly from here to avoid having to setup a message
// loop in the test environment.
diff --git a/cros/boot_control_chromeos.cc b/cros/boot_control_chromeos.cc
index a268bdf..174d673 100644
--- a/cros/boot_control_chromeos.cc
+++ b/cros/boot_control_chromeos.cc
@@ -53,6 +53,8 @@
const char kPartitionNameDlcB[] = "dlc_b";
const char kPartitionNameDlcImage[] = "dlc.img";
+constexpr char kSetGoodKernel[] = "/usr/sbin/chromeos-setgoodkernel";
+
// Returns the currently booted rootfs partition. "/dev/sda3", for example.
string GetBootDevice() {
char boot_path[PATH_MAX];
@@ -344,10 +346,22 @@
return true;
}
+bool BootControlChromeOS::MarkBootSuccessful() {
+ int ret;
+ string out, err;
+ if (!Subprocess::Get().SynchronousExec({kSetGoodKernel}, &ret, &out, &err) ||
+ ret != 0) {
+ LOG(ERROR) << "Failed to setgoodkernel, returncode=" << ret
+ << " stdout=" << out << " stderr=" << err;
+ return false;
+ }
+ return true;
+}
+
bool BootControlChromeOS::MarkBootSuccessfulAsync(
base::Callback<void(bool)> callback) {
return Subprocess::Get().Exec(
- {"/usr/sbin/chromeos-setgoodkernel"},
+ {kSetGoodKernel},
base::Bind(&OnMarkBootSuccessfulDone, callback)) != 0;
}
diff --git a/cros/boot_control_chromeos.h b/cros/boot_control_chromeos.h
index 0dff2c0..7d5f2f5 100644
--- a/cros/boot_control_chromeos.h
+++ b/cros/boot_control_chromeos.h
@@ -56,6 +56,7 @@
bool IsSlotBootable(BootControlInterface::Slot slot) const override;
bool MarkSlotUnbootable(BootControlInterface::Slot slot) override;
bool SetActiveBootSlot(BootControlInterface::Slot slot) override;
+ bool MarkBootSuccessful() override;
bool MarkBootSuccessfulAsync(base::Callback<void(bool)> callback) override;
bool IsSlotMarkedSuccessful(BootControlInterface::Slot slot) const override;
DynamicPartitionControlInterface* GetDynamicPartitionControl() override;
diff --git a/cros/update_attempter.cc b/cros/update_attempter.cc
index 197f7a1..ab93a36 100644
--- a/cros/update_attempter.cc
+++ b/cros/update_attempter.cc
@@ -1488,8 +1488,9 @@
// Mark the current slot as successful again, since marking it as active
// may reset the successful bit. We ignore the result of whether marking
- // the current slot as successful worked.
- if (!boot_control->MarkBootSuccessfulAsync(Bind([](bool successful) {}))) {
+ // the current slot as successful worked. This call must be synchronous as
+ // concurrent calls into `cgpt` can cause corrupt GPT headers.
+ if (!boot_control->MarkBootSuccessful()) {
LOG(WARNING) << "Unable to mark the current slot as successfully booted.";
success = false;
}