From 106d4636a4bdf48d1d031c6ac50cc6321516f5ad Mon Sep 17 00:00:00 2001 From: Space Meyer Date: Sun, 7 Apr 2024 19:12:31 +0200 Subject: [PATCH] Revert "Kernel+SystemServer: Make KCOVDevice a character device" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 9dbec601b0fdfe18ec425a6dd6518740309fd78a. For KCOV to be performant (or at least not even slower) we need to mmap the PC buffer from both user and kernel space at the same time. You can't mmap a character device, so this change didn't make sense. Plus even if we did invent a new method to exfiltrate the coverage information out of the kernel, it would be incompatible with existing kernel fuzzers. That would be kind of annoying. 🙃 --- Kernel/Devices/KCOVDevice.cpp | 2 +- Kernel/Devices/KCOVDevice.h | 5 +++-- Userland/Services/DeviceMapper/DeviceEventLoop.cpp | 5 ----- Userland/Services/SystemServer/CMakeLists.txt | 5 +++++ Userland/Services/SystemServer/main.cpp | 3 +++ Userland/Utilities/kcov-example.cpp | 2 +- 6 files changed, 13 insertions(+), 9 deletions(-) diff --git a/Kernel/Devices/KCOVDevice.cpp b/Kernel/Devices/KCOVDevice.cpp index e67fd172827..980d48dbcd0 100644 --- a/Kernel/Devices/KCOVDevice.cpp +++ b/Kernel/Devices/KCOVDevice.cpp @@ -28,7 +28,7 @@ UNMAP_AFTER_INIT NonnullLockRefPtr KCOVDevice::must_create() } UNMAP_AFTER_INIT KCOVDevice::KCOVDevice() - : CharacterDevice(30, 0) + : BlockDevice(30, 0) { proc_instance = new HashMap(); thread_instance = new HashMap(); diff --git a/Kernel/Devices/KCOVDevice.h b/Kernel/Devices/KCOVDevice.h index 8564c00b027..06849e92655 100644 --- a/Kernel/Devices/KCOVDevice.h +++ b/Kernel/Devices/KCOVDevice.h @@ -6,11 +6,11 @@ #pragma once -#include +#include #include namespace Kernel { -class KCOVDevice final : public CharacterDevice { +class KCOVDevice final : public BlockDevice { friend class DeviceManagement; public: @@ -32,6 +32,7 @@ protected: virtual bool can_read(OpenFileDescription const&, u64) const override final { return true; } virtual bool can_write(OpenFileDescription const&, u64) const override final { return true; } + virtual void start_request(AsyncBlockDeviceRequest& request) override final { request.complete(AsyncDeviceRequest::Failure); } virtual ErrorOr read(OpenFileDescription&, u64, UserOrKernelBuffer&, size_t) override { return EINVAL; } virtual ErrorOr write(OpenFileDescription&, u64, UserOrKernelBuffer const&, size_t) override { return EINVAL; } virtual ErrorOr ioctl(OpenFileDescription&, unsigned request, Userspace arg) override; diff --git a/Userland/Services/DeviceMapper/DeviceEventLoop.cpp b/Userland/Services/DeviceMapper/DeviceEventLoop.cpp index 02909b99d66..580fdfecd20 100644 --- a/Userland/Services/DeviceMapper/DeviceEventLoop.cpp +++ b/Userland/Services/DeviceMapper/DeviceEventLoop.cpp @@ -195,7 +195,6 @@ struct PluggableOnceCharacterDeviceNodeMatch { static constexpr PluggableOnceCharacterDeviceNodeMatch s_simple_matchers[] = { { "/dev/beep"sv, 0666, 1, 10 }, - { "/dev/kcov"sv, 0666, 30, 0 }, }; static ErrorOr create_pluggable_once_char_device_node(PluggableOnceCharacterDeviceNodeMatch const& match) @@ -251,10 +250,6 @@ ErrorOr DeviceEventLoop::drain_events_from_devctl() VERIFY(event.is_block_device == 1 || event.is_block_device == 0); TRY(register_new_device(event.is_block_device ? DeviceNodeFamily::Type::BlockDevice : DeviceNodeFamily::Type::CharacterDevice, event.major_number, event.minor_number)); } else if (event.state == DeviceEvent::State::Removed) { - if (event.major_number == 30 && event.minor_number == 0 && !event.is_block_device) { - dbgln("DeviceMapper: unregistering device failed: kcov tried to de-register itself!?"); - continue; - } if (auto error_or_void = unregister_device(event.is_block_device ? DeviceNodeFamily::Type::BlockDevice : DeviceNodeFamily::Type::CharacterDevice, event.major_number, event.minor_number); error_or_void.is_error()) dbgln("DeviceMapper: unregistering device failed: {}", error_or_void.error()); } else { diff --git a/Userland/Services/SystemServer/CMakeLists.txt b/Userland/Services/SystemServer/CMakeLists.txt index 583046f5705..dc871087c82 100644 --- a/Userland/Services/SystemServer/CMakeLists.txt +++ b/Userland/Services/SystemServer/CMakeLists.txt @@ -11,3 +11,8 @@ set(SOURCES serenity_bin(SystemServer) target_link_libraries(SystemServer PRIVATE LibCore LibFileSystem LibMain) + +# Required for conditionally creating hardcoded /dev/kcov entry +if (ENABLE_KERNEL_COVERAGE_COLLECTION) + add_compile_definitions(ENABLE_KERNEL_COVERAGE_COLLECTION) +endif() diff --git a/Userland/Services/SystemServer/main.cpp b/Userland/Services/SystemServer/main.cpp index 591fd5a277d..6b2852c9420 100644 --- a/Userland/Services/SystemServer/main.cpp +++ b/Userland/Services/SystemServer/main.cpp @@ -139,6 +139,9 @@ static ErrorOr prepare_bare_minimum_devtmpfs_directory_structure() TRY(Core::System::create_char_device("/dev/console"sv, 0666, 5, 1)); TRY(Core::System::create_char_device("/dev/ptmx"sv, 0666, 5, 2)); TRY(Core::System::create_char_device("/dev/tty"sv, 0666, 5, 0)); +#ifdef ENABLE_KERNEL_COVERAGE_COLLECTION + TRY(Core::System::create_block_device("/dev/kcov"sv, 0666, 30, 0)); +#endif umask(old_mask); TRY(Core::System::symlink("/dev/random"sv, "/dev/urandom"sv)); TRY(Core::System::chmod("/dev/urandom"sv, 0666)); diff --git a/Userland/Utilities/kcov-example.cpp b/Userland/Utilities/kcov-example.cpp index 990a82d83e4..d8cb90cc386 100644 --- a/Userland/Utilities/kcov-example.cpp +++ b/Userland/Utilities/kcov-example.cpp @@ -19,7 +19,7 @@ ErrorOr serenity_main(Main::Arguments) { constexpr size_t num_entries = 1024 * 100; - int fd = TRY(Core::System::open("/dev/kcov0"sv, O_RDWR)); + int fd = TRY(Core::System::open("/dev/kcov"sv, O_RDWR)); TRY(Core::System::ioctl(fd, KCOV_SETBUFSIZE, num_entries)); kcov_pc_t* cover = (kcov_pc_t*)TRY(Core::System::mmap(NULL, num_entries * KCOV_ENTRY_SIZE, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0)); TRY(Core::System::ioctl(fd, KCOV_ENABLE));