Clean up devpts code, and deduplicate the foreground process state.

We no longer store the foreground process directly in the terminal. Instead, we
get it from the terminal TTY's ThreadGroup. Added a new method:
tty.SignalForegroundProcessGroup to simplify this.

Cleaned up some things along the way:
* Terminal had a bunch of methods to get/set foreground process group and
  controlling TTY, but those methods were only usable by Ioctl, since they
  read/wrote to syscall arguments. I moved that logic to Ioctl, and deleted the
  methods from Terminal, which is now a very simple type.
* Fixed a bug in ThreadGroud.SetForegroundProcessGroup where we were
  overwriting the ID of an existing process group, rather than setting a new
  process group on the session.
* Simplified the construction of lineDiscipline type.

Reported-by: syzbot+ae5b769cec8ad969c086@syzkaller.appspotmail.com
PiperOrigin-RevId: 512330758

pkg/sentry/fsimpl/devpts/devpts_test.go

@@ -24,7 +24,7 @@ import (
 )
 
 func TestSimpleMasterToReplica(t *testing.T) {
-	ld := newLineDiscipline(linux.DefaultReplicaTermios)
+	ld := newLineDiscipline(linux.DefaultReplicaTermios, nil)
 	ctx := contexttest.Context(t)
 	inBytes := []byte("hello, tty\n")
 	src := usermem.BytesIOSequence(inBytes)
@@ -60,7 +60,7 @@ func TestEchoDeadlock(t *testing.T) {
 	ctx := contexttest.Context(t)
 	termios := linux.DefaultReplicaTermios
 	termios.LocalFlags |= linux.ECHO
-	ld := newLineDiscipline(termios)
+	ld := newLineDiscipline(termios, nil)
 	outBytes := make([]byte, 32)
 	dst := usermem.BytesIOSequence(outBytes)
 	entry := waiter.NewFunctionEntry(waiter.ReadableEvents, func(waiter.EventMask) {

pkg/sentry/fsimpl/devpts/line_discipline.go

@@ -116,8 +116,11 @@ type lineDiscipline struct {
 	terminal *Terminal
 }
 
-func newLineDiscipline(termios linux.KernelTermios) *lineDiscipline {
-	ld := lineDiscipline{termios: termios}
+func newLineDiscipline(termios linux.KernelTermios, terminal *Terminal) *lineDiscipline {
+	ld := lineDiscipline{
+		termios:  termios,
+		terminal: terminal,
+	}
 	ld.inQueue.transformer = &inputQueueTransformer{}
 	ld.outQueue.transformer = &outputQueueTransformer{}
 	return &ld
@@ -397,11 +400,15 @@ func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte)
 				cBytes[0] = '\r'
 			}
 		case l.termios.ControlCharacters[linux.VINTR]: // ctrl-c
-			l.terminal.fgProcessGroup.SendSignal(kernel.SignalInfoPriv(linux.SIGINT))
+			// The input queue is reading from the master TTY and
+			// writing to the replica TTY which is connected to the
+			// interactive program (like bash). We want to send the
+			// signal the process connected to the replica TTY.
+			l.terminal.replicaKTTY.SignalForegroundProcessGroup(kernel.SignalInfoPriv(linux.SIGINT))
 		case l.termios.ControlCharacters[linux.VSUSP]: // ctrl-z
-			l.terminal.fgProcessGroup.SendSignal(kernel.SignalInfoPriv(linux.SIGTSTP))
+			l.terminal.replicaKTTY.SignalForegroundProcessGroup(kernel.SignalInfoPriv(linux.SIGTSTP))
 		case l.termios.ControlCharacters[linux.VQUIT]: // ctrl-\
-			l.terminal.fgProcessGroup.SendSignal(kernel.SignalInfoPriv(linux.SIGQUIT))
+			l.terminal.replicaKTTY.SignalForegroundProcessGroup(kernel.SignalInfoPriv(linux.SIGQUIT))
 		}
 
 		// In canonical mode, we discard non-terminating characters

pkg/sentry/fsimpl/devpts/master.go

@@ -172,16 +172,26 @@ func (mfd *masterFileDescription) Ioctl(ctx context.Context, io usermem.IO, args
 		// Make the given terminal the controlling terminal of the
 		// calling process.
 		steal := args[2].Int() == 1
-		return 0, mfd.t.setControllingTTY(ctx, steal, true /* isMaster */, mfd.vfsfd.IsReadable())
+		return 0, t.ThreadGroup().SetControllingTTY(mfd.t.masterKTTY, steal, mfd.vfsfd.IsReadable())
 	case linux.TIOCNOTTY:
 		// Release this process's controlling terminal.
-		return 0, mfd.t.releaseControllingTTY(ctx, true /* isMaster */)
+		return 0, t.ThreadGroup().ReleaseControllingTTY(mfd.t.masterKTTY)
 	case linux.TIOCGPGRP:
-		// Get the foreground process group.
-		return mfd.t.foregroundProcessGroup(ctx, args, true /* isMaster */)
+		// Get the foreground process group id.
+		pgid, err := t.ThreadGroup().ForegroundProcessGroupID(mfd.t.masterKTTY)
+		if err != nil {
+			return 0, err
+		}
+		ret := primitive.Int32(pgid)
+		_, err = ret.CopyOut(t, args[2].Pointer())
+		return 0, err
 	case linux.TIOCSPGRP:
-		// Set the foreground process group.
-		return mfd.t.setForegroundProcessGroup(ctx, args, true /* isMaster */)
+		// Set the foreground process group id.
+		var pgid primitive.Int32
+		if _, err := pgid.CopyIn(t, args[2].Pointer()); err != nil {
+			return 0, err
+		}
+		return 0, t.ThreadGroup().SetForegroundProcessGroupID(mfd.t.masterKTTY, kernel.ProcessGroupID(pgid))
 	default:
 		maybeEmitUnimplementedEvent(ctx, cmd)
 		return 0, linuxerr.ENOTTY

pkg/sentry/fsimpl/devpts/replica.go

@@ -52,6 +52,10 @@ var _ kernfs.Inode = (*replicaInode)(nil)
 
 // Open implements kernfs.Inode.Open.
 func (ri *replicaInode) Open(ctx context.Context, rp *vfs.ResolvingPath, d *kernfs.Dentry, opts vfs.OpenOptions) (*vfs.FileDescription, error) {
+	t := kernel.TaskFromContext(ctx)
+	if t == nil {
+		panic("open must be called from a task goroutine")
+	}
 	fd := &replicaFileDescription{
 		inode: ri,
 	}
@@ -63,7 +67,7 @@ func (ri *replicaInode) Open(ctx context.Context, rp *vfs.ResolvingPath, d *kern
 		// Opening a replica sets the process' controlling TTY when
 		// possible. An error indicates it cannot be set, and is
 		// ignored silently.
-		_ = fd.inode.t.setControllingTTY(ctx, false /* steal */, false /* isMaster */, fd.vfsfd.IsReadable())
+		_ = t.ThreadGroup().SetControllingTTY(fd.inode.t.replicaKTTY, false /* steal */, fd.vfsfd.IsReadable())
 	}
 	return &fd.vfsfd, nil
 
@@ -174,16 +178,26 @@ func (rfd *replicaFileDescription) Ioctl(ctx context.Context, io usermem.IO, arg
 		// Make the given terminal the controlling terminal of the
 		// calling process.
 		steal := args[2].Int() == 1
-		return 0, rfd.inode.t.setControllingTTY(ctx, steal, false /* isMaster */, rfd.vfsfd.IsReadable())
+		return 0, t.ThreadGroup().SetControllingTTY(rfd.inode.t.replicaKTTY, steal, rfd.vfsfd.IsReadable())
 	case linux.TIOCNOTTY:
 		// Release this process's controlling terminal.
-		return 0, rfd.inode.t.releaseControllingTTY(ctx, false /* isMaster */)
+		return 0, t.ThreadGroup().ReleaseControllingTTY(rfd.inode.t.replicaKTTY)
 	case linux.TIOCGPGRP:
-		// Get the foreground process group.
-		return rfd.inode.t.foregroundProcessGroup(ctx, args, false /* isMaster */)
+		// Get the foreground process group id.
+		pgid, err := t.ThreadGroup().ForegroundProcessGroupID(rfd.inode.t.replicaKTTY)
+		if err != nil {
+			return 0, err
+		}
+		ret := primitive.Int32(pgid)
+		_, err = ret.CopyOut(t, args[2].Pointer())
+		return 0, err
 	case linux.TIOCSPGRP:
-		// Set the foreground process group.
-		return rfd.inode.t.setForegroundProcessGroup(ctx, args, false /* isMaster */)
+		// Set the foreground process group id.
+		var pgid primitive.Int32
+		if _, err := pgid.CopyIn(t, args[2].Pointer()); err != nil {
+			return 0, err
+		}
+		return 0, t.ThreadGroup().SetForegroundProcessGroupID(rfd.inode.t.replicaKTTY, kernel.ProcessGroupID(pgid))
 	default:
 		maybeEmitUnimplementedEvent(ctx, cmd)
 		return 0, linuxerr.ENOTTY

pkg/sentry/fsimpl/devpts/terminal.go

@@ -16,9 +16,6 @@ package devpts
 
 import (
 	"gvisor.dev/gvisor/pkg/abi/linux"
-	"gvisor.dev/gvisor/pkg/context"
-	"gvisor.dev/gvisor/pkg/marshal/primitive"
-	"gvisor.dev/gvisor/pkg/sentry/arch"
 	"gvisor.dev/gvisor/pkg/sentry/kernel"
 )
 
@@ -39,89 +36,15 @@ type Terminal struct {
 	// replicaKTTY contains the controlling process of the replica end of this
 	// terminal. This field is immutable.
 	replicaKTTY *kernel.TTY
-
-	// fgProcessGroup is the foreground process group that is currently
-	// connected to this TTY.
-	fgProcessGroup *kernel.ProcessGroup
 }
 
 func newTerminal(n uint32) *Terminal {
-	termios := linux.DefaultReplicaTermios
-	t := Terminal{
+	t := &Terminal{
 		n:           n,
-		ld:          newLineDiscipline(termios),
 		masterKTTY:  &kernel.TTY{Index: n},
 		replicaKTTY: &kernel.TTY{Index: n},
 	}
+	t.ld = newLineDiscipline(linux.DefaultReplicaTermios, t)
 
-	t.ld.terminal = &t
-
-	return &t
-}
-
-// setControllingTTY makes tm the controlling terminal of the calling thread
-// group.
-func (tm *Terminal) setControllingTTY(ctx context.Context, steal bool, isMaster, isReadable bool) error {
-	task := kernel.TaskFromContext(ctx)
-	if task == nil {
-		panic("setControllingTTY must be called from a task context")
-	}
-
-	return task.ThreadGroup().SetControllingTTY(tm.tty(isMaster), steal, isReadable)
-}
-
-// releaseControllingTTY removes tm as the controlling terminal of the calling
-// thread group.
-func (tm *Terminal) releaseControllingTTY(ctx context.Context, isMaster bool) error {
-	task := kernel.TaskFromContext(ctx)
-	if task == nil {
-		panic("releaseControllingTTY must be called from a task context")
-	}
-
-	return task.ThreadGroup().ReleaseControllingTTY(tm.tty(isMaster))
-}
-
-// foregroundProcessGroup gets the process group ID of tm's foreground process.
-func (tm *Terminal) foregroundProcessGroup(ctx context.Context, args arch.SyscallArguments, isMaster bool) (uintptr, error) {
-	task := kernel.TaskFromContext(ctx)
-	if task == nil {
-		panic("foregroundProcessGroup must be called from a task context")
-	}
-
-	ret, err := task.ThreadGroup().ForegroundProcessGroup(tm.tty(isMaster))
-	if err != nil {
-		return 0, err
-	}
-
-	// Write it out to *arg.
-	retP := primitive.Int32(ret)
-	_, err = retP.CopyOut(task, args[2].Pointer())
-	return 0, err
-}
-
-// foregroundProcessGroup sets tm's foreground process.
-func (tm *Terminal) setForegroundProcessGroup(ctx context.Context, args arch.SyscallArguments, isMaster bool) (uintptr, error) {
-	task := kernel.TaskFromContext(ctx)
-	if task == nil {
-		panic("setForegroundProcessGroup must be called from a task context")
-	}
-
-	// Read in the process group ID.
-	var pgid primitive.Int32
-	if _, err := pgid.CopyIn(task, args[2].Pointer()); err != nil {
-		return 0, err
-	}
-
-	ret, err := task.ThreadGroup().SetForegroundProcessGroup(tm.tty(isMaster), kernel.ProcessGroupID(pgid))
-	if err == nil {
-		tm.fgProcessGroup = task.PIDNamespace().ProcessGroupWithID(kernel.ProcessGroupID(pgid))
-	}
-	return uintptr(ret), err
-}
-
-func (tm *Terminal) tty(isMaster bool) *kernel.TTY {
-	if isMaster {
-		return tm.masterKTTY
-	}
-	return tm.replicaKTTY
+	return t
 }

pkg/sentry/kernel/thread_group.go

@@ -444,9 +444,9 @@ func (tg *ThreadGroup) ReleaseControllingTTY(tty *TTY) error {
 	return lastErr
 }
 
-// ForegroundProcessGroup returns the process group ID of the foreground
-// process group.
-func (tg *ThreadGroup) ForegroundProcessGroup(tty *TTY) (int32, error) {
+// ForegroundProcessGroupID returns the foreground process group ID of the
+// thread group.
+func (tg *ThreadGroup) ForegroundProcessGroupID(tty *TTY) (ProcessGroupID, error) {
 	tty.mu.Lock()
 	defer tty.mu.Unlock()
 
@@ -455,17 +455,18 @@ func (tg *ThreadGroup) ForegroundProcessGroup(tty *TTY) (int32, error) {
 	tg.signalHandlers.mu.Lock()
 	defer tg.signalHandlers.mu.Unlock()
 
-	// "When fd does not refer to the controlling terminal of the calling
-	// process, -1 is returned" - tcgetpgrp(3)
+	// fd must refer to the controlling terminal of the calling process.
+	// See tcgetpgrp(3)
 	if tg.tty != tty {
-		return -1, linuxerr.ENOTTY
+		return 0, linuxerr.ENOTTY
 	}
 
-	return int32(tg.processGroup.session.foreground.id), nil
+	return tg.processGroup.session.foreground.id, nil
 }
 
-// SetForegroundProcessGroup sets the foreground process group of tty to pgid.
-func (tg *ThreadGroup) SetForegroundProcessGroup(tty *TTY, pgid ProcessGroupID) (int32, error) {
+// SetForegroundProcessGroupID sets the foreground process group of tty to
+// pgid.
+func (tg *ThreadGroup) SetForegroundProcessGroupID(tty *TTY, pgid ProcessGroupID) error {
 	tty.mu.Lock()
 	defer tty.mu.Unlock()
 
@@ -476,24 +477,24 @@ func (tg *ThreadGroup) SetForegroundProcessGroup(tty *TTY, pgid ProcessGroupID)
 
 	// tty must be the controlling terminal.
 	if tg.tty != tty {
-		return -1, linuxerr.ENOTTY
+		return linuxerr.ENOTTY
 	}
 
 	// pgid must be positive.
 	if pgid < 0 {
-		return -1, linuxerr.EINVAL
+		return linuxerr.EINVAL
 	}
 
 	// pg must not be empty. Empty process groups are removed from their
 	// pid namespaces.
 	pg, ok := tg.pidns.processGroups[pgid]
 	if !ok {
-		return -1, linuxerr.ESRCH
+		return linuxerr.ESRCH
 	}
 
 	// pg must be part of this process's session.
 	if tg.processGroup.session != pg.session {
-		return -1, linuxerr.EPERM
+		return linuxerr.EPERM
 	}
 
 	signalAction := tg.signalHandlers.actions[linux.SIGTTOU]
@@ -504,11 +505,11 @@ func (tg *ThreadGroup) SetForegroundProcessGroup(tty *TTY, pgid ProcessGroupID)
 	blocked := (linux.SignalSet(tg.leader.signalMask.RacyLoad()) & linux.SignalSetOf(linux.SIGTTOU)) != 0
 	if tg.processGroup.id != tg.processGroup.session.foreground.id && !ignored && !blocked {
 		tg.leader.sendSignalLocked(SignalInfoPriv(linux.SIGTTOU), true)
-		return -1, linuxerr.ERESTARTSYS
+		return linuxerr.ERESTARTSYS
 	}
 
-	tg.processGroup.session.foreground.id = pgid
-	return 0, nil
+	tg.processGroup.session.foreground = pg
+	return nil
 }
 
 // itimerRealListener implements ktime.Listener for ITIMER_REAL expirations.

pkg/sentry/kernel/tty.go

@@ -14,7 +14,11 @@
 
 package kernel
 
-import "gvisor.dev/gvisor/pkg/sync"
+import (
+	"gvisor.dev/gvisor/pkg/abi/linux"
+	"gvisor.dev/gvisor/pkg/log"
+	"gvisor.dev/gvisor/pkg/sync"
+)
 
 // TTY defines the relationship between a thread group and its controlling
 // terminal.
@@ -39,3 +43,28 @@ func (tg *ThreadGroup) TTY() *TTY {
 	defer tg.signalHandlers.mu.Unlock()
 	return tg.tty
 }
+
+// SignalForegroundProcessGroup sends the signal to the foreground process
+// group of the TTY.
+func (tty *TTY) SignalForegroundProcessGroup(info *linux.SignalInfo) {
+	tty.mu.Lock()
+	defer tty.mu.Unlock()
+	tg := tty.tg
+
+	tg.pidns.owner.mu.Lock()
+	tg.signalHandlers.mu.Lock()
+	fg := tg.processGroup.session.foreground
+	tg.signalHandlers.mu.Unlock()
+	tg.pidns.owner.mu.Unlock()
+
+	if fg == nil {
+		// Nothing to signal.
+		return
+	}
+
+	// SendSignal will take TaskSet.mu and signalHandlers.mu, so we cannot
+	// hold them here.
+	if err := fg.SendSignal(info); err != nil {
+		log.Warningf("failed to signal foreground process group (pgid=%d): %v", fg.id, err)
+	}
+}