From baa6ff56497e583d706a2999134f6468f2f89b29 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Mon, 7 Mar 2022 16:34:14 +0100 Subject: [PATCH] Kernel: Wrap HIDManagement keymap data in SpinlockProtected This serializes access to the current keymap data everywhere in the kernel, allowing to mark sys$setkeymap() as not needing the big lock. --- Kernel/API/Syscall.h | 2 +- Kernel/Devices/HID/HIDManagement.cpp | 40 +++++++++++++++++----------- Kernel/Devices/HID/HIDManagement.h | 14 +++++++--- Kernel/GlobalProcessExposed.cpp | 4 ++- Kernel/Syscalls/keymap.cpp | 28 +++++++++---------- 5 files changed, 52 insertions(+), 36 deletions(-) diff --git a/Kernel/API/Syscall.h b/Kernel/API/Syscall.h index 4df48098ffc..408f7572e24 100644 --- a/Kernel/API/Syscall.h +++ b/Kernel/API/Syscall.h @@ -162,7 +162,7 @@ enum class NeedsBigProcessLock { S(setgid, NeedsBigProcessLock::Yes) \ S(setgroups, NeedsBigProcessLock::Yes) \ S(sethostname, NeedsBigProcessLock::No) \ - S(setkeymap, NeedsBigProcessLock::Yes) \ + S(setkeymap, NeedsBigProcessLock::No) \ S(setpgid, NeedsBigProcessLock::Yes) \ S(setresgid, NeedsBigProcessLock::Yes) \ S(setresuid, NeedsBigProcessLock::Yes) \ diff --git a/Kernel/Devices/HID/HIDManagement.cpp b/Kernel/Devices/HID/HIDManagement.cpp index 1d45324d0e3..084f0befc6f 100644 --- a/Kernel/Devices/HID/HIDManagement.cpp +++ b/Kernel/Devices/HID/HIDManagement.cpp @@ -86,17 +86,23 @@ size_t HIDManagement::generate_minor_device_number_for_keyboard() return m_keyboard_minor_number++; } +UNMAP_AFTER_INIT HIDManagement::KeymapData::KeymapData() + : character_map_name(KString::must_create("en-us"sv)) + , character_map(DEFAULT_CHARACTER_MAP) +{ +} + UNMAP_AFTER_INIT HIDManagement::HIDManagement() - : m_character_map_name(KString::must_create("en-us"sv)) - , m_character_map(DEFAULT_CHARACTER_MAP) { } void HIDManagement::set_maps(NonnullOwnPtr character_map_name, Keyboard::CharacterMapData const& character_map_data) { - m_character_map_name = move(character_map_name); - m_character_map = character_map_data; - dbgln("New Character map '{}' passed in by client.", m_character_map_name); + m_keymap_data.with([&](auto& keymap_data) { + keymap_data.character_map_name = move(character_map_name); + keymap_data.character_map = character_map_data; + dbgln("New Character map '{}' passed in by client.", keymap_data.character_map_name); + }); } UNMAP_AFTER_INIT ErrorOr HIDManagement::enumerate() @@ -150,17 +156,19 @@ u32 HIDManagement::get_char_from_character_map(KeyEvent event) const auto index = event.scancode & 0xFF; // Index is last byte of scan code. auto caps_lock_on = event.caps_lock_on; - u32 code_point; - if (modifiers & Mod_Alt) - code_point = m_character_map.alt_map[index]; - else if ((modifiers & Mod_Shift) && (modifiers & Mod_AltGr)) - code_point = m_character_map.shift_altgr_map[index]; - else if (modifiers & Mod_Shift) - code_point = m_character_map.shift_map[index]; - else if (modifiers & Mod_AltGr) - code_point = m_character_map.altgr_map[index]; - else - code_point = m_character_map.map[index]; + u32 code_point = 0; + m_keymap_data.with([&](auto& keymap_data) { + if (modifiers & Mod_Alt) + code_point = keymap_data.character_map.alt_map[index]; + else if ((modifiers & Mod_Shift) && (modifiers & Mod_AltGr)) + code_point = keymap_data.character_map.shift_altgr_map[index]; + else if (modifiers & Mod_Shift) + code_point = keymap_data.character_map.shift_map[index]; + else if (modifiers & Mod_AltGr) + code_point = keymap_data.character_map.altgr_map[index]; + else + code_point = keymap_data.character_map.map[index]; + }); if (caps_lock_on && (modifiers == 0 || modifiers == Mod_Shift)) { if (code_point >= 'a' && code_point <= 'z') diff --git a/Kernel/Devices/HID/HIDManagement.h b/Kernel/Devices/HID/HIDManagement.h index 5201fb98919..691fe3d476b 100644 --- a/Kernel/Devices/HID/HIDManagement.h +++ b/Kernel/Devices/HID/HIDManagement.h @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -39,8 +40,14 @@ public: ErrorOr enumerate(); - StringView keymap_name() const { return m_character_map_name->view(); } - Keyboard::CharacterMapData const& character_map() const { return m_character_map; } + struct KeymapData { + KeymapData(); + NonnullOwnPtr character_map_name; + Keyboard::CharacterMapData character_map; + }; + + SpinlockProtected& keymap_data() { return m_keymap_data; } + u32 get_char_from_character_map(KeyEvent) const; void set_client(KeyboardClient* client) { m_client = client; } @@ -50,10 +57,9 @@ private: size_t generate_minor_device_number_for_mouse(); size_t generate_minor_device_number_for_keyboard(); + SpinlockProtected m_keymap_data; size_t m_mouse_minor_number { 0 }; size_t m_keyboard_minor_number { 0 }; - NonnullOwnPtr m_character_map_name; - Keyboard::CharacterMapData m_character_map; KeyboardClient* m_client { nullptr }; RefPtr m_i8042_controller; NonnullRefPtrVector m_hid_devices; diff --git a/Kernel/GlobalProcessExposed.cpp b/Kernel/GlobalProcessExposed.cpp index a84331ebf16..c216876ff95 100644 --- a/Kernel/GlobalProcessExposed.cpp +++ b/Kernel/GlobalProcessExposed.cpp @@ -661,7 +661,9 @@ private: virtual ErrorOr try_generate(KBufferBuilder& builder) override { auto json = TRY(JsonObjectSerializer<>::try_create(builder)); - TRY(json.add("keymap", HIDManagement::the().keymap_name())); + TRY(HIDManagement::the().keymap_data().with([&](auto const& keymap_data) { + return json.add("keymap", keymap_data.character_map_name->view()); + })); TRY(json.finish()); return {}; } diff --git a/Kernel/Syscalls/keymap.cpp b/Kernel/Syscalls/keymap.cpp index 613d7891175..37c8ab7a026 100644 --- a/Kernel/Syscalls/keymap.cpp +++ b/Kernel/Syscalls/keymap.cpp @@ -13,7 +13,7 @@ constexpr size_t map_name_max_size = 50; ErrorOr Process::sys$setkeymap(Userspace user_params) { - VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this); + VERIFY_NO_PROCESS_BIG_LOCK(this); TRY(require_promise(Pledge::setkeymap)); if (!is_superuser()) @@ -37,25 +37,25 @@ ErrorOr Process::sys$setkeymap(Userspace Process::sys$getkeymap(Userspace user_params) +ErrorOr Process::sys$getkeymap(Userspace user_params) { VERIFY_NO_PROCESS_BIG_LOCK(this); TRY(require_promise(Pledge::getkeymap)); auto params = TRY(copy_typed_from_user(user_params)); - auto keymap_name = TRY(KString::try_create(HIDManagement::the().keymap_name())); - Keyboard::CharacterMapData const& character_maps = HIDManagement::the().character_map(); + return HIDManagement::the().keymap_data().with([&](auto const& keymap_data) -> ErrorOr { + if (params.map_name.size < keymap_data.character_map_name->length()) + return ENAMETOOLONG; + TRY(copy_to_user(params.map_name.data, keymap_data.character_map_name->characters(), keymap_data.character_map_name->length())); - TRY(copy_to_user(params.map, character_maps.map, CHAR_MAP_SIZE * sizeof(u32))); - TRY(copy_to_user(params.shift_map, character_maps.shift_map, CHAR_MAP_SIZE * sizeof(u32))); - TRY(copy_to_user(params.alt_map, character_maps.alt_map, CHAR_MAP_SIZE * sizeof(u32))); - TRY(copy_to_user(params.altgr_map, character_maps.altgr_map, CHAR_MAP_SIZE * sizeof(u32))); - TRY(copy_to_user(params.shift_altgr_map, character_maps.shift_altgr_map, CHAR_MAP_SIZE * sizeof(u32))); - - if (params.map_name.size < keymap_name->length()) - return ENAMETOOLONG; - TRY(copy_to_user(params.map_name.data, keymap_name->characters(), keymap_name->length())); - return 0; + auto const& character_maps = keymap_data.character_map; + TRY(copy_to_user(params.map, character_maps.map, CHAR_MAP_SIZE * sizeof(u32))); + TRY(copy_to_user(params.shift_map, character_maps.shift_map, CHAR_MAP_SIZE * sizeof(u32))); + TRY(copy_to_user(params.alt_map, character_maps.alt_map, CHAR_MAP_SIZE * sizeof(u32))); + TRY(copy_to_user(params.altgr_map, character_maps.altgr_map, CHAR_MAP_SIZE * sizeof(u32))); + TRY(copy_to_user(params.shift_altgr_map, character_maps.shift_altgr_map, CHAR_MAP_SIZE * sizeof(u32))); + return 0; + }); } }