Refactor and test set{pgid,sid}

kernel/src/process/clone.rs

@@ -511,17 +511,23 @@ fn clone_sysvsem(clone_flags: CloneFlags) -> Result<()> {
 }
 
 fn set_parent_and_group(parent: &Process, child: &Arc<Process>) {
-    let process_group = parent.process_group().unwrap();
+    // Lock order: process table -> children -> group of process
-
+    // -> group inner -> session inner
     let mut process_table_mut = process_table::process_table_mut();
-    let mut group_inner = process_group.inner.lock();
-    let mut child_group_mut = child.process_group.lock();
-    let mut children_mut = parent.children().lock();
 
+    let mut children_mut = parent.children().lock();
+    let process_group_mut = parent.process_group.lock();
+
+    let process_group = process_group_mut.upgrade().unwrap();
+    let mut process_group_inner = process_group.lock();
+
+    // Put the child process in the parent's process group
+    process_group_inner.insert_process(child.clone());
+    *child.process_group.lock() = Arc::downgrade(&process_group);
+
+    // Put the child process in the parent's `children` field
     children_mut.insert(child.pid(), child.clone());
 
-    group_inner.processes.insert(child.pid(), child.clone());
+    // Put the child process in the global table
-    *child_group_mut = Arc::downgrade(&process_group);
-
     process_table_mut.insert(child.pid(), child.clone());
 }

kernel/src/process/kill.rs

@@ -54,8 +54,8 @@ pub fn kill_group(pgid: Pgid, signal: Option<UserSignal>, ctx: &Context) -> Resu
     let process_group = process_table::get_process_group(&pgid)
         .ok_or_else(|| Error::with_message(Errno::ESRCH, "target group does not exist"))?;
 
-    let inner = process_group.inner.lock();
+    let inner = process_group.lock();
-    for process in inner.processes.values() {
+    for process in inner.iter() {
         kill_process(process, signal, ctx)?;
     }
 

kernel/src/process/process/job_control.rs

@@ -161,7 +161,8 @@ impl JobControl {
             return true;
         };
 
-        foreground.contains_process(current!().pid())
+        let foreground_inner = foreground.lock();
+        foreground_inner.contains_process(&current!().pid())
     }
 }
 

kernel/src/process/process/mod.rs

