Clean up host.TTYFileOperations.

pkg/sentry/control/proc.go

@@ -273,10 +273,7 @@ func (proc *Proc) execAsync(args *ExecArgs) (*kernel.ThreadGroup, kernel.ThreadI
 	}
 
 	if ttyFile != nil {
-		// Index does not matter here. This tty is not coming from a
-		// devpts mount, so it won't collide with any of the ptys
-		// created there.
-		initArgs.TTY = kernel.NewTTY(0, ttyFile)
+		initArgs.TTY = ttyFile.TTY()
 	}
 
 	// Set cgroups to the new exec task if cgroups are mounted.
@@ -299,11 +296,6 @@ func (proc *Proc) execAsync(args *ExecArgs) (*kernel.ThreadGroup, kernel.ThreadI
 		return nil, 0, nil, err
 	}
 
-	// Set the foreground process group on the TTY before starting the process.
-	if ttyFile != nil {
-		ttyFile.InitForegroundProcessGroup(tg.ProcessGroup())
-	}
-
 	// Start the newly created process.
 	proc.Kernel.StartProcess(tg)
 

pkg/sentry/fsimpl/devpts/master.go

@@ -177,7 +177,7 @@ func (mfd *masterFileDescription) Ioctl(ctx context.Context, io usermem.IO, sysn
 		// Make the given terminal the controlling terminal of the
 		// calling process.
 		steal := args[2].Int() == 1
-		return 0, t.ThreadGroup().SetControllingTTY(mfd.t.masterKTTY, steal, mfd.vfsfd.IsReadable())
+		return 0, t.ThreadGroup().SetControllingTTY(ctx, mfd.t.masterKTTY, steal, mfd.vfsfd.IsReadable())
 	case linux.TIOCNOTTY:
 		// Release this process's controlling terminal.
 		return 0, t.ThreadGroup().ReleaseControllingTTY(mfd.t.masterKTTY)

pkg/sentry/fsimpl/devpts/replica.go

@@ -167,7 +167,7 @@ func (rfd *replicaFileDescription) Ioctl(ctx context.Context, io usermem.IO, sys
 		// Make the given terminal the controlling terminal of the
 		// calling process.
 		steal := args[2].Int() == 1
-		return 0, t.ThreadGroup().SetControllingTTY(rfd.inode.t.replicaKTTY, steal, rfd.vfsfd.IsReadable())
+		return 0, t.ThreadGroup().SetControllingTTY(ctx, rfd.inode.t.replicaKTTY, steal, rfd.vfsfd.IsReadable())
 	case linux.TIOCNOTTY:
 		// Release this process's controlling terminal.
 		return 0, t.ThreadGroup().ReleaseControllingTTY(rfd.inode.t.replicaKTTY)

pkg/sentry/fsimpl/devpts/terminal.go

@@ -71,7 +71,7 @@ func (t *Terminal) Open(ctx context.Context, mnt *vfs.Mount, vfsd *vfs.Dentry, o
 		// Opening a replica sets the process' controlling TTY when
 		// possible. An error indicates it cannot be set, and is
 		// ignored silently. See Linux tty_open().
-		_ = tsk.ThreadGroup().SetControllingTTY(t.replicaKTTY, false /* steal */, fd.vfsfd.IsReadable())
+		_ = tsk.ThreadGroup().SetControllingTTY(ctx, t.replicaKTTY, false /* steal */, fd.vfsfd.IsReadable())
 	}
 	ri.t.ld.replicaOpen()
 	return &fd.vfsfd, nil

pkg/sentry/fsimpl/host/host.go

@@ -32,7 +32,6 @@ import (
 	"gvisor.dev/gvisor/pkg/sentry/arch"
 	"gvisor.dev/gvisor/pkg/sentry/fsimpl/kernfs"
 	"gvisor.dev/gvisor/pkg/sentry/hostfd"
-	"gvisor.dev/gvisor/pkg/sentry/kernel"
 	"gvisor.dev/gvisor/pkg/sentry/kernel/auth"
 	"gvisor.dev/gvisor/pkg/sentry/memmap"
 	unixsocket "gvisor.dev/gvisor/pkg/sentry/socket/unix"
@@ -669,14 +668,7 @@ func (i *inode) open(ctx context.Context, d *kernfs.Dentry, mnt *vfs.Mount, file
 
 	case unix.S_IFREG, unix.S_IFIFO, unix.S_IFCHR:
 		if i.isTTY {
-			fd := &TTYFileDescription{
-				fileDescription: fileDescription{inode: i},
-				termios:         linux.DefaultReplicaTermios,
-			}
-			if task := kernel.TaskFromContext(ctx); task != nil {
-				fd.fgProcessGroup = task.ThreadGroup().ProcessGroup()
-				fd.session = fd.fgProcessGroup.Session()
-			}
+			fd := NewTTYFileDescription(i)
 			fd.LockFD.Init(&i.locks)
 			vfsfd := &fd.vfsfd
 			if err := vfsfd.Init(fd, flags, mnt, d.VFSDentry(), &vfs.FileDescriptionOptions{}); err != nil {

pkg/sentry/fsimpl/host/tty.go

@@ -40,15 +40,23 @@ type TTYFileDescription struct {
 	// mu protects the fields below.
 	mu sync.Mutex `state:"nosave"`
 
-	// session is the session attached to this TTYFileDescription.
-	session *kernel.Session
-
-	// fgProcessGroup is the foreground process group that is currently
-	// connected to this TTY.
-	fgProcessGroup *kernel.ProcessGroup
-
 	// termios contains the terminal attributes for this TTY.
 	termios linux.KernelTermios
+
+	// tty is the kernel.TTY associated with this host tty.
+	tty *kernel.TTY
+}
+
+// NewTTYFileDescription returns a new TTYFileDescription.
+func NewTTYFileDescription(i *inode) *TTYFileDescription {
+	fd := &TTYFileDescription{
+		fileDescription: fileDescription{inode: i},
+		termios:         linux.DefaultReplicaTermios,
+	}
+	// Index does not matter here. This tty is not coming from a devpts
+	// mount, so it won't collide with any of the ptys created there.
+	fd.tty = kernel.NewTTY(0, fd)
+	return fd
 }
 
 // Open re-opens the tty fd, for example via open(/dev/tty). See Linux's
@@ -58,33 +66,23 @@ func (t *TTYFileDescription) Open(_ context.Context, _ *vfs.Mount, _ *vfs.Dentry
 	return &t.vfsfd, nil
 }
 
-// InitForegroundProcessGroup sets the foreground process group and session for
-// the TTY. This should only be called once, after the foreground process group
-// has been created, but before it has started running.
-func (t *TTYFileDescription) InitForegroundProcessGroup(pg *kernel.ProcessGroup) {
-	t.mu.Lock()
-	defer t.mu.Unlock()
-	if t.fgProcessGroup != nil {
-		panic("foreground process group is already set")
-	}
-	t.fgProcessGroup = pg
-	t.session = pg.Session()
+// Release implements fs.FileOperations.Release.
+func (t *TTYFileDescription) Release(ctx context.Context) {
+	t.fileDescription.Release(ctx)
 }
 
-// ForegroundProcessGroup returns the foreground process for the TTY.
-func (t *TTYFileDescription) ForegroundProcessGroup() *kernel.ProcessGroup {
+// TTY returns the kernel.TTY.
+func (t *TTYFileDescription) TTY() *kernel.TTY {
 	t.mu.Lock()
 	defer t.mu.Unlock()
-	return t.fgProcessGroup
+	return t.tty
 }
 
-// Release implements fs.FileOperations.Release.
-func (t *TTYFileDescription) Release(ctx context.Context) {
+// ThreadGroup returns the kernel.ThreadGroup associated with this tty.
+func (t *TTYFileDescription) ThreadGroup() *kernel.ThreadGroup {
 	t.mu.Lock()
-	t.fgProcessGroup = nil
-	t.mu.Unlock()
-
-	t.fileDescription.Release(ctx)
+	defer t.mu.Unlock()
+	return t.tty.ThreadGroup()
 }
 
 // PRead implements vfs.FileDescriptionImpl.PRead.
@@ -215,9 +213,14 @@ func (t *TTYFileDescription) Ioctl(ctx context.Context, io usermem.IO, sysno uin
 		t.mu.Lock()
 		defer t.mu.Unlock()
 
+		fgpg, err := t.tty.ThreadGroup().ForegroundProcessGroup(t.tty)
+		if err != nil {
+			return 0, err
+		}
+
 		// Map the ProcessGroup into a ProcessGroupID in the task's PID namespace.
-		pgID := primitive.Int32(pidns.IDOfProcessGroup(t.fgProcessGroup))
-		_, err := pgID.CopyOut(task, args[2].Pointer())
+		pgID := primitive.Int32(pidns.IDOfProcessGroup(fgpg))
+		_, err = pgID.CopyOut(task, args[2].Pointer())
 		return 0, err
 
 	case linux.TIOCSPGRP:
@@ -239,7 +242,7 @@ func (t *TTYFileDescription) Ioctl(ctx context.Context, io usermem.IO, sysno uin
 		}
 
 		// Check that calling task's process group is in the TTY session.
-		if task.ThreadGroup().Session() != t.session {
+		if task.ThreadGroup().Session() != t.tty.ThreadGroup().Session() {
 			return 0, linuxerr.ENOTTY
 		}
 
@@ -248,25 +251,10 @@ func (t *TTYFileDescription) Ioctl(ctx context.Context, io usermem.IO, sysno uin
 			return 0, err
 		}
 		pgID := kernel.ProcessGroupID(pgIDP)
-
-		// pgID must be non-negative.
-		if pgID < 0 {
-			return 0, linuxerr.EINVAL
-		}
-
-		// Process group with pgID must exist in this PID namespace.
-		pidns := task.PIDNamespace()
-		pg := pidns.ProcessGroupWithID(pgID)
-		if pg == nil {
-			return 0, linuxerr.ESRCH
-		}
-
-		// Check that new process group is in the TTY session.
-		if pg.Session() != t.session {
-			return 0, linuxerr.EPERM
+		if err := t.tty.ThreadGroup().SetForegroundProcessGroupID(t.tty, pgID); err != nil {
+			return 0, err
 		}
 
-		t.fgProcessGroup = pg
 		return 0, nil
 
 	case linux.TIOCGWINSZ:
@@ -350,12 +338,12 @@ func (t *TTYFileDescription) checkChange(ctx context.Context, sig linux.Signal)
 	// If the session for the task is different than the session for the
 	// controlling TTY, then the change is allowed. Seems like a bad idea,
 	// but that's exactly what linux does.
-	if tg.Session() != t.fgProcessGroup.Session() {
+	if tg.Session() != t.tty.ThreadGroup().Session() {
 		return nil
 	}
 
 	// If we are the foreground process group, then the change is allowed.
-	if pg == t.fgProcessGroup {
+	if fgpg, _ := t.tty.ThreadGroup().ForegroundProcessGroup(t.tty); pg == fgpg {
 		return nil
 	}
 

pkg/sentry/kernel/kernel.go

@@ -992,7 +992,7 @@ type CreateProcessArgs struct {
 	// Origin indicates how the task was first created.
 	Origin TaskOrigin
 
-	// TTY is the optional TTY to associate with this process.
+	// TTY is the optional controlling TTY to associate with this process.
 	TTY *TTY
 }
 
@@ -1227,8 +1227,9 @@ func (k *Kernel) CreateProcess(args CreateProcessArgs) (*ThreadGroup, ThreadID,
 
 	// Set TTY if configured.
 	if args.TTY != nil {
-		t.tg.tty = args.TTY
-		args.TTY.tg = t.tg
+		if err := t.tg.SetControllingTTY(ctx, args.TTY, false /* steal */, true /* isReadable */); err != nil {
+			return nil, 0, fmt.Errorf("setting controlling tty: %w", err)
+		}
 	}
 
 	// Success.

pkg/sentry/kernel/thread_group.go

@@ -397,8 +397,16 @@ func (tg *ThreadGroup) walkDescendantThreadGroupsLocked(visitor func(*ThreadGrou
 	}
 }
 
+// TTY returns the thread group's controlling terminal. If nil, there is no
+// controlling terminal.
+func (tg *ThreadGroup) TTY() *TTY {
+	sh := tg.signalLock()
+	defer sh.mu.Unlock()
+	return tg.tty
+}
+
 // SetControllingTTY sets tty as the controlling terminal of tg.
-func (tg *ThreadGroup) SetControllingTTY(tty *TTY, steal bool, isReadable bool) error {
+func (tg *ThreadGroup) SetControllingTTY(ctx context.Context, tty *TTY, steal bool, isReadable bool) error {
 	tty.mu.Lock()
 	defer tty.mu.Unlock()
 
@@ -416,11 +424,9 @@ func (tg *ThreadGroup) SetControllingTTY(tty *TTY, steal bool, isReadable bool)
 	}
 	if tg.tty == tty {
 		return nil
-	} else if tg.tty != nil {
-		return linuxerr.EINVAL
 	}
 
-	creds := auth.CredentialsFromContext(tg.leader)
+	creds := auth.CredentialsFromContext(ctx)
 	hasAdmin := creds.HasCapabilityIn(linux.CAP_SYS_ADMIN, creds.UserNamespace.Root())
 
 	// "If this terminal is already the controlling terminal of a different
@@ -519,9 +525,9 @@ func (tg *ThreadGroup) ReleaseControllingTTY(tty *TTY) error {
 	return lastErr
 }
 
-// ForegroundProcessGroupID returns the foreground process group ID of the
-// thread group.
-func (tg *ThreadGroup) ForegroundProcessGroupID(tty *TTY) (ProcessGroupID, error) {
+// ForegroundProcessGroup returns the foreground process group of the thread
+// group.
+func (tg *ThreadGroup) ForegroundProcessGroup(tty *TTY) (*ProcessGroup, error) {
 	tty.mu.Lock()
 	defer tty.mu.Unlock()
 
@@ -533,10 +539,20 @@ func (tg *ThreadGroup) ForegroundProcessGroupID(tty *TTY) (ProcessGroupID, error
 	// fd must refer to the controlling terminal of the calling process.
 	// See tcgetpgrp(3)
 	if tg.tty != tty {
-		return 0, linuxerr.ENOTTY
+		return nil, linuxerr.ENOTTY
 	}
 
-	return tg.processGroup.session.foreground.id, nil
+	return tg.processGroup.session.foreground, nil
+}
+
+// ForegroundProcessGroupID returns the foreground process group ID of the
+// thread group.
+func (tg *ThreadGroup) ForegroundProcessGroupID(tty *TTY) (ProcessGroupID, error) {
+	pg, err := tg.ForegroundProcessGroup(tty)
+	if err != nil {
+		return 0, err
+	}
+	return pg.id, nil
 }
 
 // SetForegroundProcessGroupID sets the foreground process group of tty to

pkg/sentry/kernel/tty.go

@@ -34,12 +34,12 @@ type TTYOperations interface {
 //
 // +stateify savable
 type TTY struct {
-	// index is the terminal index. It is immutable.
-	index uint32
-
 	// TTYOperations holds operations on the tty. It is immutable.
 	TTYOperations
 
+	// index is the terminal index. It is immutable.
+	index uint32
+
 	mu sync.Mutex `state:"nosave"`
 
 	// tg is protected by mu.
@@ -59,12 +59,11 @@ func (tty *TTY) Index() uint32 {
 	return tty.index
 }
 
-// TTY returns the thread group's controlling terminal. If nil, there is no
-// controlling terminal.
-func (tg *ThreadGroup) TTY() *TTY {
-	sh := tg.signalLock()
-	defer sh.mu.Unlock()
-	return tg.tty
+// ThreadGroup returns the ThreadGroup this TTY is associated with.
+func (tty *TTY) ThreadGroup() *ThreadGroup {
+	tty.mu.Lock()
+	defer tty.mu.Unlock()
+	return tty.tg
 }
 
 // SignalForegroundProcessGroup sends the signal to the foreground process

runsc/boot/loader.go

@@ -1111,10 +1111,7 @@ func (l *Loader) createContainerProcess(info *containerInfo) (*kernel.ThreadGrou
 	info.procArgs.FDTable = fdTable
 
 	if ttyFile != nil {
-		// Index does not matter here. This tty is not coming from a
-		// devpts mount, so it won't collide with any of the ptys
-		// created there.
-		info.procArgs.TTY = kernel.NewTTY(0, ttyFile)
+		info.procArgs.TTY = ttyFile.TTY()
 	}
 
 	if info.execFD != nil {
@@ -1176,12 +1173,6 @@ func (l *Loader) createContainerProcess(info *containerInfo) (*kernel.ThreadGrou
 	// CreateProcess takes a reference on FDTable if successful.
 	info.procArgs.FDTable.DecRef(ctx)
 
-	// Set the foreground process group on the TTY to the global init process
-	// group, since that is what we are about to start running.
-	if ttyFile != nil {
-		ttyFile.InitForegroundProcessGroup(tg.ProcessGroup())
-	}
-
 	// Install seccomp filters with the new task if there are any.
 	if info.conf.OCISeccomp {
 		if info.spec.Linux != nil && info.spec.Linux.Seccomp != nil {
@@ -1667,7 +1658,7 @@ func (l *Loader) signalForegrondProcessGroup(cid string, tgid kernel.ThreadID, s
 	if tty == nil {
 		return fmt.Errorf("no TTY attached")
 	}
-	pg := tty.ForegroundProcessGroup()
+	pg, _ := tty.ThreadGroup().ForegroundProcessGroup(tty.TTY())
 	si := &linux.SignalInfo{Signo: signo}
 	if pg == nil {
 		// No foreground process group has been set. Signal the