Kernel: Allow acquiring the same LockRank multiple times #25932

Hendiadyoin1 · 2025-05-17T12:37:09Z

Revival of #20619

Will likely conflict with #25931

Otherwise we'd fail on acquiring the process level lockrank twice during jail propagation in `sys$fork`. Also while we are at it, make the VERIFYs neat little PANICs with good debug messages.

This delays it's destruction until the end of the function, which is necessary to avoid taking a mutex during said destruction, while under a spinlock.

This helps to enforce that we do not take Mutexes under SpinLocks. They does not have a lasting effect on the LockRank while a Mutex is held, but only during acquisition and release.

Hendiadyoin1 · 2025-06-05T22:38:20Z

New this iteration:
Random KUBSAN fixes,
will move those to a separate PR soon:tm:, as well as stop lying to userspace about it (hopefully)

kleinesfilmroellchen

Just notes, LGTM

kleinesfilmroellchen · 2025-06-09T00:32:48Z

Kernel/Locking/LockRank.h

@@ -34,6 +34,9 @@ enum class LockRank : int {
    // Process locks are the highest rank, as they normally are taken
    // first thing when processing syscalls.
    Process = 0x010,
+
+    // Mutexes need to be taken before spinlocks, so they get their own lock-rank
+    Mutex = 0x020,


Note to self: We should reorganize this bitfield such that lowest 16 bits are reserved for spinlocks (and 0x8000 is the Process lock) and highest 16 bits are reserved for mutexes. If you want you can already make this 0x1'0000 even though the circular dependency prevents any rank differentiation on Mutex right now.

Currently Mutexes don't have any lasting effect on the lock rank, as this technically is the lock rank of the internal spinlock of the mutex...

kleinesfilmroellchen · 2025-06-09T00:34:42Z

Kernel/Tasks/Thread.cpp

@@ -1401,6 +1401,10 @@ bool Thread::track_lock_acquire(LockRank rank)
    // Nothing to do for locks without a rank.
    if (rank == LockRank::None)
        return false;
+    if (rank == LockRank::Mutex && m_lock_rank_mask != LockRank::None) {


This check could be generalized if we had a notion in the bitfield about which ranks are for mutexes and which are for spinlocks :^) (not necessary for now)

stale · 2025-07-02T16:22:06Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions!

spholz · 2025-06-11T15:38:50Z

Kernel/Locking/Spinlock.h

@@ -13,6 +13,11 @@

 namespace Kernel {

+struct [[nodiscard]] SpinLockKey {


s/SpinLock/Spinlock

spholz · 2025-06-11T15:43:22Z

Kernel/Syscalls/execve.cpp

+        auto key = g_scheduler_lock.lock();
+        VERIFY(key.interrupts_state == InterruptsState::Disabled);
+        // FIXME: Remove the None check
+        VERIFY(key.affect_lock_rank || g_scheduler_lock.rank() == LockRank::None);


What is the VERIFY here supposed to check?
(same for all other checks like this one)

good question, I dont rememeber that anymore, will check that again

spholz · 2025-06-11T15:44:18Z

Kernel/Tasks/Scheduler.cpp

@@ -184,7 +184,10 @@ UNMAP_AFTER_INIT void Scheduler::start()

    // We need to acquire our scheduler lock, which will be released
    // by the idle thread once control transferred there
-    g_scheduler_lock.lock();
+    auto key = g_scheduler_lock.lock();
+    // FIXME: InterruptsEnabled check


Why can't you check if interrupts were disabled here?
(same for the other "InterruptsEnabled check" fixme in this file)

spholz · 2025-06-11T15:45:49Z

Kernel/Tasks/Scheduler.cpp

@@ -336,7 +339,7 @@ void Scheduler::leave_on_first_switch(InterruptsState previous_interrupts_state)
    // At this point, enter_current has already be called, but because
    // Scheduler::context_switch is not in the call stack we need to
    // clean up and release locks manually here
-    g_scheduler_lock.unlock(previous_interrupts_state);
+    g_scheduler_lock.unlock({ .interrupts_state = previous_interrupts_state, .affect_lock_rank = true });


Are we guaranteed to always be on a different lock rank here?

spholz · 2025-06-11T15:51:42Z

Kernel/Tasks/Thread.cpp

-        // Verify we are only attempting to take a lock of a higher rank.
-        VERIFY(m_lock_rank_mask > rank);
+        if (m_lock_rank_mask > rank)
+            PANIC("Acquiring lock of higher rank than already held, currently held: {:#05b}, to-be-acquired: {:#05b}", (int)m_lock_rank_mask, (int)rank);


nit: "Attempting to acquire a lock of higher rank than the one currently held. Currently held: {:#b}, to be acquired: {:#b}."

The maximum lock rank might change, so I wouldn't use a hard-coded minimum length for these fields. These hyphens also feel out of place.

spholz · 2025-07-03T11:55:05Z

Kernel/Locking/LockRank.h

nit: commit message: "They does" should be This does" I think.
and s/SpinLock/Spinlock

spholz · 2025-07-03T11:56:01Z

Kernel/Locking/LockRank.h

@@ -34,6 +34,9 @@ enum class LockRank : int {
    // Process locks are the highest rank, as they normally are taken
    // first thing when processing syscalls.
    Process = 0x010,
+
+    // Mutexes need to be taken before spinlocks, so they get their own lock-rank


nit: s/lock-rank/LockRank. The hyphen looks weird.
Also please end all of your comments with a period (https://github.com/SerenityOS/serenity/blob/master/Documentation/CodingStyle.md#comments).

spholz · 2025-07-03T11:58:34Z

Kernel/Tasks/Thread.cpp

@@ -1401,6 +1401,10 @@ bool Thread::track_lock_acquire(LockRank rank)
    // Nothing to do for locks without a rank.
    if (rank == LockRank::None)
        return false;
+    if (rank == LockRank::Mutex && m_lock_rank_mask != LockRank::None) {
+        // If we already hold any Spinlock, we can't acquire a mutex.
+        PANIC("Trying to acquire a mutex while holding a lock of higher rank: {:#05b}", (int)m_lock_rank_mask);


s/:#05b/:#b

spholz · 2025-07-03T11:59:43Z

Kernel/Locking/Mutex.h

+    // FIXME: This should have a lasting effect on the threads lock-rank
+    //        and therefore return some sort of key for unlocking


So do you want to give Mutexes multiple lock ranks as well?

Would probably make sense in the future

spholz · 2025-07-03T12:02:22Z

Kernel/Locking/Mutex.h

+    //  Note: The internal Spinlock might have a different Rank than the Mutex
+    //  Note: Making this a template would currently mess with Threads and our implementation
+    //        As we currently include Thread.h and Thread uses Mutexes in its API


1st note: Does this might refer to something that should be done in the future? Because currently the spinlocks can't have a dfiferent rank.
2nd note: Does this cause an include cycle or what do you mean?

IIRC Mutecies are part of the out-of-line API of Thread so slapping a template on them would make that harder

github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label May 17, 2025

Hendiadyoin1 added 3 commits June 5, 2025 22:09

Kernel: Add a few missing Tasks/WaitQueue.h includes

bfed4e8

Kernel: Allow acquiring the same LockRank multiple times

282c38e

Otherwise we'd fail on acquiring the process level lockrank twice during jail propagation in `sys$fork`. Also while we are at it, make the VERIFYs neat little PANICs with good debug messages.

Kernel: Swap out original Custody in VFSRootContext::pivot_root

8a48568

This delays it's destruction until the end of the function, which is necessary to avoid taking a mutex during said destruction, while under a spinlock.

Hendiadyoin1 force-pushed the Make-lockranks-great-again branch from 8442d89 to 080c7a7 Compare June 5, 2025 21:01

Kernel: Give Mutexes their own LockRank

1c2574b

This helps to enforce that we do not take Mutexes under SpinLocks. They does not have a lasting effect on the LockRank while a Mutex is held, but only during acquisition and release.

Hendiadyoin1 force-pushed the Make-lockranks-great-again branch from 080c7a7 to 1c2574b Compare June 5, 2025 21:21

Hendiadyoin1 marked this pull request as draft June 5, 2025 23:19

github-actions bot removed the 👀 pr-needs-review PR needs review from a maintainer or community member label Jun 5, 2025

Hendiadyoin1 force-pushed the Make-lockranks-great-again branch from a6a7a26 to adcd48b Compare June 6, 2025 18:29

Hendiadyoin1 marked this pull request as ready for review June 6, 2025 18:32

github-actions bot added the 👀 pr-needs-review PR needs review from a maintainer or community member label Jun 6, 2025

Hendiadyoin1 force-pushed the Make-lockranks-great-again branch from adcd48b to 1c2574b Compare June 6, 2025 18:32

kleinesfilmroellchen reviewed Jun 9, 2025

View reviewed changes

kleinesfilmroellchen approved these changes Jun 9, 2025

View reviewed changes

kleinesfilmroellchen added ✅ pr-community-approved PR has been approved by a community member and removed 👀 pr-needs-review PR needs review from a maintainer or community member labels Jun 9, 2025

stale bot added the stale label Jul 2, 2025

spholz reviewed Jul 3, 2025

View reviewed changes

stale bot removed stale labels Jul 3, 2025

		@@ -13,6 +13,11 @@

		namespace Kernel {

		struct [[nodiscard]] SpinLockKey {

		// FIXME: This should have a lasting effect on the threads lock-rank
		// and therefore return some sort of key for unlocking

Uh oh!

Kernel: Allow acquiring the same LockRank multiple times #25932

Are you sure you want to change the base?

Kernel: Allow acquiring the same LockRank multiple times #25932

Uh oh!

Conversation

Hendiadyoin1 commented May 17, 2025

Uh oh!

Hendiadyoin1 commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kleinesfilmroellchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Jul 2, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Hendiadyoin1 commented Jun 5, 2025 •

edited

Loading