Allow non-leaders to operate jobs

kernel/src/process/process/mod.rs

@@ -316,27 +316,6 @@ impl Process {
             .map_or(0, |session| session.sid())
     }
 
-    /// Returns the process group and the session if the process is a session leader.
-    ///
-    /// Note that once a process becomes a session leader, it remains a session leader forever.
-    /// Specifically, it can no longer be moved to a new process group or a new session. So this
-    /// method is race-free.
-    //
-    // FIXME: If we call this method on a non-current process without holding the process table
-    // lock, the session may be dead if the process is reaped at the same time.
-    fn leading_session(&self) -> Option<(Arc<ProcessGroup>, Arc<Session>)> {
-        let process_group_mut = self.process_group.lock();
-
-        let process_group = process_group_mut.upgrade().unwrap();
-        let session = process_group.session().unwrap();
-
-        if session.is_leader(self) {
-            Some((process_group, session))
-        } else {
-            None
-        }
-    }
-
     /// Returns the controlling terminal of the process, if any.
     pub fn terminal(&self) -> Option<Arc<dyn Terminal>> {
         self.process_group

kernel/src/process/process/terminal.rs

@@ -92,7 +92,13 @@ impl dyn Terminal {
 
     /// Sets the terminal to be the controlling terminal of the process.
     pub(super) fn set_control(self: Arc<Self>, process: &Process) -> Result<()> {
-        let Some((process_group, session)) = process.leading_session() else {
+        // Lock order: group of process -> session inner -> job control
+        let process_group_mut = process.process_group.lock();
+
+        let process_group = process_group_mut.upgrade().unwrap();
+        let session = process_group.session().unwrap();
+
+        if !session.is_leader(process) {
             return_errno_with_message!(
                 Errno::EPERM,
                 "the process who sets the controlling terminal is not a session leader"
@@ -119,13 +125,25 @@ impl dyn Terminal {
 
     /// Unsets the terminal from the controlling terminal of the process.
     fn unset_control(self: Arc<Self>, process: &Process) -> Result<()> {
-        self.is_control_and(process, |_, session_inner| {
+        // Lock order: group of process -> session inner -> job control
+        self.is_control_and(process, |session, session_inner| {
+            if !session.is_leader(process) {
+                // TODO: The Linux kernel keeps track of the controlling terminal of each process
+                // in `current->signal->tty`. So even if we're not the session leader, this may
+                // still succeed in releasing the controlling terminal of the current process. Note
+                // that the controlling terminal of the session will never be released in this
+                // case. We cannot mimic the exact Linux behavior, so we just return `Ok(())` here.
+                return Ok(());
+            }
+
             session_inner.set_terminal(None);
             if let Some(foreground) = self.job_control().unset_session() {
                 use crate::process::signal::{
                     constants::{SIGCONT, SIGHUP},
                     signals::kernel::KernelSignal,
                 };
+                // FIXME: Correct the lock order here. We cannot lock the group inner after locking
+                // the session inner.
                 foreground.broadcast_signal(KernelSignal::new(SIGHUP));
                 foreground.broadcast_signal(KernelSignal::new(SIGCONT));
             }
@@ -136,7 +154,7 @@ impl dyn Terminal {
 
     /// Sets the foreground process group of the terminal.
     fn set_foreground(self: Arc<Self>, pgid: Pgid, process: &Process) -> Result<()> {
-        // Take this lock before calling `is_control_and` to avoid dead locks.
+        // Lock order: group table -> group of process -> session inner -> job control
         let group_table_mut = process_table::group_table_mut();
 
         self.is_control_and(process, |session, _| {
@@ -162,19 +180,16 @@ impl dyn Terminal {
 
     /// Runs `op` when the process controls the terminal.
     ///
-    /// Note that this requires that _both_ of the following two conditions are met:
+    /// Note that this requires that the terminal is the controlling terminal of the session, but
-    /// * the process is a session leader, and
+    /// does _not_ require that the process is a session leader.
-    /// * the terminal is the controlling terminal of the session.
     fn is_control_and<F, R>(self: &Arc<Self>, process: &Process, op: F) -> Result<R>
     where
         F: FnOnce(&Arc<Session>, &mut SessionGuard) -> Result<R>,
     {
-        let Some((_, session)) = process.leading_session() else {
+        let process_group_mut = process.process_group.lock();
-            return_errno_with_message!(
+
-                Errno::ENOTTY,
+        let process_group = process_group_mut.upgrade().unwrap();
-                "the process who operates the terminal is not a session leader"
+        let session = process_group.session().unwrap();
-            );
-        };
 
         let mut session_inner = session.lock();
 

test/apps/process/job_control.c

@@ -26,7 +26,11 @@ FN_SETUP(run_in_new_session)
 	}
 
 	sid = CHECK(setsid());
+}
+END_SETUP()
 
+FN_SETUP(run_child)
+{
 	if ((child = CHECK(fork())) == 0) {
 		child = getpid();
 
@@ -144,19 +148,30 @@ FN_TEST(leader_set_unset_tty)
 }
 END_TEST()
 
-BARRIER()
+// TODO: Next we do `kill_child` and `run_child_with_tty` because the
+// Linux kernel keeps track of the controlling terminal of each process
+// in `current->signal->tty`. So without killing and restarting the
+// child, the controlling terminal won't be visible to it. This problem
+// does not exist in Asterinas, but we may want to fix this behavioral
+// difference later.
 
-FN_TEST(nonleader_unset_tty)
+FN_SETUP(kill_child)
 {
-	if (is_leader())
+	int status;
-		return;
 
-	// Only session leaders can use `TIOCNOTTY`, but we're not the leader
+	if (!is_leader())
-	TEST_ERRNO(ioctl(master, TIOCNOTTY), ENOTTY);
+		exit(__total_failures ? EXIT_FAILURE : EXIT_SUCCESS);
-	TEST_ERRNO(ioctl(slave, TIOCNOTTY), ENOTTY);
+
-	TEST_ERRNO(ioctl(STDIN_FILENO, TIOCNOTTY), ENOTTY);
+	CHECK_WITH(wait(&status),
+		   WIFEXITED(status) && WEXITSTATUS(status) == 0);
 }
-END_TEST()
+END_SETUP()
+
+FN_SETUP(run_child_with_tty)
+{
+	setup_run_child();
+}
+END_SETUP()
 
 FN_TEST(query_foreground)
 {
@@ -165,24 +180,23 @@ FN_TEST(query_foreground)
 	// All processes can use `TIOCGPGRP` on the master PTY
 	TEST_RES(ioctl(master, TIOCGPGRP, &arg), arg == sid);
 
-	// Only session leaders can use `TIOCGPGRP` on the slave PTY
+	// The slave PTY is our controlling terminal, so `TIOCGPGRP` works
-	if (is_leader())
+	TEST_RES(ioctl(slave, TIOCGPGRP, &arg), arg == sid);
-		TEST_RES(ioctl(slave, TIOCGPGRP, &arg), arg == sid);
-	else
-		TEST_ERRNO(ioctl(slave, TIOCGPGRP, &arg), ENOTTY);
 }
 END_TEST()
 
 BARRIER()
 
-FN_TEST(leader_set_foreground)
+FN_TEST(set_foreground)
 {
 	pid_t arg;
 
+	// Make sure the tests won't be run concurrently
 	if (!is_leader())
-		return;
+		TEST_SUCC(usleep(100 * 1000));
 
-	// Only session leaders can use `TIOCSPGRP`, and we're the leader
+	// All processes can use `TIOCSPGRP` on the master PTY
+	// The slave PTY is our controlling terminal, so `TIOSGPGRP` works
 
 	arg = child;
 	TEST_SUCC(ioctl(master, TIOCSPGRP, &arg));
@@ -203,33 +217,15 @@ FN_TEST(leader_set_foreground)
 	TEST_SUCC(ioctl(slave, TIOCSPGRP, &arg));
 	arg = 0xdeadbeef;
 	TEST_RES(ioctl(master, TIOCGPGRP, &arg), arg == sid);
-}
-END_TEST()
-
-BARRIER()
-
-FN_TEST(nonleader_set_foreground)
-{
-	pid_t arg = child;
 
+	// Make sure the tests won't be run concurrently
 	if (is_leader())
-		return;
+		TEST_SUCC(usleep(100 * 1000));
-
-	// Only session leaders can use `TIOCSPGRP`, but we're not the leader
-
-	TEST_ERRNO(ioctl(master, TIOCSPGRP, &arg), ENOTTY);
-	TEST_ERRNO(ioctl(slave, TIOCSPGRP, &arg), ENOTTY);
 }
 END_TEST()
 
 FN_SETUP(cleanup)
 {
-	int status;
+	setup_kill_child();
-
-	if (!is_leader())
-		return;
-
-	CHECK_WITH(wait(&status),
-		   WIFEXITED(status) && WEXITSTATUS(status) == 0);
 }
 END_SETUP()

test/syscall_test/blocklists/pty_test

@@ -24,9 +24,5 @@ PtyTest.SwitchTwiceMultiline
 JobControlTest.SetTTYBadArg
 JobControlTest.SetTTYDifferentSession
 JobControlTest.ReleaseTTYSignals
-# FIXME: `ReleaseTTYNonLeader` does pass in Linux, but is disabled because we
-# cannot mimic the exact Linux behavior due to some design differences.
-# See <https://github.com/asterinas/asterinas/pull/2066#issuecomment-2865177068>.
-JobControlTest.ReleaseTTYNonLeader
 JobControlTest.SetForegroundProcessGroup
 JobControlTest.SetForegroundProcessGroupEmptyProcessGroup