From cceb04f05a462f90001dc96b75b9216cc688ca84 Mon Sep 17 00:00:00 2001 From: Nicolas Lacasse Date: Thu, 3 Oct 2024 11:23:05 -0700 Subject: [PATCH] Clean up host.TTYFileOperations. We used to track the foreground process group & session on the TTYFileOperation, but these are already tracked in kernel.TTY.ThreadGroup. So remove TTYFileOperations.fgProcessGroup and .session, and replace them with a kernel.TTY. This is analogous to how sentry-internal tty's already work. Updates #10925 PiperOrigin-RevId: 681957240 --- pkg/sentry/control/proc.go | 10 +--- pkg/sentry/fsimpl/devpts/master.go | 2 +- pkg/sentry/fsimpl/devpts/replica.go | 2 +- pkg/sentry/fsimpl/devpts/terminal.go | 2 +- pkg/sentry/fsimpl/host/host.go | 10 +--- pkg/sentry/fsimpl/host/tty.go | 86 ++++++++++++---------------- pkg/sentry/kernel/kernel.go | 7 ++- pkg/sentry/kernel/thread_group.go | 34 ++++++++--- pkg/sentry/kernel/tty.go | 17 +++--- runsc/boot/loader.go | 13 +---- 10 files changed, 81 insertions(+), 102 deletions(-) diff --git a/pkg/sentry/control/proc.go b/pkg/sentry/control/proc.go index 5cfdb53b68..94e84cd4c9 100644 --- a/pkg/sentry/control/proc.go +++ b/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) diff --git a/pkg/sentry/fsimpl/devpts/master.go b/pkg/sentry/fsimpl/devpts/master.go index 1318759723..5bf13481f4 100644 --- a/pkg/sentry/fsimpl/devpts/master.go +++ b/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) diff --git a/pkg/sentry/fsimpl/devpts/replica.go b/pkg/sentry/fsimpl/devpts/replica.go index 59be0d6ad3..bc30831479 100644 --- a/pkg/sentry/fsimpl/devpts/replica.go +++ b/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) diff --git a/pkg/sentry/fsimpl/devpts/terminal.go b/pkg/sentry/fsimpl/devpts/terminal.go index 457840356f..466d818abb 100644 --- a/pkg/sentry/fsimpl/devpts/terminal.go +++ b/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 diff --git a/pkg/sentry/fsimpl/host/host.go b/pkg/sentry/fsimpl/host/host.go index c606050a22..1b2cfd5e57 100644 --- a/pkg/sentry/fsimpl/host/host.go +++ b/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 { diff --git a/pkg/sentry/fsimpl/host/tty.go b/pkg/sentry/fsimpl/host/tty.go index 7de57787f8..73ba03ab94 100644 --- a/pkg/sentry/fsimpl/host/tty.go +++ b/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 } diff --git a/pkg/sentry/kernel/kernel.go b/pkg/sentry/kernel/kernel.go index 7b722882b6..2068293cf2 100644 --- a/pkg/sentry/kernel/kernel.go +++ b/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. diff --git a/pkg/sentry/kernel/thread_group.go b/pkg/sentry/kernel/thread_group.go index a78e0fbffa..9597a85431 100644 --- a/pkg/sentry/kernel/thread_group.go +++ b/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 diff --git a/pkg/sentry/kernel/tty.go b/pkg/sentry/kernel/tty.go index afbe651251..7b36548f47 100644 --- a/pkg/sentry/kernel/tty.go +++ b/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 diff --git a/runsc/boot/loader.go b/runsc/boot/loader.go index 9ad780ea34..aefd82fb04 100644 --- a/runsc/boot/loader.go +++ b/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