LadybirdBrowser/ladybird
1
1
Fork 0
mirror of https://github.com/LadybirdBrowser/ladybird.git synced 2024-10-26 14:57:54 +03:00
Code Issues Packages Projects Releases Wiki Activity

Kernel/aarch64: Make sure no reordering of DAIF::read is possible

Browse Source 
We were crashing on the VERIFY_INTERRUPTS_DISABLED() in
RecursiveSpinlock::unlock, which was caused by the compiler reordering
instructions in `sys$get_root_session_id`. In this function, a SpinLock
is locked and quickly unlocked again, and since the lock and unlock
functions were inlined into `sys$get_root_session_id` and the DAIF::read
was missing the `volatile` keyword, the compiler was free to reorder the
reads from the DAIF register to the top of this function. This caused
the CPU to read the interrupts state at the beginning of the function,
and storing the result on the stack, which in turn caused the
VERIFY_INTERRUPTS_DISABLED() assertion to fail. By adding the `volatile`
modifier to the inline assembly, the compiler will not reorder the
instructions.

In aa40cef2b7, I mistakenly assumed that the crash was related to the
initial interrupts state of the kernel threads, but it turns out that
the missing `volatile` keyword was the actual problem. This commit also
removes that code again.
This commit is contained in:
Timon Kruiper 2023-04-06 12:01:18 +02:00 committed by Linus Groh
parent 7795b7bc17
commit 10030038e9
Notes: sideshowbarker 2024-07-17 04:10:16 +09:00 
Author: https://github.com/FireFox317
Commit: https://github.com/SerenityOS/serenity/commit/10030038e9
Pull-request: https://github.com/SerenityOS/serenity/pull/18291
Reviewed-by: https://github.com/nico ✅
2 changed files with 2 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
Kernel/Arch/aarch64/Registers.h
View File

@ -1287,8 +1287,8 @@ struct DAIF {
{
DAIF daif;
asm("mrs %[value], daif"
: [value] "=r"(daif));
asm volatile("mrs %[value], daif"
: [value] "=r"(daif));
return daif;
}

7
Kernel/WorkQueue.cpp
View File

@ -28,13 +28,6 @@ UNMAP_AFTER_INIT WorkQueue::WorkQueue(StringView name)
if (name_kstring.is_error())
TODO();
auto [_, thread] = Process::create_kernel_process(name_kstring.release_value(), [this] {
#if ARCH(AARCH64)
// FIXME: This function expects to be executed with interrupts disabled, however on
// aarch64 we spawn (kernel) threads with interrupts enabled, so we need to disable them.
// This code should be written in a way that it is able to be executed with interrupts enabled.
Processor::disable_interrupts();
#endif
for (;;) {
WorkItem* item;
bool have_more;