@@ -243,37 +243,31 @@ impl Process {
         argv: Vec<CString>,
         envp: Vec<CString>,
     ) -> Result<Arc<Self>> {
-        let process_builder = {
+        let process = {
             let pid = allocate_posix_tid();
             let parent = Weak::new();
-
             let credentials = Credentials::new_root();
 
             let mut builder = ProcessBuilder::new(pid, executable_path, parent);
             builder.argv(argv).envp(envp).credentials(credentials);
-            builder
+            builder.build()?
         };
 
-        let process = process_builder.build()?;
-
         // Lock order: session table -> group table -> process table -> group of process
-        // -> group inner -> session inner
         let mut session_table_mut = process_table::session_table_mut();
         let mut group_table_mut = process_table::group_table_mut();
         let mut process_table_mut = process_table::process_table_mut();
 
-        // Creates new group
+        // Create a new process group and a new session for the new process
-        let group = ProcessGroup::new(process.clone());
+        process.set_new_session(
-        *process.process_group.lock() = Arc::downgrade(&group);
+            &mut process.process_group.lock(),
-        group_table_mut.insert(group.pgid(), group.clone());
+            &mut session_table_mut,
-
+            &mut group_table_mut,
-        // Creates new session
+        );
-        let session = Session::new(group.clone());
-        group.inner.lock().session = Arc::downgrade(&session);
-        session.inner.lock().leader = Some(process.clone());
-        session_table_mut.insert(session.sid(), session);
 
+        // Insert the new process to the global table
         process_table_mut.insert(process.pid(), process.clone());
+
         Ok(process)
     }
 
@@ -331,6 +325,7 @@ impl Process {
     }
 
     // *********** Parent and child ***********
+
     pub fn parent(&self) -> &ParentProcess {
         &self.parent
     }
@@ -343,254 +338,280 @@ impl Process {
         &self.children
     }
 
-    pub fn has_child(&self, pid: &Pid) -> bool {
-        self.children.lock().contains_key(pid)
-    }
-
     pub fn children_wait_queue(&self) -> &WaitQueue {
         &self.children_wait_queue
     }
 
     // *********** Process group & Session ***********
 
-    /// Returns the process group ID of the process.
+    /// Returns the process group to which the process belongs.
-    pub fn pgid(&self) -> Pgid {
-        if let Some(process_group) = self.process_group.lock().upgrade() {
-            process_group.pgid()
-        } else {
-            0
-        }
-    }
-
-    /// Returns the process group which the process belongs to.
     pub fn process_group(&self) -> Option<Arc<ProcessGroup>> {
         self.process_group.lock().upgrade()
     }
 
-    /// Returns whether `self` is the leader of process group.
+    /// Returns the process group ID of the process.
-    fn is_group_leader(self: &Arc<Self>) -> bool {
+    pub fn pgid(&self) -> Pgid {
-        let Some(process_group) = self.process_group() else {
+        self.process_group().map_or(0, |group| group.pgid())
-            return false;
-        };
-
-        let Some(leader) = process_group.leader() else {
-            return false;
-        };
-
-        Arc::ptr_eq(self, &leader)
     }
 
-    /// Returns the session which the process belongs to.
+    /// Returns the session to which the process belongs.
     pub fn session(&self) -> Option<Arc<Session>> {
-        let process_group = self.process_group()?;
+        self.process_group()?.session()
-        process_group.session()
     }
 
     /// Returns whether the process is session leader.
-    pub fn is_session_leader(self: &Arc<Self>) -> bool {
+    pub fn is_session_leader(&self) -> bool {
-        let session = self.session().unwrap();
+        self.session()
-
+            .is_some_and(|session| session.sid() == self.pid)
-        let Some(leading_process) = session.leader() else {
-            return false;
-        };
-
-        Arc::ptr_eq(self, &leading_process)
     }
 
     /// Moves the process to the new session.
     ///
-    /// If the process is already session leader, this method does nothing.
+    /// This method will create a new process group in a new session, move the process to the new
+    /// session, and return the session ID (which is equal to the process ID and the process group
+    /// ID).
     ///
-    /// Otherwise, this method creates a new process group in a new session
+    /// # Errors
-    /// and moves the process to the session, returning the new session.
     ///
-    /// This method may return the following errors:
+    /// This method will return `EPERM` if an existing process group has the same identifier as the
-    ///  * `EPERM`, if the process is a process group leader, or some existing session
+    /// process ID. This means that the process is or was a process group leader and that the
-    ///    or process group has the same ID as the process.
+    /// process group is still alive.
-    pub fn to_new_session(self: &Arc<Self>) -> Result<Arc<Session>> {
+    pub fn to_new_session(self: &Arc<Self>) -> Result<Sid> {
-        if self.is_session_leader() {
+        // Lock order: session table -> group table -> group of process
-            return Ok(self.session().unwrap());
+        // -> group inner -> session inner
-        }
+        let mut session_table_mut = process_table::session_table_mut();
+        let mut group_table_mut = process_table::group_table_mut();
 
-        if self.is_group_leader() {
+        if session_table_mut.contains_key(&self.pid) {
+            // FIXME: According to the Linux implementation, this check should be removed, so we'll
+            // return `EPERM` due to hitting the following check. However, we need to work around a
+            // gVisor bug. The upstream gVisor has fixed the issue in:
+            // <https://github.com/google/gvisor/commit/582f7bf6c0ccccaeb1215a232709df38d5d409f7>.
+            return Ok(self.pid);
+        }
+        if group_table_mut.contains_key(&self.pid) {
             return_errno_with_message!(
                 Errno::EPERM,
-                "process group leader cannot be moved to new session."
+                "a process group leader cannot be moved to a new session"
             );
         }
 
-        let session = self.session().unwrap();
+        let mut process_group_mut = self.process_group.lock();
 
-        // Lock order: session table -> group table -> group of process -> group inner -> session inner
+        self.clear_old_group_and_session(
-        let mut session_table_mut = process_table::session_table_mut();
+            &mut process_group_mut,
-        let mut group_table_mut = process_table::group_table_mut();
+            &mut session_table_mut,
-        let mut self_group_mut = self.process_group.lock();
+            &mut group_table_mut,
+        );
 
-        if session_table_mut.contains_key(&self.pid) {
+        Ok(self.set_new_session(
-            return_errno_with_message!(Errno::EPERM, "cannot create new session");
+            &mut process_group_mut,
+            &mut session_table_mut,
+            &mut group_table_mut,
+        ))
     }
 
-        if group_table_mut.contains_key(&self.pid) {
+    pub(super) fn clear_old_group_and_session(
-            return_errno_with_message!(Errno::EPERM, "cannot create process group");
+        &self,
-        }
+        process_group_mut: &mut MutexGuard<Weak<ProcessGroup>>,
+        session_table_mut: &mut MutexGuard<BTreeMap<Sid, Arc<Session>>>,
+        group_table_mut: &mut MutexGuard<BTreeMap<Pgid, Arc<ProcessGroup>>>,
+    ) {
+        let process_group = process_group_mut.upgrade().unwrap();
+        let mut process_group_inner = process_group.lock();
+        let session = process_group.session().unwrap();
+        let mut session_inner = session.lock();
 
-        // Removes the process from old group
+        // Remove the process from the process group.
-        if let Some(old_group) = self_group_mut.upgrade() {
+        process_group_inner.remove_process(&self.pid);
-            let mut group_inner = old_group.inner.lock();
+        if process_group_inner.is_empty() {
-            let mut session_inner = session.inner.lock();
+            group_table_mut.remove(&process_group.pgid());
-            group_inner.remove_process(&self.pid);
-            *self_group_mut = Weak::new();
-
-            if group_inner.is_empty() {
-                group_table_mut.remove(&old_group.pgid());
-                debug_assert!(session_inner.process_groups.contains_key(&old_group.pgid()));
-                session_inner.process_groups.remove(&old_group.pgid());
 
+            // Remove the process group from the session.
+            session_inner.remove_process_group(&process_group.pgid());
             if session_inner.is_empty() {
                 session_table_mut.remove(&session.sid());
             }
         }
+
+        **process_group_mut = Weak::new();
     }
 
-        // Creates a new process group
+    fn set_new_session(
-        let new_group = ProcessGroup::new(self.clone());
+        self: &Arc<Self>,
-        *self_group_mut = Arc::downgrade(&new_group);
+        process_group_mut: &mut MutexGuard<Weak<ProcessGroup>>,
-        group_table_mut.insert(new_group.pgid(), new_group.clone());
+        session_table_mut: &mut MutexGuard<BTreeMap<Sid, Arc<Session>>>,
+        group_table_mut: &mut MutexGuard<BTreeMap<Pgid, Arc<ProcessGroup>>>,
+    ) -> Sid {
+        let (session, process_group) = Session::new_pair(self.clone());
+        let sid = session.sid();
 
-        // Creates a new session
+        **process_group_mut = Arc::downgrade(&process_group);
-        let new_session = Session::new(new_group.clone());
-        let mut new_group_inner = new_group.inner.lock();
-        new_group_inner.session = Arc::downgrade(&new_session);
-        new_session.inner.lock().leader = Some(self.clone());
-        session_table_mut.insert(new_session.sid(), new_session.clone());
 
-        // Removes the process from session.
+        // Insert the new session and the new process group to the global table.
-        let mut session_inner = session.inner.lock();
+        session_table_mut.insert(session.sid(), session);
-        session_inner.remove_process(self);
+        group_table_mut.insert(process_group.pgid(), process_group);
 
-        Ok(new_session)
+        sid
     }
 
-    /// Moves the process to other process group.
+    /// Moves the process itself or its child process to another process group.
     ///
-    ///  * If the group already exists, the process and the group should belong to the same session.
+    /// The process to be moved is specified with the process ID `pid`; `self` is used only for
-    ///  * If the group does not exist, this method creates a new group for the process and move the
+    /// permission checking purposes (see the Errors section below), which is typically
-    ///    process to the group. The group is added to the session of the process.
+    /// `current!()` when implementing system calls.
     ///
-    /// This method may return `EPERM` in following cases:
+    /// If `pgid` is equal to the process ID, a new process group with the given PGID will be
-    ///  * The process is session leader;
+    /// created (if it does not already exist). Then, the process will be moved to the process
-    ///  * The group already exists, but the group does not belong to the same session as the process;
+    /// group with the given PGID, if the process group exists and belongs to the same session as
-    ///  * The group does not exist, but `pgid` is not equal to `pid` of the process.
+    /// the given process.
-    pub fn to_other_group(self: &Arc<Self>, pgid: Pgid) -> Result<()> {
+    ///
-        // if the process already belongs to the process group
+    /// # Errors
-        if self.pgid() == pgid {
+    ///
-            return Ok(());
+    /// This method will return `ESRCH` in following cases:
-        }
+    ///  * The process specified by `pid` does not exist;
+    ///  * The process specified by `pid` is neither `self` or a child process of `self`.
+    ///
+    /// This method will return `EPERM` in following cases:
+    ///  * The process is not in the same session as `self`;
+    ///  * The process is a session leader, but the given PGID is not the process's PID/PGID;
+    ///  * The process group already exists, but the group does not belong to the same session;
+    ///  * The process group does not exist, but `pgid` is not equal to the process ID.
+    pub fn move_process_to_group(&self, pid: Pid, pgid: Pgid) -> Result<()> {
+        // Lock order: group table -> process table -> group of process
+        // -> group inner -> session inner
+        let group_table_mut = process_table::group_table_mut();
+        let process_table_mut = process_table::process_table_mut();
 
-        if self.is_session_leader() {
+        let process = process_table_mut.get(pid).ok_or(Error::with_message(
-            return_errno_with_message!(Errno::EPERM, "the process cannot be a session leader");
+            Errno::ESRCH,
-        }
+            "the process to set the PGID does not exist",
+        ))?;
 
-        if let Some(process_group) = process_table::get_process_group(&pgid) {
+        let current_session = if self.pid == process.pid() {
-            let session = self.session().unwrap();
+            // There is no need to check if the session is the same in this case.
-            if !session.contains_process_group(&process_group) {
+            None
-                return_errno_with_message!(
+        } else if self.pid == process.parent().pid() {
-                    Errno::EPERM,
+            // FIXME: If the child process has called `execve`, we should fail with `EACCESS`.
-                    "the group and process does not belong to same session"
+
-                );
+            // Immediately release the `self.process_group` lock to avoid deadlocks. Race
-            }
+            // conditions don't matter because this is used for comparison purposes only.
-            self.to_specified_group(&process_group)?;
+            Some(
+                self.process_group
+                    .lock()
+                    .upgrade()
+                    .unwrap()
+                    .session()
+                    .unwrap(),
+            )
         } else {
-            if pgid != self.pid() {
             return_errno_with_message!(
-                    Errno::EPERM,
+                Errno::ESRCH,
-                    "the new process group should have the same ID as the process."
+                "the process to set the PGID is neither the current process nor its child process"
             );
+        };
+
+        if let Some(new_process_group) = group_table_mut.get(&pgid).cloned() {
+            process.to_existing_group(current_session, group_table_mut, new_process_group)
+        } else if pgid == process.pid() {
+            process.to_new_group(current_session, group_table_mut)
+        } else {
+            return_errno_with_message!(Errno::EPERM, "the new process group does not exist");
+        }
     }
 
-            self.to_new_group()?;
+    /// Moves the process to an existing group.
+    fn to_existing_group(
+        self: &Arc<Self>,
+        current_session: Option<Arc<Session>>,
+        mut group_table_mut: MutexGuard<BTreeMap<Pgid, Arc<ProcessGroup>>>,
+        new_process_group: Arc<ProcessGroup>,
+    ) -> Result<()> {
+        let mut process_group_mut = self.process_group.lock();
+        let process_group = process_group_mut.upgrade().unwrap();
+
+        let session = process_group.session().unwrap();
+        if session.sid() == self.pid {
+            return_errno_with_message!(
+                Errno::EPERM,
+                "a session leader cannot be moved to a new process group"
+            );
         }
+        if !Arc::ptr_eq(&session, &new_process_group.session().unwrap()) {
+            return_errno_with_message!(
+                Errno::EPERM,
+                "the new process group does not belong to the same session"
+            );
+        }
+        if current_session.is_some_and(|current| !Arc::ptr_eq(&current, &session)) {
+            return_errno_with_message!(Errno::EPERM, "the process belongs to a different session");
+        }
+
+        // Lock order: group with a smaller PGID -> group with a larger PGID
+        let (mut process_group_inner, mut new_group_inner) =
+            match process_group.pgid().cmp(&new_process_group.pgid()) {
+                core::cmp::Ordering::Less => {
+                    let process_group_inner = process_group.lock();
+                    let new_group_inner = new_process_group.lock();
+                    (process_group_inner, new_group_inner)
+                }
+                core::cmp::Ordering::Greater => {
+                    let new_group_inner = new_process_group.lock();
+                    let process_group_inner = process_group.lock();
+                    (process_group_inner, new_group_inner)
+                }
+                core::cmp::Ordering::Equal => return Ok(()),
+            };
+        let mut session_inner = session.lock();
+
+        // Remove the process from the old process group
+        process_group_inner.remove_process(&self.pid);
+        if process_group_inner.is_empty() {
+            group_table_mut.remove(&process_group.pgid());
+            session_inner.remove_process_group(&process_group.pgid());
+        }
+
+        // Insert the process to the new process group
+        new_group_inner.insert_process(self.clone());
+        *process_group_mut = Arc::downgrade(&new_process_group);
 
         Ok(())
     }
 
     /// Creates a new process group and moves the process to the group.
-    ///
+    fn to_new_group(
-    /// The new group will be added to the same session as the process.
+        self: &Arc<Self>,
-    fn to_new_group(self: &Arc<Self>) -> Result<()> {
+        current_session: Option<Arc<Session>>,
-        let session = self.session().unwrap();
+        mut group_table_mut: MutexGuard<BTreeMap<Pgid, Arc<ProcessGroup>>>,
-        // Lock order: group table -> group of process -> group inner -> session inner
+    ) -> Result<()> {
-        let mut group_table_mut = process_table::group_table_mut();
+        let mut process_group_mut = self.process_group.lock();
-        let mut self_group_mut = self.process_group.lock();
 
-        // Removes the process from old group
+        let process_group = process_group_mut.upgrade().unwrap();
-        if let Some(old_group) = self_group_mut.upgrade() {
+        let session = process_group.session().unwrap();
-            let mut group_inner = old_group.inner.lock();
-            let mut session_inner = session.inner.lock();
-            group_inner.remove_process(&self.pid);
-            *self_group_mut = Weak::new();
 
-            if group_inner.is_empty() {
+        if current_session.is_some_and(|current| !Arc::ptr_eq(&current, &session)) {
-                group_table_mut.remove(&old_group.pgid());
+            return_errno_with_message!(Errno::EPERM, "the process belongs to a different session");
-                debug_assert!(session_inner.process_groups.contains_key(&old_group.pgid()));
-                // The old session won't be empty, since we will add a new group to the session.
-                session_inner.process_groups.remove(&old_group.pgid());
         }
+        if process_group.pgid() == self.pid {
+            // We'll hit this if the process is a session leader. There is no need to check below.
+            return Ok(());
         }
 
-        // Creates a new process group. Adds the new group to group table and session.
+        let mut process_group_inner = process_group.lock();
-        let new_group = ProcessGroup::new(self.clone());
+        let mut session_inner = session.lock();
 
-        let mut new_group_inner = new_group.inner.lock();
+        // Remove the process from the old process group
-        let mut session_inner = session.inner.lock();
+        process_group_inner.remove_process(&self.pid);
-
+        if process_group_inner.is_empty() {
-        *self_group_mut = Arc::downgrade(&new_group);
+            group_table_mut.remove(&process_group.pgid());
-
+            session_inner.remove_process_group(&process_group.pgid());
-        group_table_mut.insert(new_group.pgid(), new_group.clone());
-
-        new_group_inner.session = Arc::downgrade(&session);
-        session_inner
-            .process_groups
-            .insert(new_group.pgid(), new_group.clone());
-
-        Ok(())
         }
 
-    /// Moves the process to a specified group.
+        // Create a new process group and insert the process to it
-    ///
+        let new_process_group = ProcessGroup::new(self.clone(), Arc::downgrade(&session));
-    /// The caller needs to ensure that the process and the group belongs to the same session.
+        *process_group_mut = Arc::downgrade(&new_process_group);
-    fn to_specified_group(self: &Arc<Process>, group: &Arc<ProcessGroup>) -> Result<()> {
+        group_table_mut.insert(new_process_group.pgid(), new_process_group.clone());
-        // Lock order: group table -> group of process -> group inner (small pgid -> big pgid)
+        session_inner.insert_process_group(new_process_group);
-        let mut group_table_mut = process_table::group_table_mut();
-        let mut self_group_mut = self.process_group.lock();
-
-        // Removes the process from old group
-        let mut group_inner = if let Some(old_group) = self_group_mut.upgrade() {
-            // Lock order: group with smaller pgid first
-            let (mut old_group_inner, group_inner) = match old_group.pgid().cmp(&group.pgid()) {
-                core::cmp::Ordering::Equal => return Ok(()),
-                core::cmp::Ordering::Less => (old_group.inner.lock(), group.inner.lock()),
-                core::cmp::Ordering::Greater => {
-                    let group_inner = group.inner.lock();
-                    let old_group_inner = old_group.inner.lock();
-                    (old_group_inner, group_inner)
-                }
-            };
-            old_group_inner.remove_process(&self.pid);
-            *self_group_mut = Weak::new();
-
-            if old_group_inner.is_empty() {
-                group_table_mut.remove(&old_group.pgid());
-            }
-
-            group_inner
-        } else {
-            group.inner.lock()
-        };
-
-        // Adds the process to the specified group
-        group_inner.processes.insert(self.pid, self.clone());
-        *self_group_mut = Arc::downgrade(group);
 
         Ok(())
     }
@@ -725,104 +746,3 @@ impl Process {
         }
     }
 }
-
-#[cfg(ktest)]
-mod test {
-
-    use ostd::prelude::*;
-
-    use super::*;
-
-    fn new_process(parent: Option<Arc<Process>>) -> Arc<Process> {
-        crate::util::random::init();
-        crate::fs::rootfs::init_root_mount();
-        let pid = allocate_posix_tid();
-        let parent = if let Some(parent) = parent {
-            Arc::downgrade(&parent)
-        } else {
-            Weak::new()
-        };
-        Process::new(
-            pid,
-            parent,
-            String::new(),
-            ProcessVm::alloc(),
-            ResourceLimits::default(),
-            Nice::default(),
-            Arc::new(Mutex::new(SigDispositions::default())),
-        )
-    }
-
-    fn new_process_in_session(parent: Option<Arc<Process>>) -> Arc<Process> {
-        // Lock order: session table -> group table -> group of process -> group inner
-        // -> session inner
-        let mut session_table_mut = process_table::session_table_mut();
-        let mut group_table_mut = process_table::group_table_mut();
-
-        let process = new_process(parent);
-        // Creates new group
-        let group = ProcessGroup::new(process.clone());
-        *process.process_group.lock() = Arc::downgrade(&group);
-
-        // Creates new session
-        let sess = Session::new(group.clone());
-        group.inner.lock().session = Arc::downgrade(&sess);
-        sess.inner.lock().leader = Some(process.clone());
-
-        group_table_mut.insert(group.pgid(), group);
-        session_table_mut.insert(sess.sid(), sess);
-
-        process
-    }
-
-    fn remove_session_and_group(process: Arc<Process>) {
-        // Lock order: session table -> group table
-        let mut session_table_mut = process_table::session_table_mut();
-        let mut group_table_mut = process_table::group_table_mut();
-        if let Some(sess) = process.session() {
-            session_table_mut.remove(&sess.sid());
-        }
-
-        if let Some(group) = process.process_group() {
-            group_table_mut.remove(&group.pgid());
-        }
-    }
-
-    #[ktest]
-    fn init_process() {
-        crate::time::clocks::init_for_ktest();
-        let process = new_process(None);
-        assert!(process.process_group().is_none());
-        assert!(process.session().is_none());
-    }
-
-    #[ktest]
-    fn init_process_in_session() {
-        crate::time::clocks::init_for_ktest();
-        let process = new_process_in_session(None);
-        assert!(process.is_group_leader());
-        assert!(process.is_session_leader());
-        remove_session_and_group(process);
-    }
-
-    #[ktest]
-    fn to_new_session() {
-        crate::time::clocks::init_for_ktest();
-        let process = new_process_in_session(None);
-        let sess = process.session().unwrap();
-        sess.inner.lock().leader = None;
-
-        assert!(!process.is_session_leader());
-        assert!(process
-            .to_new_session()
-            .is_err_and(|e| e.error() == Errno::EPERM));
-
-        let group = process.process_group().unwrap();
-        group.inner.lock().leader = None;
-        assert!(!process.is_group_leader());
-
-        assert!(process
-            .to_new_session()
-            .is_err_and(|e| e.error() == Errno::EPERM));
-    }
-}

kernel/src/process/process/process_group.rs

@@ -5,71 +5,53 @@ use alloc::collections::btree_map::Values;
 use super::{Pgid, Pid, Process, Session};
 use crate::{prelude::*, process::signal::signals::Signal};
 
-/// `ProcessGroup` represents a set of processes. Each `ProcessGroup` has a unique
+/// A process group.
-/// identifier `pgid`.
+///
+/// A process group represents a set of processes,
+/// which has a unique identifier PGID (i.e., [`Pgid`]).
 pub struct ProcessGroup {
     pgid: Pgid,
-    pub(in crate::process) inner: Mutex<Inner>,
+    session: Weak<Session>,
+    inner: Mutex<Inner>,
 }
 
-pub(in crate::process) struct Inner {
+struct Inner {
-    pub(in crate::process) processes: BTreeMap<Pid, Arc<Process>>,
+    processes: BTreeMap<Pid, Arc<Process>>,
-    pub(in crate::process) leader: Option<Arc<Process>>,
-    pub(in crate::process) session: Weak<Session>,
-}
-
-impl Inner {
-    pub(in crate::process) fn remove_process(&mut self, pid: &Pid) {
-        let Some(process) = self.processes.remove(pid) else {
-            return;
-        };
-
-        if let Some(leader) = &self.leader
-            && Arc::ptr_eq(leader, &process)
-        {
-            self.leader = None;
-        }
-    }
-
-    pub(in crate::process) fn is_empty(&self) -> bool {
-        self.processes.is_empty()
-    }
 }
 
 impl ProcessGroup {
-    /// Creates a new process group with one process. The pgid is the same as the process
+    /// Creates a new process group with one process.
-    /// id. The process will become the leading process of the new process group.
     ///
-    /// The caller needs to ensure that the process does not belong to any group.
+    /// The PGID is the same as the process ID, which means that the process will become the leader
-    pub(in crate::process) fn new(process: Arc<Process>) -> Arc<Self> {
+    /// process of the new process group.
+    ///
+    /// The caller needs to ensure that the process does not belong to other process group.
+    pub(super) fn new(process: Arc<Process>, session: Weak<Session>) -> Arc<Self> {
         let pid = process.pid();
 
         let inner = {
             let mut processes = BTreeMap::new();
-            processes.insert(pid, process.clone());
+            processes.insert(pid, process);
-            Inner {
+            Inner { processes }
-                processes,
-                leader: Some(process.clone()),
-                session: Weak::new(),
-            }
         };
 
         Arc::new(ProcessGroup {
             pgid: pid,
+            session,
             inner: Mutex::new(inner),
         })
     }
 
-    /// Returns whether self contains a process with `pid`.
+    /// Returns the process group identifier.
-    pub(in crate::process) fn contains_process(&self, pid: Pid) -> bool {
-        self.inner.lock().processes.contains_key(&pid)
-    }
-
-    /// Returns the process group identifier
     pub fn pgid(&self) -> Pgid {
         self.pgid
     }
 
+    /// Returns the session to which the process group belongs.
+    pub fn session(&self) -> Option<Arc<Session>> {
+        self.session.upgrade()
+    }
+
     /// Acquires a lock on the process group.
     pub fn lock(&self) -> ProcessGroupGuard {
         ProcessGroupGuard {
@@ -77,29 +59,19 @@ impl ProcessGroup {
         }
     }
 
-    /// Broadcasts signal to all processes in the group.
+    /// Broadcasts the signal to all processes in the process group.
     ///
-    /// This method should only be used to broadcast fault signal and kernel signal.
+    /// This method should only be used to broadcast fault signals and kernel signals.
-    ///
+    //
-    /// TODO: do more check to forbid user signal
+    // TODO: Do some checks to forbid user signals.
     pub fn broadcast_signal(&self, signal: impl Signal + Clone + 'static) {
         for process in self.inner.lock().processes.values() {
             process.enqueue_signal(signal.clone());
         }
     }
-
-    /// Returns the leader process.
-    pub fn leader(&self) -> Option<Arc<Process>> {
-        self.inner.lock().leader.clone()
 }
 
-    /// Returns the session which the group belongs to
+/// A scoped lock guard for a process group.
-    pub fn session(&self) -> Option<Arc<Session>> {
-        self.inner.lock().session.upgrade()
-    }
-}
-
-/// A scoped lock for a process group.
 ///
 /// It provides some public methods to prevent the exposure of the inner type.
 #[clippy::has_significant_drop]
@@ -109,12 +81,40 @@ pub struct ProcessGroupGuard<'a> {
 }
 
 impl ProcessGroupGuard<'_> {
-    /// Returns an iterator over the processes in the group.
+    /// Returns an iterator over the processes in the process group.
     pub fn iter(&self) -> ProcessGroupIter {
         ProcessGroupIter {
             inner: self.inner.processes.values(),
         }
     }
+
+    /// Returns whether the process group contains the process.
+    pub(super) fn contains_process(&self, pid: &Pid) -> bool {
+        self.inner.processes.contains_key(pid)
+    }
+
+    /// Inserts a process into the process group.
+    ///
+    /// The caller needs to ensure that the process didn't previously belong to the process group,
+    /// but now does.
+    pub(in crate::process) fn insert_process(&mut self, process: Arc<Process>) {
+        let old_process = self.inner.processes.insert(process.pid(), process);
+        debug_assert!(old_process.is_none());
+    }
+
+    /// Removes a process from the process group.
+    ///
+    /// The caller needs to ensure that the process previously belonged to the process group, but
+    /// now doesn't.
+    pub(in crate::process) fn remove_process(&mut self, pid: &Pid) {
+        let process = self.inner.processes.remove(pid);
+        debug_assert!(process.is_some());
+    }
+
+    /// Returns whether the process group is empty.
+    pub(in crate::process) fn is_empty(&self) -> bool {
+        self.inner.processes.is_empty()
+    }
 }
 
 /// An iterator over the processes of the process group.

kernel/src/process/process/session.rs

@@ -3,87 +3,72 @@
 use super::{Pgid, Process, ProcessGroup, Sid, Terminal};
 use crate::prelude::*;
 
-/// A `Session` is a collection of related process groups. Each session has a
+/// A session.
-/// unique identifier `sid`. Process groups and sessions form a two-level
-/// hierarchical relationship between processes.
 ///
-/// **Leader**: A *session leader* is the process that creates a new session and whose process
+/// A session is a collection of related process groups, which has a unique identifier SID (i.e.,
-/// ID becomes the session ID.
+/// [`Sid`]). Process groups and sessions form a two-level hierarchical relationship between
+/// processes.
 ///
-/// **Controlling terminal**: The terminal can be used to manage all processes in the session. The
+/// **Leader**: A *session leader* is the process that creates the session and whose process ID is
+/// equal to the session ID.
+///
+/// **Controlling terminal**: A terminal can be used to manage all processes in the session. The
 /// controlling terminal is established when the session leader first opens a terminal.
 pub struct Session {
     sid: Sid,
-    pub(in crate::process) inner: Mutex<Inner>,
+    inner: Mutex<Inner>,
 }
 
-pub(in crate::process) struct Inner {
+struct Inner {
-    pub(in crate::process) process_groups: BTreeMap<Pgid, Arc<ProcessGroup>>,
+    process_groups: BTreeMap<Pgid, Arc<ProcessGroup>>,
-    pub(in crate::process) leader: Option<Arc<Process>>,
+    terminal: Option<Arc<dyn Terminal>>,
-    pub(in crate::process) terminal: Option<Arc<dyn Terminal>>,
-}
-
-impl Inner {
-    pub(in crate::process) fn is_empty(&self) -> bool {
-        self.process_groups.is_empty()
-    }
-
-    pub(in crate::process) fn remove_process(&mut self, process: &Arc<Process>) {
-        if let Some(leader) = &self.leader
-            && Arc::ptr_eq(leader, process)
-        {
-            self.leader = None;
-        }
-    }
-
-    pub(in crate::process) fn remove_process_group(&mut self, pgid: &Pgid) {
-        self.process_groups.remove(pgid);
-    }
 }
 
 impl Session {
-    /// Creates a new session for the process group. The process group becomes the member of
+    /// Creates a new session and a new process group with one process.
-    /// the new session.
     ///
-    /// The caller needs to ensure that the group does not belong to any session, and the caller
+    /// The SID and the PGID are the same as the process ID, which means that the process will
-    /// should set the leader process after creating the session.
+    /// become the leader process of the new session and the new process group.
-    pub(in crate::process) fn new(group: Arc<ProcessGroup>) -> Arc<Self> {
+    ///
-        let sid = group.pgid();
+    /// The caller needs to ensure that the process does not belong to other process group or other
+    /// session.
+    pub(in crate::process) fn new_pair(process: Arc<Process>) -> (Arc<Self>, Arc<ProcessGroup>) {
+        let mut process_group = None;
+
+        let session = Arc::new_cyclic(|weak_session| {
+            let group = ProcessGroup::new(process, weak_session.clone());
+            process_group = Some(group.clone());
+
+            let pgid = group.pgid();
+
             let inner = {
                 let mut process_groups = BTreeMap::new();
-            process_groups.insert(group.pgid(), group);
+                process_groups.insert(pgid, group);
-
                 Inner {
                     process_groups,
-                leader: None,
                     terminal: None,
                 }
             };
-        Arc::new(Self {
+
-            sid,
+            Self {
+                sid: pgid,
                 inner: Mutex::new(inner),
-        })
+            }
+        });
+
+        (session, process_group.unwrap())
     }
 
-    /// Returns the session id
+    /// Returns the session identifier.
     pub fn sid(&self) -> Sid {
         self.sid
     }
 
-    /// Returns the leader process.
+    /// Acquires a lock on the session.
-    pub fn leader(&self) -> Option<Arc<Process>> {
+    pub fn lock(&self) -> SessionGuard {
-        self.inner.lock().leader.clone()
+        SessionGuard {
+            inner: self.inner.lock(),
         }
-
-    /// Returns whether `self` contains the `process_group`
-    pub(in crate::process) fn contains_process_group(
-        self: &Arc<Self>,
-        process_group: &Arc<ProcessGroup>,
-    ) -> bool {
-        self.inner
-            .lock()
-            .process_groups
-            .contains_key(&process_group.pgid())
     }
 
     /// Sets terminal as the controlling terminal of the session. The `get_terminal` method
@@ -134,3 +119,39 @@ impl Session {
         self.inner.lock().terminal.clone()
     }
 }
+
+/// A scoped lock guard for a session.
+///
+/// It provides some public methods to prevent the exposure of the inner type.
+#[clippy::has_significant_drop]
+#[must_use]
+pub struct SessionGuard<'a> {
+    inner: MutexGuard<'a, Inner>,
+}
+
+impl SessionGuard<'_> {
+    /// Inserts a process group into the session.
+    ///
+    /// The caller needs to ensure that the process group didn't previously belong to the session,
+    /// but now does.
+    pub(in crate::process) fn insert_process_group(&mut self, process_group: Arc<ProcessGroup>) {
+        let old_process_group = self
+            .inner
+            .process_groups
+            .insert(process_group.pgid(), process_group);
+        debug_assert!(old_process_group.is_none());
+    }
+
+    /// Removes a process group from the session.
+    ///
+    /// The caller needs to ensure that the process group previously belonged to the session, but
+    /// now doesn't.
+    pub(in crate::process) fn remove_process_group(&mut self, pgid: &Pgid) {
+        self.inner.process_groups.remove(pgid);
+    }
+
+    /// Returns whether the session is empty.
+    pub(in crate::process) fn is_empty(&self) -> bool {
+        self.inner.process_groups.is_empty()
+    }
+}

kernel/src/process/wait.rs

@@ -100,6 +100,7 @@ pub fn wait_child_exit(
 fn reap_zombie_child(process: &Process, pid: Pid) -> ExitCode {
     let child_process = process.children().lock().remove(&pid).unwrap();
     assert!(child_process.status().is_zombie());
+
     for task in child_process.tasks().lock().as_slice() {
         thread_table::remove_thread(task.as_posix_thread().unwrap().tid());
     }
@@ -108,28 +109,19 @@ fn reap_zombie_child(process: &Process, pid: Pid) -> ExitCode {
     // -> group inner -> session inner
     let mut session_table_mut = process_table::session_table_mut();
     let mut group_table_mut = process_table::group_table_mut();
+
+    // Remove the process from the global table
     let mut process_table_mut = process_table::process_table_mut();
+    process_table_mut.remove(child_process.pid());
 
+    // Remove the process group and the session from global table, if necessary
     let mut child_group_mut = child_process.process_group.lock();
-
+    child_process.clear_old_group_and_session(
-    let process_group = child_group_mut.upgrade().unwrap();
+        &mut child_group_mut,
-    let mut group_inner = process_group.inner.lock();
+        &mut session_table_mut,
-    let session = group_inner.session.upgrade().unwrap();
+        &mut group_table_mut,
-    let mut session_inner = session.inner.lock();
+    );
-
-    group_inner.remove_process(&child_process.pid());
-    session_inner.remove_process(&child_process);
     *child_group_mut = Weak::new();
 
-    if group_inner.is_empty() {
-        group_table_mut.remove(&process_group.pgid());
-        session_inner.remove_process_group(&process_group.pgid());
-
-        if session_inner.is_empty() {
-            session_table_mut.remove(&session.sid());
-        }
-    }
-
-    process_table_mut.remove(child_process.pid());
     child_process.status().exit_code()
 }

kernel/src/syscall/setpgid.rs

@@ -3,35 +3,24 @@
 use super::SyscallReturn;
 use crate::{
     prelude::*,
-    process::{process_table, Pgid, Pid},
+    process::{Pgid, Pid},
 };
 
 pub fn sys_setpgid(pid: Pid, pgid: Pgid, ctx: &Context) -> Result<SyscallReturn> {
     let current = ctx.process;
-    // if pid is 0, pid should be the pid of current process
+
+    // The documentation quoted below is from
+    // <https://www.man7.org/linux/man-pages/man2/setpgid.2.html>.
+
+    // "If `pid` is zero, then the process ID of the calling process is used."
     let pid = if pid == 0 { current.pid() } else { pid };
-    // if pgid is 0, pgid should be pid
+    // "If `pgid` is zero, then the PGID of the process specified by `pid` is made the same as its
+    // process ID."
     let pgid = if pgid == 0 { pid } else { pgid };
+
     debug!("pid = {}, pgid = {}", pid, pgid);
 
-    if pid != current.pid() && !current.has_child(&pid) {
+    current.move_process_to_group(pid, pgid)?;
-        return_errno_with_message!(
-            Errno::ESRCH,
-            "cannot set pgid for process other than current or children of current"
-        );
-    }
-    // FIXME: If pid is child process of current and already calls execve, should return error.
-    // How can we determine a child process has called execve?
-
-    // only can move process to an existing group or self
-    if pgid != pid && !process_table::contain_process_group(&pgid) {
-        return_errno_with_message!(Errno::EPERM, "process group must exist");
-    }
-
-    let process = process_table::get_process(pid)
-        .ok_or(Error::with_message(Errno::ESRCH, "process does not exist"))?;
-
-    process.to_other_group(pgid)?;
 
     Ok(SyscallReturn::Return(0))
 }

kernel/src/syscall/setsid.rs

@@ -4,8 +4,7 @@ use super::SyscallReturn;
 use crate::prelude::*;
 
 pub fn sys_setsid(_ctx: &Context) -> Result<SyscallReturn> {
-    let current = current!();
+    let sid = current!().to_new_session()?;
-    let session = current.to_new_session()?;
 
-    Ok(SyscallReturn::Return(session.sid() as _))
+    Ok(SyscallReturn::Return(sid as _))
 }

test/apps/Makefile

@@ -34,6 +34,7 @@ TEST_APPS := \
 	network \
 	pipe \
 	prctl \
+	process \
 	pthread \
 	pty \
 	sched \

test/apps/process/Makefile

@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: MPL-2.0
+
+include ../test_common.mk
+
+EXTRA_C_FLAGS :=

test/apps/process/group_session.c

@@ -0,0 +1,138 @@
+// SPDX-License-Identifier: MPL-2.0
+
+#include "../network/test.h"
+
+#include <unistd.h>
+#include <sys/wait.h>
+
+static pid_t current;
+static pid_t child1, child2;
+
+FN_SETUP(setpgrp)
+{
+	CHECK(setpgid(0, 0));
+}
+END_SETUP()
+
+FN_SETUP(spawn_child)
+{
+	current = CHECK(getpid());
+
+	if ((child1 = CHECK(fork())) == 0) {
+		sleep(60);
+		exit(EXIT_FAILURE);
+	}
+
+	if ((child2 = CHECK(fork())) == 0) {
+		sleep(60);
+		exit(EXIT_FAILURE);
+	}
+}
+END_SETUP()
+
+FN_TEST(setpgid_invalid)
+{
+	// Non-present process groups
+	TEST_ERRNO(setpgid(child1, child2), EPERM);
+	TEST_ERRNO(setpgid(child2, child1), EPERM);
+	TEST_ERRNO(setpgid(child1, 0x3c3c3c3c), EPERM);
+	TEST_ERRNO(setpgid(child2, 0x3c3c3c3c), EPERM);
+
+	// Non-present processes
+	TEST_ERRNO(setpgid(0x3c3c3c3c, current), ESRCH);
+	TEST_ERRNO(setpgid(0x3c3c3c3c, current), ESRCH);
+
+	// Non-current and non-child processes
+	TEST_ERRNO(setpgid(getppid(), 0), ESRCH);
+	TEST_ERRNO(setpgid(getppid(), 0x3c3c3c3c), ESRCH);
+}
+END_TEST()
+
+FN_TEST(setpgid)
+{
+	//                   PGID       members
+	//                     |           |
+	//                     v           v
+	// Process groups: [current] = { current, child1, child2 }
+
+	TEST_SUCC(setpgid(0, 0));
+	TEST_SUCC(setpgid(0, current));
+	TEST_SUCC(setpgid(current, 0));
+	TEST_SUCC(setpgid(current, getpid()));
+
+	TEST_ERRNO(setpgid(child1, child2), EPERM);
+	TEST_ERRNO(setpgid(child2, child1), EPERM);
+
+	// Process groups: [current] = { current, child2 }, [child1] = { child1 }
+	TEST_SUCC(setpgid(child1, 0));
+
+	// Process groups: [current] = { current }, [child1] = { child1, child2 }
+	TEST_SUCC(setpgid(child2, child1));
+
+	// Process groups: [current] = { current, child1 }, [child1] = { child2 }
+	TEST_SUCC(setpgid(child1, current));
+
+	// Process groups: [current] = { current }, [child1] = { child1, child2 }
+	TEST_SUCC(setpgid(child1, child1));
+}
+END_TEST()
+
+FN_TEST(setsid_group_leader)
+{
+	// Process groups: [current] = { current }, [child1] = { child1, child2 }
+
+	TEST_ERRNO(setsid(), EPERM);
+
+	// Process groups: [current] = { child1 }, [child1] = { current, child2 }
+	TEST_SUCC(setpgid(child1, current));
+	TEST_SUCC(setpgid(current, child1));
+
+	TEST_ERRNO(setsid(), EPERM);
+}
+END_TEST()
+
+FN_TEST(setsid)
+{
+	// Process groups: [child1] = { current, child1, child2 }
+	TEST_SUCC(setpgid(child1, child1));
+
+	// Process groups (old session): [child1] = { child1, child2 }
+	// Process groups (new session): [current] = { current }
+	TEST_SUCC(setsid());
+}
+END_TEST()
+
+// From now on, the current process and the child processes are in two sessions!
+
+FN_TEST(setsid_session_leader)
+{
+	// FIXME: We fail this test to work around a gVisor bug.
+	// See comments in `Process::to_new_session` for details.
+	//
+	// TEST_ERRNO(setsid(), EPERM);
+}
+END_TEST()
+
+FN_TEST(setpgid_two_sessions)
+{
+	// Setting process groups in another session should never succeed
+
+	TEST_ERRNO(setpgid(child1, child1), EPERM);
+	TEST_ERRNO(setpgid(child2, child2), EPERM);
+
+	TEST_ERRNO(setpgid(child1, current), EPERM);
+	TEST_ERRNO(setpgid(child2, current), EPERM);
+
+	TEST_ERRNO(setpgid(child2, child1), EPERM);
+}
+END_TEST()
+
+FN_SETUP(kill_child)
+{
+	CHECK(kill(child1, SIGKILL));
+	CHECK_WITH(wait(NULL), _ret == child1);
+
+	CHECK(kill(child2, SIGKILL));
+	CHECK_WITH(wait(NULL), _ret == child2);
+}
+END_SETUP()

test/apps/scripts/process.sh

@@ -29,6 +29,7 @@ itimer/timer_create
 mmap/mmap_and_fork
 mmap/mmap_shared_filebacked
 mmap/mmap_readahead
+process/group_session
 pthread/pthread_test
 pty/open_pty
 sched/sched_attr