From 25bb29362999398e17faf3ebc3004907037c7388 Mon Sep 17 00:00:00 2001 From: Liav A Date: Wed, 21 Dec 2022 22:33:56 +0200 Subject: [PATCH] Kernel: Make Device::after_inserting to return ErrorOr Instead of just returning nothing, let's return Error or nothing. This would help later on with error propagation in case of failure during this method. This also makes us more paranoid about failure in this method, so when initializing a DisplayConnector we safely tear down the internal members of the object. This applies the same for a StorageDevice object, but its after_inserting method is much smaller compared to the DisplayConnector overriden method. --- Kernel/Devices/Device.cpp | 3 +- Kernel/Devices/Device.h | 2 +- Kernel/Devices/DeviceManagement.h | 4 +-- Kernel/Graphics/DisplayConnector.cpp | 54 +++++++++++++++++++++------- Kernel/Graphics/DisplayConnector.h | 2 +- Kernel/Storage/StorageDevice.cpp | 14 +++++--- Kernel/Storage/StorageDevice.h | 2 +- Kernel/TTY/MasterPTY.cpp | 4 +-- Kernel/TTY/PTYMultiplexer.cpp | 2 +- 9 files changed, 61 insertions(+), 26 deletions(-) diff --git a/Kernel/Devices/Device.cpp b/Kernel/Devices/Device.cpp index 8e9cd4dd45a..a8dff9914ef 100644 --- a/Kernel/Devices/Device.cpp +++ b/Kernel/Devices/Device.cpp @@ -32,13 +32,14 @@ void Device::after_inserting_add_to_device_management() DeviceManagement::the().after_inserting_device({}, *this); } -void Device::after_inserting() +ErrorOr Device::after_inserting() { after_inserting_add_to_device_management(); VERIFY(!m_sysfs_component); auto sys_fs_component = SysFSDeviceComponent::must_create(*this); m_sysfs_component = sys_fs_component; after_inserting_add_to_device_identifier_directory(); + return {}; } void Device::will_be_destroyed() diff --git a/Kernel/Devices/Device.h b/Kernel/Devices/Device.h index 4a1e51c2191..a7d0369927b 100644 --- a/Kernel/Devices/Device.h +++ b/Kernel/Devices/Device.h @@ -50,7 +50,7 @@ public: virtual bool is_device() const override { return true; } virtual void will_be_destroyed() override; - virtual void after_inserting(); + virtual ErrorOr after_inserting(); virtual bool is_openable_by_jailed_processes() const { return false; } void process_next_queued_request(Badge, AsyncDeviceRequest const&); diff --git a/Kernel/Devices/DeviceManagement.h b/Kernel/Devices/DeviceManagement.h index d96a4d2a934..bd8f7a03f3e 100644 --- a/Kernel/Devices/DeviceManagement.h +++ b/Kernel/Devices/DeviceManagement.h @@ -57,7 +57,7 @@ public: requires(requires(Args... args) { DeviceType::try_create(args...); }) { auto device = TRY(DeviceType::try_create(forward(args)...)); - device->after_inserting(); + TRY(device->after_inserting()); return device; } @@ -65,7 +65,7 @@ public: static inline ErrorOr> try_create_device(Args&&... args) { auto device = TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) DeviceType(forward(args)...))); - device->after_inserting(); + TRY(device->after_inserting()); return device; } diff --git a/Kernel/Graphics/DisplayConnector.cpp b/Kernel/Graphics/DisplayConnector.cpp index b358eda0e01..1e969deda80 100644 --- a/Kernel/Graphics/DisplayConnector.cpp +++ b/Kernel/Graphics/DisplayConnector.cpp @@ -55,43 +55,73 @@ void DisplayConnector::will_be_destroyed() { GraphicsManagement::the().detach_display_connector({}, *this); - VERIFY(m_symlink_sysfs_component); - before_will_be_destroyed_remove_symlink_from_device_identifier_directory(); + // NOTE: We check if m_symlink_sysfs_component is not null, because if we failed + // at some point in DisplayConnector::after_inserting(), then that method will tear down + // the object internal members safely, so we don't want to do it again here. + if (m_symlink_sysfs_component) { + before_will_be_destroyed_remove_symlink_from_device_identifier_directory(); + m_symlink_sysfs_component.clear(); + } + + // NOTE: We check if m_sysfs_device_directory is not null, because if we failed + // at some point in DisplayConnector::after_inserting(), then that method will tear down + // the object internal members safely, so we don't want to do it again here. + if (m_sysfs_device_directory) { + SysFSDisplayConnectorsDirectory::the().unplug({}, *m_sysfs_device_directory); + m_sysfs_device_directory.clear(); + } - m_symlink_sysfs_component.clear(); - SysFSDisplayConnectorsDirectory::the().unplug({}, *m_sysfs_device_directory); before_will_be_destroyed_remove_from_device_management(); } -void DisplayConnector::after_inserting() +ErrorOr DisplayConnector::after_inserting() { after_inserting_add_to_device_management(); + ArmedScopeGuard clean_from_device_management([&] { + before_will_be_destroyed_remove_from_device_management(); + }); + auto sysfs_display_connector_device_directory = DisplayConnectorSysFSDirectory::create(SysFSDisplayConnectorsDirectory::the(), *this); m_sysfs_device_directory = sysfs_display_connector_device_directory; SysFSDisplayConnectorsDirectory::the().plug({}, *sysfs_display_connector_device_directory); + ArmedScopeGuard clean_from_sysfs_display_connector_device_directory([&] { + SysFSDisplayConnectorsDirectory::the().unplug({}, *m_sysfs_device_directory); + m_sysfs_device_directory.clear(); + }); + VERIFY(!m_symlink_sysfs_component); - auto sys_fs_component = MUST(SysFSSymbolicLinkDeviceComponent::try_create(SysFSCharacterDevicesDirectory::the(), *this, *m_sysfs_device_directory)); + auto sys_fs_component = TRY(SysFSSymbolicLinkDeviceComponent::try_create(SysFSCharacterDevicesDirectory::the(), *this, *m_sysfs_device_directory)); m_symlink_sysfs_component = sys_fs_component; after_inserting_add_symlink_to_device_identifier_directory(); + ArmedScopeGuard clean_symlink_to_device_identifier_directory([&] { + VERIFY(m_symlink_sysfs_component); + before_will_be_destroyed_remove_symlink_from_device_identifier_directory(); + m_symlink_sysfs_component.clear(); + }); - auto rounded_size = MUST(Memory::page_round_up(m_framebuffer_resource_size)); + auto rounded_size = TRY(Memory::page_round_up(m_framebuffer_resource_size)); if (!m_framebuffer_at_arbitrary_physical_range) { VERIFY(m_framebuffer_address.value().page_base() == m_framebuffer_address.value()); - m_shared_framebuffer_vmobject = MUST(Memory::SharedFramebufferVMObject::try_create_for_physical_range(m_framebuffer_address.value(), rounded_size)); - m_framebuffer_region = MUST(MM.allocate_kernel_region(m_framebuffer_address.value().page_base(), rounded_size, "Framebuffer"sv, Memory::Region::Access::ReadWrite)); + m_shared_framebuffer_vmobject = TRY(Memory::SharedFramebufferVMObject::try_create_for_physical_range(m_framebuffer_address.value(), rounded_size)); + m_framebuffer_region = TRY(MM.allocate_kernel_region(m_framebuffer_address.value().page_base(), rounded_size, "Framebuffer"sv, Memory::Region::Access::ReadWrite)); } else { - m_shared_framebuffer_vmobject = MUST(Memory::SharedFramebufferVMObject::try_create_at_arbitrary_physical_range(rounded_size)); - m_framebuffer_region = MUST(MM.allocate_kernel_region_with_vmobject(m_shared_framebuffer_vmobject->real_writes_framebuffer_vmobject(), rounded_size, "Framebuffer"sv, Memory::Region::Access::ReadWrite)); + m_shared_framebuffer_vmobject = TRY(Memory::SharedFramebufferVMObject::try_create_at_arbitrary_physical_range(rounded_size)); + m_framebuffer_region = TRY(MM.allocate_kernel_region_with_vmobject(m_shared_framebuffer_vmobject->real_writes_framebuffer_vmobject(), rounded_size, "Framebuffer"sv, Memory::Region::Access::ReadWrite)); } m_framebuffer_data = m_framebuffer_region->vaddr().as_ptr(); - m_fake_writes_framebuffer_region = MUST(MM.allocate_kernel_region_with_vmobject(m_shared_framebuffer_vmobject->fake_writes_framebuffer_vmobject(), rounded_size, "Fake Writes Framebuffer"sv, Memory::Region::Access::ReadWrite)); + m_fake_writes_framebuffer_region = TRY(MM.allocate_kernel_region_with_vmobject(m_shared_framebuffer_vmobject->fake_writes_framebuffer_vmobject(), rounded_size, "Fake Writes Framebuffer"sv, Memory::Region::Access::ReadWrite)); + + clean_from_device_management.disarm(); + clean_from_sysfs_display_connector_device_directory.disarm(); + clean_symlink_to_device_identifier_directory.disarm(); GraphicsManagement::the().attach_new_display_connector({}, *this); if (m_enable_write_combine_optimization) { [[maybe_unused]] auto result = m_framebuffer_region->set_write_combine(true); } + return {}; } bool DisplayConnector::console_mode() const diff --git a/Kernel/Graphics/DisplayConnector.h b/Kernel/Graphics/DisplayConnector.h index 157dffcbb08..8c9db638070 100644 --- a/Kernel/Graphics/DisplayConnector.h +++ b/Kernel/Graphics/DisplayConnector.h @@ -146,7 +146,7 @@ private: DisplayConnector(DisplayConnector&&) = delete; virtual void will_be_destroyed() override; - virtual void after_inserting() override; + virtual ErrorOr after_inserting() override; ErrorOr ioctl_requires_ownership(unsigned request) const; diff --git a/Kernel/Storage/StorageDevice.cpp b/Kernel/Storage/StorageDevice.cpp index 6677794d7b3..af4ea96f3fa 100644 --- a/Kernel/Storage/StorageDevice.cpp +++ b/Kernel/Storage/StorageDevice.cpp @@ -36,23 +36,27 @@ StorageDevice::StorageDevice(Badge, LUNAddress logical_unit_numbe { } -void StorageDevice::after_inserting() +ErrorOr StorageDevice::after_inserting() { after_inserting_add_to_device_management(); auto sysfs_storage_device_directory = StorageDeviceSysFSDirectory::create(SysFSStorageDirectory::the(), *this); m_sysfs_device_directory = sysfs_storage_device_directory; SysFSStorageDirectory::the().plug({}, *sysfs_storage_device_directory); VERIFY(!m_symlink_sysfs_component); - auto sys_fs_component = MUST(SysFSSymbolicLinkDeviceComponent::try_create(SysFSBlockDevicesDirectory::the(), *this, *m_sysfs_device_directory)); + auto sys_fs_component = TRY(SysFSSymbolicLinkDeviceComponent::try_create(SysFSBlockDevicesDirectory::the(), *this, *m_sysfs_device_directory)); m_symlink_sysfs_component = sys_fs_component; after_inserting_add_symlink_to_device_identifier_directory(); + return {}; } void StorageDevice::will_be_destroyed() { - VERIFY(m_symlink_sysfs_component); - before_will_be_destroyed_remove_symlink_from_device_identifier_directory(); - m_symlink_sysfs_component.clear(); + // NOTE: We check if m_symlink_sysfs_component is not null, because if we failed + // in StorageDevice::after_inserting(), then that method will not set m_symlink_sysfs_component. + if (m_symlink_sysfs_component) { + before_will_be_destroyed_remove_symlink_from_device_identifier_directory(); + m_symlink_sysfs_component.clear(); + } SysFSStorageDirectory::the().unplug({}, *m_sysfs_device_directory); before_will_be_destroyed_remove_from_device_management(); } diff --git a/Kernel/Storage/StorageDevice.h b/Kernel/Storage/StorageDevice.h index 2732c44709d..165f8904fe3 100644 --- a/Kernel/Storage/StorageDevice.h +++ b/Kernel/Storage/StorageDevice.h @@ -88,7 +88,7 @@ protected: virtual StringView class_name() const override; private: - virtual void after_inserting() override; + virtual ErrorOr after_inserting() override; virtual void will_be_destroyed() override; mutable IntrusiveListNode> m_list_node; diff --git a/Kernel/TTY/MasterPTY.cpp b/Kernel/TTY/MasterPTY.cpp index acf897c8bb5..f8b9447540c 100644 --- a/Kernel/TTY/MasterPTY.cpp +++ b/Kernel/TTY/MasterPTY.cpp @@ -22,8 +22,8 @@ ErrorOr> MasterPTY::try_create(unsigned int index) auto master_pty = TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) MasterPTY(index, move(buffer)))); auto slave_pty = TRY(adopt_nonnull_lock_ref_or_enomem(new (nothrow) SlavePTY(*master_pty, index))); master_pty->m_slave = slave_pty; - master_pty->after_inserting(); - slave_pty->after_inserting(); + TRY(master_pty->after_inserting()); + TRY(slave_pty->after_inserting()); return master_pty; } diff --git a/Kernel/TTY/PTYMultiplexer.cpp b/Kernel/TTY/PTYMultiplexer.cpp index d831926e582..118775858f2 100644 --- a/Kernel/TTY/PTYMultiplexer.cpp +++ b/Kernel/TTY/PTYMultiplexer.cpp @@ -35,7 +35,7 @@ UNMAP_AFTER_INIT PTYMultiplexer::~PTYMultiplexer() = default; UNMAP_AFTER_INIT void PTYMultiplexer::initialize() { - the().after_inserting(); + MUST(the().after_inserting()); } ErrorOr> PTYMultiplexer::open(int options)