From 8184fa1db0faaa9cf742a6c094f40e5442635bf1 Mon Sep 17 00:00:00 2001 From: Nicolas Lacasse Date: Sat, 25 Feb 2023 14:06:36 -0800 Subject: [PATCH] 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 | 4 +- pkg/sentry/fsimpl/devpts/line_discipline.go | 17 +++-- pkg/sentry/fsimpl/devpts/master.go | 22 ++++-- pkg/sentry/fsimpl/devpts/replica.go | 28 +++++-- pkg/sentry/fsimpl/devpts/terminal.go | 83 +-------------------- pkg/sentry/kernel/thread_group.go | 33 ++++---- pkg/sentry/kernel/tty.go | 31 +++++++- 7 files changed, 101 insertions(+), 117 deletions(-) diff --git a/pkg/sentry/fsimpl/devpts/devpts_test.go b/pkg/sentry/fsimpl/devpts/devpts_test.go index 80e14b0f2..5b7cb9671 100644 --- a/pkg/sentry/fsimpl/devpts/devpts_test.go +++ b/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) { diff --git a/pkg/sentry/fsimpl/devpts/line_discipline.go b/pkg/sentry/fsimpl/devpts/line_discipline.go index 9b58051db..60dd4476f 100644 --- a/pkg/sentry/fsimpl/devpts/line_discipline.go +++ b/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 diff --git a/pkg/sentry/fsimpl/devpts/master.go b/pkg/sentry/fsimpl/devpts/master.go index 738f5c9c0..b3945499d 100644 --- a/pkg/sentry/fsimpl/devpts/master.go +++ b/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 diff --git a/pkg/sentry/fsimpl/devpts/replica.go b/pkg/sentry/fsimpl/devpts/replica.go index 24d925a8f..98c2e5806 100644 --- a/pkg/sentry/fsimpl/devpts/replica.go +++ b/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 diff --git a/pkg/sentry/fsimpl/devpts/terminal.go b/pkg/sentry/fsimpl/devpts/terminal.go index a3068f8a4..e48a3fd44 100644 --- a/pkg/sentry/fsimpl/devpts/terminal.go +++ b/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 } diff --git a/pkg/sentry/kernel/thread_group.go b/pkg/sentry/kernel/thread_group.go index 87c89872d..8285503c7 100644 --- a/pkg/sentry/kernel/thread_group.go +++ b/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. diff --git a/pkg/sentry/kernel/tty.go b/pkg/sentry/kernel/tty.go index d0e0810e8..509ac2e81 100644 --- a/pkg/sentry/kernel/tty.go +++ b/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) + } +}