Kernel: Make ProcessGroup a ListedRefCounted and fix two races

This closes two race windows:

- ProcessGroup removed itself from the "all process groups" list in its
  destructor. It was possible to walk the list between the last unref()
  and the destructor invocation, and grab a pointer to a ProcessGroup
  that was about to get deleted.

- sys$setsid() could end up creating a process group that already
  existed, as there was a race window between checking if the PGID
  is used, and actually creating a ProcessGroup with that PGID.

Author: awesomekling

Kernel/ProcessGroup.cpp

@@ -1,6 +1,6 @@
 /*
  * Copyright (c) 2020, the SerenityOS developers.
- * Copyright (c) 2021, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2021-2023, Andreas Kling <kling@serenityos.org>
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
@@ -10,45 +10,44 @@
 
 namespace Kernel {
 
-static Singleton<SpinlockProtected<ProcessGroup::List, LockRank::None>> s_process_groups;
+static Singleton<SpinlockProtected<ProcessGroup::AllInstancesList, LockRank::None>> s_all_instances;
 
-SpinlockProtected<ProcessGroup::List, LockRank::None>& process_groups()
+SpinlockProtected<ProcessGroup::AllInstancesList, LockRank::None>& ProcessGroup::all_instances()
 {
-    return *s_process_groups;
+    return s_all_instances;
 }
 
-ProcessGroup::~ProcessGroup()
-{
-    process_groups().with([&](auto& groups) {
-        groups.remove(*this);
-    });
-}
+ProcessGroup::~ProcessGroup() = default;
 
-ErrorOr<NonnullRefPtr<ProcessGroup>> ProcessGroup::create(ProcessGroupID pgid)
+ErrorOr<NonnullRefPtr<ProcessGroup>> ProcessGroup::create_if_unused_pgid(ProcessGroupID pgid)
 {
-    auto process_group = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) ProcessGroup(pgid)));
-    process_groups().with([&](auto& groups) {
-        groups.prepend(*process_group);
+    return all_instances().with([&](auto& all_instances) -> ErrorOr<NonnullRefPtr<ProcessGroup>> {
+        for (auto& process_group : all_instances) {
+            if (process_group.pgid() == pgid)
+                return EPERM;
+        }
+        auto process_group = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) ProcessGroup(pgid)));
+        all_instances.prepend(*process_group);
+        return process_group;
     });
-    return process_group;
 }
 
 ErrorOr<NonnullRefPtr<ProcessGroup>> ProcessGroup::find_or_create(ProcessGroupID pgid)
 {
-    return process_groups().with([&](auto& groups) -> ErrorOr<NonnullRefPtr<ProcessGroup>> {
-        for (auto& group : groups) {
+    return all_instances().with([&](auto& all_instances) -> ErrorOr<NonnullRefPtr<ProcessGroup>> {
+        for (auto& group : all_instances) {
             if (group.pgid() == pgid)
                 return group;
         }
         auto process_group = TRY(adopt_nonnull_ref_or_enomem(new (nothrow) ProcessGroup(pgid)));
-        groups.prepend(*process_group);
+        all_instances.prepend(*process_group);
         return process_group;
     });
 }
 
 RefPtr<ProcessGroup> ProcessGroup::from_pgid(ProcessGroupID pgid)
 {
-    return process_groups().with([&](auto& groups) -> RefPtr<ProcessGroup> {
+    return all_instances().with([&](auto& groups) -> RefPtr<ProcessGroup> {
         for (auto& group : groups) {
             if (group.pgid() == pgid)
                 return &group;

Kernel/ProcessGroup.h

@@ -1,23 +1,23 @@
 /*
- * Copyright (c) 2020, the SerenityOS developers.
+ * Copyright (c) 2020-2023, the SerenityOS developers.
  *
  * SPDX-License-Identifier: BSD-2-Clause
  */
 
 #pragma once
 
-#include <AK/AtomicRefCounted.h>
 #include <AK/IntrusiveList.h>
 #include <AK/RefPtr.h>
 #include <Kernel/Forward.h>
+#include <Kernel/Library/ListedRefCounted.h>
 #include <Kernel/Library/LockWeakable.h>
 #include <Kernel/Locking/SpinlockProtected.h>
 #include <Kernel/UnixTypes.h>
 
 namespace Kernel {
 
 class ProcessGroup
-    : public AtomicRefCounted<ProcessGroup>
+    : public ListedRefCounted<ProcessGroup, LockType::Spinlock>
     , public LockWeakable<ProcessGroup> {
 
     AK_MAKE_NONMOVABLE(ProcessGroup);
@@ -26,7 +26,7 @@ class ProcessGroup
 public:
     ~ProcessGroup();
 
-    static ErrorOr<NonnullRefPtr<ProcessGroup>> create(ProcessGroupID);
+    static ErrorOr<NonnullRefPtr<ProcessGroup>> create_if_unused_pgid(ProcessGroupID);
     static ErrorOr<NonnullRefPtr<ProcessGroup>> find_or_create(ProcessGroupID);
     static RefPtr<ProcessGroup> from_pgid(ProcessGroupID);
 
@@ -38,13 +38,13 @@ class ProcessGroup
     {
     }
 
-    IntrusiveListNode<ProcessGroup> m_list_node;
     ProcessGroupID m_pgid;
 
+    mutable IntrusiveListNode<ProcessGroup> m_list_node;
+
 public:
-    using List = IntrusiveList<&ProcessGroup::m_list_node>;
+    using AllInstancesList = IntrusiveList<&ProcessGroup::m_list_node>;
+    static SpinlockProtected<AllInstancesList, LockRank::None>& all_instances();
 };
 
-SpinlockProtected<ProcessGroup::List, LockRank::None>& process_groups();
-
 }

Kernel/Syscalls/setpgid.cpp

@@ -28,16 +28,10 @@ ErrorOr<FlatPtr> Process::sys$setsid()
 {
     VERIFY_PROCESS_BIG_LOCK_ACQUIRED(this);
     TRY(require_promise(Pledge::proc));
-    bool found_process_with_same_pgid_as_my_pid = false;
-    TRY(Process::for_each_in_pgrp_in_same_jail(pid().value(), [&](auto&) -> ErrorOr<void> {
-        found_process_with_same_pgid_as_my_pid = true;
-        return {};
-    }));
-    if (found_process_with_same_pgid_as_my_pid)
-        return EPERM;
-    // Create a new Session and a new ProcessGroup.
 
-    auto process_group = TRY(ProcessGroup::create(ProcessGroupID(pid().value())));
+    // NOTE: ProcessGroup::create_if_unused_pgid() will fail with EPERM
+    //       if a process group with the same PGID already exists.
+    auto process_group = TRY(ProcessGroup::create_if_unused_pgid(ProcessGroupID(pid().value())));
     return with_mutable_protected_data([&](auto& protected_data) -> ErrorOr<FlatPtr> {
         protected_data.tty = nullptr;
         protected_data.process_group = move(process_group);