Mark the active slot from update_engine instead of /postinstall.
In Chrome OS, we were reliying on the /postinst script to generate the
verity hashes and mark the new kernel as bootable. This means that we
also need to run /postinst from the other (not verified) slot when
doing a user-initiated rollback. The update_engine already interacts
with the bootloader via the BootControlInterface to mark the other slot
as unbootable and check if there are other slots available for
rollback.
This patch moves the responsibility of marking the new slot as bootable
from the /postinst script to the update_engine, introducing a new
SetActiveBootSlot() method in the BootControlInterface. Chrome OS
builds will continue to mark the new slot as active from /postinstall
in order to be compatible with old builds, resulting in the new slot
marked as active twice during a successful normal update.
Bug: 23523562
Test: cros flash and image to the new daemon; rolled it back
Change-Id: I02502d7b8e85523a6eb9a7721053739e8381d266
diff --git a/boot_control_android.cc b/boot_control_android.cc
index 43a1c18..7cf42b6 100644
--- a/boot_control_android.cc
+++ b/boot_control_android.cc
@@ -142,7 +142,8 @@
const char* suffix = module_->getSuffix(module_, slot);
if (suffix == nullptr) {
- LOG(ERROR) << "boot_control impl returned no suffix for slot " << slot;
+ LOG(ERROR) << "boot_control impl returned no suffix for slot "
+ << SlotName(slot);
return false;
}
@@ -159,7 +160,7 @@
bool BootControlAndroid::IsSlotBootable(Slot slot) const {
int ret = module_->isSlotBootable(module_, slot);
if (ret < 0) {
- LOG(ERROR) << "Unable to determine if slot " << slot
+ LOG(ERROR) << "Unable to determine if slot " << SlotName(slot)
<< " is bootable: " << strerror(-ret);
return false;
}
@@ -169,13 +170,22 @@
bool BootControlAndroid::MarkSlotUnbootable(Slot slot) {
int ret = module_->setSlotAsUnbootable(module_, slot);
if (ret < 0) {
- LOG(ERROR) << "Unable to mark slot " << slot
+ LOG(ERROR) << "Unable to mark slot " << SlotName(slot)
<< " as bootable: " << strerror(-ret);
return false;
}
return ret == 0;
}
+bool BootControlAndroid::SetActiveBootSlot(Slot slot) {
+ int ret = module_->setActiveBootSlot(module_, slot);
+ if (ret < 0) {
+ LOG(ERROR) << "Unable to set the active slot to slot " << SlotName(slot)
+ << ": " << strerror(-ret);
+ }
+ return ret == 0;
+}
+
bool BootControlAndroid::MarkBootSuccessfulAsync(
base::Callback<void(bool)> callback) {
int ret = module_->markBootSuccessful(module_);
diff --git a/boot_control_android.h b/boot_control_android.h
index f429d9b..8f4f2f9 100644
--- a/boot_control_android.h
+++ b/boot_control_android.h
@@ -45,6 +45,7 @@
std::string* device) const override;
bool IsSlotBootable(BootControlInterface::Slot slot) const override;
bool MarkSlotUnbootable(BootControlInterface::Slot slot) override;
+ bool SetActiveBootSlot(BootControlInterface::Slot slot) override;
bool MarkBootSuccessfulAsync(base::Callback<void(bool)> callback) override;
private:
diff --git a/boot_control_chromeos.cc b/boot_control_chromeos.cc
index 5f8f74e..d9a38b0 100644
--- a/boot_control_chromeos.cc
+++ b/boot_control_chromeos.cc
@@ -126,8 +126,8 @@
}
LOG(INFO) << "Booted from slot " << current_slot_ << " (slot "
- << BootControlInterface::SlotName(current_slot_) << ") of "
- << num_slots_ << " slots present on disk " << boot_disk_name_;
+ << SlotName(current_slot_) << ") of " << num_slots_
+ << " slots present on disk " << boot_disk_name_;
return true;
}
@@ -172,8 +172,7 @@
}
bool BootControlChromeOS::MarkSlotUnbootable(Slot slot) {
- LOG(INFO) << "Marking slot " << BootControlInterface::SlotName(slot)
- << " unbootable";
+ LOG(INFO) << "Marking slot " << SlotName(slot) << " unbootable";
if (slot == current_slot_) {
LOG(ERROR) << "Refusing to mark current slot as unbootable.";
@@ -205,6 +204,48 @@
return true;
}
+bool BootControlChromeOS::SetActiveBootSlot(Slot slot) {
+ LOG(INFO) << "Marking slot " << SlotName(slot) << " active.";
+
+ int partition_num = GetPartitionNumber(kChromeOSPartitionNameKernel, slot);
+ if (partition_num < 0)
+ return false;
+
+ CgptPrioritizeParams prio_params;
+ memset(&prio_params, 0, sizeof(prio_params));
+
+ prio_params.drive_name = const_cast<char*>(boot_disk_name_.c_str());
+ prio_params.set_partition = partition_num;
+
+ prio_params.max_priority = 0;
+
+ int retval = CgptPrioritize(&prio_params);
+ if (retval != CGPT_OK) {
+ LOG(ERROR) << "Unable to set highest priority for slot " << SlotName(slot)
+ << " (partition " << partition_num << ").";
+ return false;
+ }
+
+ CgptAddParams add_params;
+ memset(&add_params, 0, sizeof(add_params));
+
+ add_params.drive_name = const_cast<char*>(boot_disk_name_.c_str());
+ add_params.partition = partition_num;
+
+ add_params.tries = 6;
+ add_params.set_tries = true;
+
+ retval = CgptSetAttributes(&add_params);
+ if (retval != CGPT_OK) {
+ LOG(ERROR) << "Unable to set NumTriesLeft to " << add_params.tries
+ << " for slot " << SlotName(slot) << " (partition "
+ << partition_num << ").";
+ return false;
+ }
+
+ return true;
+}
+
bool BootControlChromeOS::MarkBootSuccessfulAsync(
base::Callback<void(bool)> callback) {
return Subprocess::Get().Exec(
diff --git a/boot_control_chromeos.h b/boot_control_chromeos.h
index 6f5497b..6002712 100644
--- a/boot_control_chromeos.h
+++ b/boot_control_chromeos.h
@@ -48,6 +48,7 @@
std::string* device) const override;
bool IsSlotBootable(BootControlInterface::Slot slot) const override;
bool MarkSlotUnbootable(BootControlInterface::Slot slot) override;
+ bool SetActiveBootSlot(BootControlInterface::Slot slot) override;
bool MarkBootSuccessfulAsync(base::Callback<void(bool)> callback) override;
private:
diff --git a/boot_control_interface.h b/boot_control_interface.h
index 36f2caf..2fe6890 100644
--- a/boot_control_interface.h
+++ b/boot_control_interface.h
@@ -65,6 +65,12 @@
// Returns true on success.
virtual bool MarkSlotUnbootable(Slot slot) = 0;
+ // Set the passed |slot| as the preferred boot slot. Returns whether it
+ // succeeded setting the active slot. If succeeded, on next boot the
+ // bootloader will attempt to load the |slot| marked as active. Note that this
+ // 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 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/fake_boot_control.h b/fake_boot_control.h
index 9d1b54b..f5dcdd6 100644
--- a/fake_boot_control.h
+++ b/fake_boot_control.h
@@ -65,6 +65,8 @@
return true;
}
+ bool SetActiveBootSlot(Slot slot) override { 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/postinstall_runner_action.cc b/postinstall_runner_action.cc
index 747ff43..184d66d 100644
--- a/postinstall_runner_action.cc
+++ b/postinstall_runner_action.cc
@@ -109,8 +109,23 @@
const string& output) {
ScopedActionCompleter completer(processor_, this);
ScopedTempUnmounter temp_unmounter(temp_rootfs_dir_);
+
+ bool success = true;
+
if (return_code != 0) {
LOG(ERROR) << "Postinst command failed with code: " << return_code;
+ success = false;
+ }
+
+ // We only attempt to mark the new slot as active if the /postinst script
+ // succeeded.
+ if (success && !system_state_->boot_control()->SetActiveBootSlot(
+ install_plan_.target_slot)) {
+ success = false;
+ }
+
+ if (!success) {
+ LOG(ERROR) << "Postinstall action failed.";
// Undo any changes done to trigger Powerwash using clobber-state.
if (powerwash_marker_created_)
diff --git a/postinstall_runner_action.h b/postinstall_runner_action.h
index 421ed01..c56218a 100644
--- a/postinstall_runner_action.h
+++ b/postinstall_runner_action.h
@@ -21,6 +21,7 @@
#include "update_engine/action.h"
#include "update_engine/install_plan.h"
+#include "update_engine/system_state.h"
// The Postinstall Runner Action is responsible for running the postinstall
// script of a successfully downloaded update.
@@ -29,9 +30,8 @@
class PostinstallRunnerAction : public InstallPlanAction {
public:
- PostinstallRunnerAction()
- : powerwash_marker_created_(false),
- powerwash_marker_file_(nullptr) {}
+ explicit PostinstallRunnerAction(SystemState* system_state)
+ : PostinstallRunnerAction(system_state, nullptr) {}
void PerformAction();
@@ -43,6 +43,14 @@
std::string Type() const { return StaticType(); }
private:
+ friend class PostinstallRunnerActionTest;
+
+ // Special constructor used for testing purposes.
+ PostinstallRunnerAction(SystemState* system_state,
+ const char* powerwash_marker_file)
+ : system_state_(system_state),
+ powerwash_marker_file_(powerwash_marker_file) {}
+
// Subprocess::Exec callback.
void CompletePostinstall(int return_code,
const std::string& output);
@@ -50,21 +58,17 @@
InstallPlan install_plan_;
std::string temp_rootfs_dir_;
+ // The main SystemState singleton.
+ SystemState* system_state_;
+
// True if Powerwash Marker was created before invoking post-install script.
// False otherwise. Used for cleaning up if post-install fails.
- bool powerwash_marker_created_;
+ bool powerwash_marker_created_ = false;
// Non-null value will cause post-install to override the default marker
// file name; used for testing.
const char* powerwash_marker_file_;
- // Special ctor + friend declaration for testing purposes.
- explicit PostinstallRunnerAction(const char* powerwash_marker_file)
- : powerwash_marker_created_(false),
- powerwash_marker_file_(powerwash_marker_file) {}
-
- friend class PostinstallRunnerActionTest;
-
DISALLOW_COPY_AND_ASSIGN(PostinstallRunnerAction);
};
diff --git a/postinstall_runner_action_unittest.cc b/postinstall_runner_action_unittest.cc
index fd03a8f..a44d8d3 100644
--- a/postinstall_runner_action_unittest.cc
+++ b/postinstall_runner_action_unittest.cc
@@ -34,6 +34,7 @@
#include <gtest/gtest.h>
#include "update_engine/constants.h"
+#include "update_engine/fake_system_state.h"
#include "update_engine/test_utils.h"
#include "update_engine/utils.h"
@@ -58,13 +59,14 @@
// powerwash_required.
void DoTest(bool do_losetup, int err_code, bool powerwash_required);
- private:
+ protected:
static const char* kImageMountPointTemplate;
base::MessageLoopForIO base_loop_;
chromeos::BaseMessageLoop loop_{&base_loop_};
chromeos::AsynchronousSignalHandler async_signal_handler_;
Subprocess subprocess_;
+ FakeSystemState fake_system_state_;
};
class PostinstActionProcessorDelegate : public ActionProcessorDelegate {
@@ -190,7 +192,8 @@
install_plan.download_url = "http://devserver:8080/update";
install_plan.powerwash_required = powerwash_required;
feeder_action.set_obj(install_plan);
- PostinstallRunnerAction runner_action(powerwash_marker_file.c_str());
+ PostinstallRunnerAction runner_action(&fake_system_state_,
+ powerwash_marker_file.c_str());
BondActions(&feeder_action, &runner_action);
ObjectCollectorAction<InstallPlan> collector_action;
BondActions(&runner_action, &collector_action);
@@ -246,7 +249,7 @@
// Death tests don't seem to be working on Hardy
TEST_F(PostinstallRunnerActionTest, DISABLED_RunAsRootDeathTest) {
ASSERT_EQ(0, getuid());
- PostinstallRunnerAction runner_action;
+ PostinstallRunnerAction runner_action(&fake_system_state_);
ASSERT_DEATH({ runner_action.TerminateProcessing(); },
"postinstall_runner_action.h:.*] Check failed");
}
diff --git a/update_attempter.cc b/update_attempter.cc
index c5320f7..81f423d 100644
--- a/update_attempter.cc
+++ b/update_attempter.cc
@@ -601,7 +601,7 @@
void UpdateAttempter::BuildPostInstallActions(
InstallPlanAction* previous_action) {
shared_ptr<PostinstallRunnerAction> postinstall_runner_action(
- new PostinstallRunnerAction());
+ new PostinstallRunnerAction(system_state_));
actions_.push_back(shared_ptr<AbstractAction>(postinstall_runner_action));
BondActions(previous_action,
postinstall_runner_action.get());
diff --git a/update_attempter_unittest.cc b/update_attempter_unittest.cc
index 10ab843..7d7b796 100644
--- a/update_attempter_unittest.cc
+++ b/update_attempter_unittest.cc
@@ -294,7 +294,7 @@
EXPECT_EQ(ErrorCode::kFilesystemVerifierError,
GetErrorCodeForAction(&filesystem_verifier_action,
ErrorCode::kError));
- PostinstallRunnerAction postinstall_runner_action;
+ PostinstallRunnerAction postinstall_runner_action(&fake_system_state);
EXPECT_EQ(ErrorCode::kPostinstallRunnerError,
GetErrorCodeForAction(&postinstall_runner_action,
ErrorCode::kError));
