[f2fs] Allow multiple shutdown calls and fix UAF Synchronize ComponentRunner::Shutdown to safely support multiple and racing shutdown calls (e.g. Lifecycle.Stop and Admin.Shutdown). Queue callbacks so that ManagedVfs::Shutdown runs once and all callers receive the shutdown result. Switch AdminService's shutdown requester to fit::function to tolerate repeated shutdown invocations, and retain strong fbl::RefPtr references in on_unbound handlers for AdminService and StartupService to eliminate use-after-free risks while FIDL channels remain active. Test: fx test f2fs-component-runner-test Test: fx test f2fs-component-tests Test: fx test f2fs-unittest TAG: agy CONV: 69fcc240-ebc8-4412-9773-735011240313 Change-Id: Ia5976fc2c861a6a7f4612a4f007d084e90c1a2da Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/1797177 Reviewed-by: Rui Qi Sim <rqsim@google.com> Commit-Queue: Aaron Drew <ripper@google.com>
diff --git a/src/storage/f2fs/component_runner.cc b/src/storage/f2fs/component_runner.cc index dac191b6..0b565a7 100644 --- a/src/storage/f2fs/component_runner.cc +++ b/src/storage/f2fs/component_runner.cc
@@ -148,29 +148,65 @@ void ComponentRunner::Shutdown(fs::FuchsiaVfs::ShutdownCallback cb) { TRACE_DURATION("f2fs", "ComponentRunner::Shutdown"); - if (!IsTerminating()) { - ManagedVfs::Shutdown([this, cb = std::move(cb)](zx_status_t status) mutable { - async::PostTask(dispatcher(), [this, status, cb = std::move(cb)]() mutable { - if (f2fs_) { - f2fs_->Sync([this](zx_status_t) mutable { f2fs_->PutSuper(); }); - TearDown(); - // F2fs has a BcacheMapper which has a VmoBuffer which has a Vmoid. When the VmoBuffer is - // destructed it tries to detach the Vmoid from the block device. F2fs must be destructed - // before responding to the Shutdown request to ensure that the block device still exists - // for these operations. - f2fs_.reset(); - } - if (on_unmount_) { - on_unmount_(); - } - - // Tell the unmounting channel that we've completed teardown. This *must* be the last - // thing we do because after this, the caller can assume that it's safe to destroy the - // runner. - cb(status); - }); - }); + fs::FuchsiaVfs::ShutdownCallback to_call; + zx_status_t cached_result = ZX_OK; + { + std::scoped_lock lock(shutdown_lock_); + // If the shutdown has already completed, just report it and be done. + if (is_shutdown_) { + to_call = std::move(cb); + cached_result = shutdown_result_.status_value(); + } else { + // Queue up any callbacks to be run at the end. + shutdown_callbacks_.push_back(std::move(cb)); + // Only if this is the first entry should it actually perform the shutdown. + if (shutdown_callbacks_.size() > 1) { + return; + } + } } + if (to_call) { + to_call(cached_result); + return; + } + + fs::FuchsiaVfs::ShutdownCallback final_cb = [this](zx_status_t status) { + std::vector<fs::FuchsiaVfs::ShutdownCallback> callbacks; + { + std::scoped_lock lock(shutdown_lock_); + is_shutdown_ = true; + shutdown_result_ = zx::make_result(status); + callbacks = std::move(shutdown_callbacks_); + } + for (auto& cb : callbacks) { + cb(status); + } + }; + + ManagedVfs::Shutdown([this, cb = std::move(final_cb)](zx_status_t status) mutable { + if (status != ZX_OK) { + FX_PLOGS(ERROR, status) << "Managed VFS shutdown failed"; + } + async::PostTask(dispatcher(), [this, status, cb = std::move(cb)]() mutable { + if (f2fs_) { + f2fs_->Sync([this](zx_status_t) mutable { f2fs_->PutSuper(); }); + TearDown(); + // F2fs has a BcacheMapper which has a VmoBuffer which has a Vmoid. When the VmoBuffer is + // destructed it tries to detach the Vmoid from the block device. F2fs must be destructed + // before responding to the Shutdown request to ensure that the block device still exists + // for these operations. + f2fs_.reset(); + } + if (on_unmount_) { + on_unmount_(); + } + + // Tell the unmounting channel that we've completed teardown. This *must* be the last + // thing we do because after this, the caller can assume that it's safe to destroy the + // runner. + cb(status); + }); + }); } zx::result<fs::FilesystemInfo> ComponentRunner::GetFilesystemInfo() {
diff --git a/src/storage/f2fs/component_runner.h b/src/storage/f2fs/component_runner.h index 6193c76..d1aab00b 100644 --- a/src/storage/f2fs/component_runner.h +++ b/src/storage/f2fs/component_runner.h
@@ -7,6 +7,10 @@ #include <fidl/fuchsia.process.lifecycle/cpp/wire.h> #include <lib/inspect/component/cpp/component.h> +#include <zircon/compiler.h> + +#include <mutex> +#include <vector> #include "src/storage/f2fs/common.h" #include "src/storage/lib/vfs/cpp/pseudo_dir.h" @@ -57,6 +61,12 @@ std::unique_ptr<F2fs> f2fs_; std::optional<inspect::ComponentInspector> exposed_inspector_ = {}; + + std::mutex shutdown_lock_; + bool is_shutdown_ __TA_GUARDED(shutdown_lock_) = false; + zx::result<> shutdown_result_ __TA_GUARDED(shutdown_lock_) = zx::ok(); + // A queue of callbacks for shutdown requests that arrive while shutdown is running. + std::vector<fs::FuchsiaVfs::ShutdownCallback> shutdown_callbacks_ __TA_GUARDED(shutdown_lock_); }; } // namespace f2fs
diff --git a/src/storage/f2fs/service/admin.cc b/src/storage/f2fs/service/admin.cc index 8898402..51ebc17 100644 --- a/src/storage/f2fs/service/admin.cc +++ b/src/storage/f2fs/service/admin.cc
@@ -10,7 +10,10 @@ AdminService::AdminService(async_dispatcher_t* dispatcher, ShutdownRequester shutdown) : fs::Service([dispatcher, this](fidl::ServerEnd<fuchsia_fs::Admin> server_end) { - fidl::BindServer(dispatcher, std::move(server_end), this); + fidl::BindServer( + dispatcher, std::move(server_end), this, + [self = fbl::RefPtr<AdminService>(this)](AdminService*, fidl::UnbindInfo, + fidl::ServerEnd<fuchsia_fs::Admin>) {}); return ZX_OK; }), shutdown_(std::move(shutdown)) {}
diff --git a/src/storage/f2fs/service/admin.h b/src/storage/f2fs/service/admin.h index ec6d3a4..e713df8 100644 --- a/src/storage/f2fs/service/admin.h +++ b/src/storage/f2fs/service/admin.h
@@ -16,7 +16,7 @@ class AdminService final : public fidl::WireServer<fuchsia_fs::Admin>, public fs::Service { public: - using ShutdownRequester = fit::callback<void(fs::FuchsiaVfs::ShutdownCallback)>; + using ShutdownRequester = fit::function<void(fs::FuchsiaVfs::ShutdownCallback)>; AdminService(async_dispatcher_t* dispatcher, ShutdownRequester shutdown); void Shutdown(ShutdownCompleter::Sync& completer) final;
diff --git a/src/storage/f2fs/service/startup.cc b/src/storage/f2fs/service/startup.cc index d0cffa9..13eca66 100644 --- a/src/storage/f2fs/service/startup.cc +++ b/src/storage/f2fs/service/startup.cc
@@ -14,7 +14,10 @@ StartupService::StartupService(async_dispatcher_t* dispatcher, ConfigureCallback cb) : fs::Service([dispatcher, this](fidl::ServerEnd<fuchsia_fs_startup::Startup> server_end) { - fidl::BindServer(dispatcher, std::move(server_end), this); + fidl::BindServer(dispatcher, std::move(server_end), this, + [self = fbl::RefPtr<StartupService>(this)]( + StartupService*, fidl::UnbindInfo, + fidl::ServerEnd<fuchsia_fs_startup::Startup>) {}); return ZX_OK; }), configure_(std::move(cb)) {}
diff --git a/src/storage/f2fs/test/unit/component_runner_test.cc b/src/storage/f2fs/test/unit/component_runner_test.cc index ccae84a..8e87b85 100644 --- a/src/storage/f2fs/test/unit/component_runner_test.cc +++ b/src/storage/f2fs/test/unit/component_runner_test.cc
@@ -5,10 +5,12 @@ #include "src/storage/f2fs/component_runner.h" #include <fidl/fuchsia.fs.startup/cpp/wire.h> +#include <fidl/fuchsia.fs/cpp/wire.h> #include <fidl/fuchsia.io/cpp/wire.h> #include <fidl/fuchsia.process.lifecycle/cpp/wire.h> #include <lib/async-loop/cpp/loop.h> #include <lib/async-loop/default.h> +#include <lib/async/cpp/task.h> #include <lib/component/incoming/cpp/protocol.h> #include <lib/zx/resource.h> @@ -191,5 +193,132 @@ ASSERT_TRUE(unmount_callback_called); } +TEST_F(F2fsComponentRunnerTest, DoubleShutdown) { + ASSERT_NO_FATAL_FAILURE(StartServe()); + + auto svc_dir = GetSvcDir(); + auto client_end = component::ConnectAt<fuchsia_fs_startup::Startup>(svc_dir.borrow()); + ASSERT_EQ(client_end.status_value(), ZX_OK); + + MountOptions options; + zx::result status = runner_->Configure(std::move(bcache_), options); + ASSERT_EQ(status.status_value(), ZX_OK); + ASSERT_EQ(loop_.RunUntilIdle(), ZX_OK); + + // ManagedVfs::Shutdown doesn't support being called twice. Lifecycle.Stop and Admin.Shutdown + // could race and both call ComponentRunner::Shutdown. ComponentRunner::Shutdown needs to handle + // being called twice, only calling ManagedVfs::Shutdown once and calling both callbacks with the + // result. + std::atomic<bool> callback_called = false; + async::PostTask(loop_.dispatcher(), [this, callback_called = &callback_called]() { + runner_->Shutdown([callback_called](zx_status_t status) { + EXPECT_EQ(status, ZX_OK); + *callback_called = true; + }); + }); + std::atomic<bool> callback2_called = false; + async::PostTask(loop_.dispatcher(), [this, callback_called = &callback2_called]() { + runner_->Shutdown([callback_called](zx_status_t status) { + EXPECT_EQ(status, ZX_OK); + *callback_called = true; + }); + }); + + // Shutdown quits the loop, but not before running the callbacks. + ASSERT_EQ(loop_.Run(), ZX_ERR_CANCELED); + // Both callbacks were completed. + ASSERT_TRUE(callback_called); + ASSERT_TRUE(callback2_called); +} + +TEST_F(F2fsComponentRunnerTest, AdminShutdownCanBeCalledMultipleTimes) { + ASSERT_NO_FATAL_FAILURE(StartServe()); + + auto svc_dir = GetSvcDir(); + auto startup_client_end = component::ConnectAt<fuchsia_fs_startup::Startup>(svc_dir.borrow()); + ASSERT_EQ(startup_client_end.status_value(), ZX_OK); + + MountOptions options; + zx::result status = runner_->Configure(std::move(bcache_), options); + ASSERT_EQ(status.status_value(), ZX_OK); + ASSERT_EQ(loop_.RunUntilIdle(), ZX_OK); + + auto admin_client_end1 = component::ConnectAt<fuchsia_fs::Admin>(svc_dir.borrow()); + ASSERT_EQ(admin_client_end1.status_value(), ZX_OK); + auto admin_client_end2 = component::ConnectAt<fuchsia_fs::Admin>(svc_dir.borrow()); + ASSERT_EQ(admin_client_end2.status_value(), ZX_OK); + ASSERT_EQ(loop_.RunUntilIdle(), ZX_OK); + + fidl::WireSharedClient<fuchsia_fs::Admin> admin_client1(std::move(*admin_client_end1), + loop_.dispatcher()); + fidl::WireSharedClient<fuchsia_fs::Admin> admin_client2(std::move(*admin_client_end2), + loop_.dispatcher()); + + runner_->SetUnmountCallback( + [this]() { async::PostTask(loop_.dispatcher(), [this]() { loop_.Quit(); }); }); + + std::atomic<bool> shutdown1_called = false; + admin_client1->Shutdown().ThenExactlyOnce( + [&](fidl::WireUnownedResult<fuchsia_fs::Admin::Shutdown>& result) { + EXPECT_EQ(result.status(), ZX_OK); + shutdown1_called = true; + }); + + std::atomic<bool> shutdown2_called = false; + admin_client2->Shutdown().ThenExactlyOnce( + [&](fidl::WireUnownedResult<fuchsia_fs::Admin::Shutdown>& result) { + EXPECT_EQ(result.status(), ZX_OK); + shutdown2_called = true; + }); + + ASSERT_EQ(loop_.Run(), ZX_ERR_CANCELED); + ASSERT_TRUE(shutdown1_called); + ASSERT_TRUE(shutdown2_called); +} + +TEST_F(F2fsComponentRunnerTest, AdminShutdownAndLifecycleStopRace) { + auto lifecycle_endpoints = fidl::Endpoints<fuchsia_process_lifecycle::Lifecycle>::Create(); + auto lifecycle = std::move(lifecycle_endpoints.client); + + runner_ = std::make_unique<ComponentRunner>(loop_.dispatcher()); + std::atomic<bool> unmount_callback_called = false; + runner_->SetUnmountCallback([this, &unmount_callback_called]() { + EXPECT_FALSE(unmount_callback_called); + unmount_callback_called = true; + async::PostTask(loop_.dispatcher(), [this]() { loop_.Quit(); }); + }); + zx::result status = + runner_->ServeRoot(std::move(server_end_), std::move(lifecycle_endpoints.server)); + ASSERT_EQ(status.status_value(), ZX_OK); + ASSERT_EQ(loop_.RunUntilIdle(), ZX_OK); + + MountOptions options; + status = runner_->Configure(std::move(bcache_), options); + ASSERT_EQ(status.status_value(), ZX_OK); + ASSERT_EQ(loop_.RunUntilIdle(), ZX_OK); + + auto svc_dir = GetSvcDir(); + auto admin_client_end = component::ConnectAt<fuchsia_fs::Admin>(svc_dir.borrow()); + ASSERT_EQ(admin_client_end.status_value(), ZX_OK); + ASSERT_EQ(loop_.RunUntilIdle(), ZX_OK); + + fidl::WireSharedClient<fuchsia_fs::Admin> admin_client(std::move(*admin_client_end), + loop_.dispatcher()); + + std::atomic<bool> admin_shutdown_called = false; + admin_client->Shutdown().ThenExactlyOnce( + [&](fidl::WireUnownedResult<fuchsia_fs::Admin::Shutdown>& result) { + EXPECT_EQ(result.status(), ZX_OK); + admin_shutdown_called = true; + }); + + auto lifecycle_stop_res = fidl::WireCall(lifecycle)->Stop(); + ASSERT_EQ(lifecycle_stop_res.status(), ZX_OK); + + ASSERT_EQ(loop_.Run(), ZX_ERR_CANCELED); + ASSERT_TRUE(unmount_callback_called); + ASSERT_TRUE(admin_shutdown_called); +} + } // namespace } // namespace f2fs