From 7e8dab775bb800d3a1808ef5d0fb6cfd8ff21905 Mon Sep 17 00:00:00 2001 From: Thurston Dang Date: Thu, 7 Oct 2021 23:08:56 +0000 Subject: [PATCH] storageproxyd: discard writes when checkpointing, if necessary If a checkpointing operation is in progress, discard any write operations that are flagged as STORAGE_MSG_FLAG_PRE_COMMIT_CHECKPOINT. In tandem with trusty-side changes that set the flag appropriately, this avoids the awkward case where the checkpoint is rolled back, which potentially leads to inconsistency between the data and the superblock. Based on Stephen's CL/1845477 "Add helper to check checkpoint state of mounts". Test: m storageproxyd Bug: 194313068 Change-Id: Ib6a432db1bc1b034f803b743b0d7322e3f31d814 --- .../include/trusty/interface/storage.h | 40 +++++----- trusty/storage/proxy/Android.bp | 3 + trusty/storage/proxy/checkpoint_handling.cpp | 77 +++++++++++++++++++ trusty/storage/proxy/checkpoint_handling.h | 37 +++++++++ trusty/storage/proxy/proxy.c | 16 ++++ 5 files changed, 155 insertions(+), 18 deletions(-) create mode 100644 trusty/storage/proxy/checkpoint_handling.cpp create mode 100644 trusty/storage/proxy/checkpoint_handling.h diff --git a/trusty/storage/interface/include/trusty/interface/storage.h b/trusty/storage/interface/include/trusty/interface/storage.h index b196d88b3..3f1dcb8c6 100644 --- a/trusty/storage/interface/include/trusty/interface/storage.h +++ b/trusty/storage/interface/include/trusty/interface/storage.h @@ -112,26 +112,30 @@ enum storage_file_open_flag { /** * enum storage_msg_flag - protocol-level flags in struct storage_msg - * @STORAGE_MSG_FLAG_BATCH: if set, command belongs to a batch transaction. - * No response will be sent by the server until - * it receives a command with this flag unset, at - * which point a cummulative result for all messages - * sent with STORAGE_MSG_FLAG_BATCH will be sent. - * This is only supported by the non-secure disk proxy - * server. - * @STORAGE_MSG_FLAG_PRE_COMMIT: if set, indicates that server need to commit - * pending changes before processing this message. - * @STORAGE_MSG_FLAG_POST_COMMIT: if set, indicates that server need to commit - * pending changes after processing this message. - * @STORAGE_MSG_FLAG_TRANSACT_COMPLETE: if set, indicates that server need to commit - * current transaction after processing this message. - * It is an alias for STORAGE_MSG_FLAG_POST_COMMIT. + * @STORAGE_MSG_FLAG_BATCH: if set, command belongs to a batch transaction. + * No response will be sent by the server until + * it receives a command with this flag unset, at + * which point a cumulative result for all messages + * sent with STORAGE_MSG_FLAG_BATCH will be sent. + * This is only supported by the non-secure disk proxy + * server. + * @STORAGE_MSG_FLAG_PRE_COMMIT: if set, indicates that server need to commit + * pending changes before processing this message. + * @STORAGE_MSG_FLAG_POST_COMMIT: if set, indicates that server need to commit + * pending changes after processing this message. + * @STORAGE_MSG_FLAG_TRANSACT_COMPLETE: if set, indicates that server need to commit + * current transaction after processing this message. + * It is an alias for STORAGE_MSG_FLAG_POST_COMMIT. + * @STORAGE_MSG_FLAG_PRE_COMMIT_CHECKPOINT: if set, indicates that server needs to ensure + * that there is not a pending checkpoint for + * userdata before processing this message. */ enum storage_msg_flag { - STORAGE_MSG_FLAG_BATCH = 0x1, - STORAGE_MSG_FLAG_PRE_COMMIT = 0x2, - STORAGE_MSG_FLAG_POST_COMMIT = 0x4, - STORAGE_MSG_FLAG_TRANSACT_COMPLETE = STORAGE_MSG_FLAG_POST_COMMIT, + STORAGE_MSG_FLAG_BATCH = 0x1, + STORAGE_MSG_FLAG_PRE_COMMIT = 0x2, + STORAGE_MSG_FLAG_POST_COMMIT = 0x4, + STORAGE_MSG_FLAG_TRANSACT_COMPLETE = STORAGE_MSG_FLAG_POST_COMMIT, + STORAGE_MSG_FLAG_PRE_COMMIT_CHECKPOINT = 0x8, }; /* diff --git a/trusty/storage/proxy/Android.bp b/trusty/storage/proxy/Android.bp index d67089fb2..38d868508 100644 --- a/trusty/storage/proxy/Android.bp +++ b/trusty/storage/proxy/Android.bp @@ -23,6 +23,7 @@ cc_binary { vendor: true, srcs: [ + "checkpoint_handling.cpp", "ipc.c", "rpmb.c", "storage.c", @@ -30,12 +31,14 @@ cc_binary { ], shared_libs: [ + "libbase", "liblog", "libhardware_legacy", ], header_libs: ["libcutils_headers"], static_libs: [ + "libfstab", "libtrustystorageinterface", "libtrusty", ], diff --git a/trusty/storage/proxy/checkpoint_handling.cpp b/trusty/storage/proxy/checkpoint_handling.cpp new file mode 100644 index 000000000..6c2fd363e --- /dev/null +++ b/trusty/storage/proxy/checkpoint_handling.cpp @@ -0,0 +1,77 @@ +/* + * Copyright (C) 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "checkpoint_handling.h" +#include "log.h" + +#include +#include +#include + +namespace { + +bool checkpointingDoneForever = false; + +} // namespace + +int is_data_checkpoint_active(bool* active) { + if (!active) { + ALOGE("active out parameter is null"); + return 0; + } + + *active = false; + + if (checkpointingDoneForever) { + return 0; + } + + android::fs_mgr::Fstab procMounts; + bool success = android::fs_mgr::ReadFstabFromFile("/proc/mounts", &procMounts); + if (!success) { + ALOGE("Could not parse /proc/mounts\n"); + /* Really bad. Tell the caller to abort the write. */ + return -1; + } + + android::fs_mgr::FstabEntry* dataEntry = + android::fs_mgr::GetEntryForMountPoint(&procMounts, "/data"); + if (dataEntry == NULL) { + ALOGE("/data is not mounted yet\n"); + return 0; + } + + /* We can't handle e.g., ext4. Nothing we can do about it for now. */ + if (dataEntry->fs_type != "f2fs") { + ALOGW("Checkpoint status not supported for filesystem %s\n", dataEntry->fs_type.c_str()); + checkpointingDoneForever = true; + return 0; + } + + /* + * The data entry looks like "... blah,checkpoint=disable:0,blah ...". + * checkpoint=disable means checkpointing is on (yes, arguably reversed). + */ + size_t checkpointPos = dataEntry->fs_options.find("checkpoint=disable"); + if (checkpointPos == std::string::npos) { + /* Assumption is that once checkpointing turns off, it stays off */ + checkpointingDoneForever = true; + } else { + *active = true; + } + + return 0; +} diff --git a/trusty/storage/proxy/checkpoint_handling.h b/trusty/storage/proxy/checkpoint_handling.h new file mode 100644 index 000000000..f1bf27c8d --- /dev/null +++ b/trusty/storage/proxy/checkpoint_handling.h @@ -0,0 +1,37 @@ +/* + * Copyright (C) 2021 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#pragma once + +#include + +#ifdef __cplusplus +extern "C" { +#endif + +/** + * is_data_checkpoint_active() - Check for an active, uncommitted checkpoint of + * /data. If a checkpoint is active, storage should not commit any + * rollback-protected writes to /data. + * @active: Out parameter that will be set to the result of the check. + * + * Return: 0 if active was set and is valid, non-zero otherwise. + */ +int is_data_checkpoint_active(bool* active); + +#ifdef __cplusplus +} +#endif diff --git a/trusty/storage/proxy/proxy.c b/trusty/storage/proxy/proxy.c index e23094183..c690a2876 100644 --- a/trusty/storage/proxy/proxy.c +++ b/trusty/storage/proxy/proxy.c @@ -26,6 +26,7 @@ #include +#include "checkpoint_handling.h" #include "ipc.h" #include "log.h" #include "rpmb.h" @@ -130,6 +131,21 @@ static int handle_req(struct storage_msg* msg, const void* req, size_t req_len) } } + if (msg->flags & STORAGE_MSG_FLAG_PRE_COMMIT_CHECKPOINT) { + bool is_checkpoint_active = false; + + rc = is_data_checkpoint_active(&is_checkpoint_active); + if (rc != 0) { + ALOGE("is_data_checkpoint_active failed in an unexpected way. Aborting.\n"); + msg->result = STORAGE_ERR_GENERIC; + return ipc_respond(msg, NULL, 0); + } else if (is_checkpoint_active) { + ALOGE("Checkpoint in progress, dropping write ...\n"); + msg->result = STORAGE_ERR_GENERIC; + return ipc_respond(msg, NULL, 0); + } + } + switch (msg->cmd) { case STORAGE_FILE_DELETE: rc = storage_file_delete(msg, req, req_len);