From 07f2c8b56b5948759b3df6587a8fcea13fbcc82b Mon Sep 17 00:00:00 2001 From: Etienne Perot Date: Thu, 8 Jul 2021 12:01:47 -0700 Subject: devpts: Notify of echo'd input queue bytes only after locks have been released. PiperOrigin-RevId: 383684320 --- pkg/sentry/fsimpl/devpts/BUILD | 1 + pkg/sentry/fsimpl/devpts/devpts_test.go | 34 +++++++++++++++++ pkg/sentry/fsimpl/devpts/line_discipline.go | 57 ++++++++++++++++++++--------- pkg/sentry/fsimpl/devpts/queue.go | 42 ++++++++++++--------- 4 files changed, 99 insertions(+), 35 deletions(-) (limited to 'pkg') diff --git a/pkg/sentry/fsimpl/devpts/BUILD b/pkg/sentry/fsimpl/devpts/BUILD index 50b4c02ef..f981ff296 100644 --- a/pkg/sentry/fsimpl/devpts/BUILD +++ b/pkg/sentry/fsimpl/devpts/BUILD @@ -60,5 +60,6 @@ go_test( "//pkg/abi/linux", "//pkg/sentry/contexttest", "//pkg/usermem", + "//pkg/waiter", ], ) diff --git a/pkg/sentry/fsimpl/devpts/devpts_test.go b/pkg/sentry/fsimpl/devpts/devpts_test.go index 448390cfe..1ef07d702 100644 --- a/pkg/sentry/fsimpl/devpts/devpts_test.go +++ b/pkg/sentry/fsimpl/devpts/devpts_test.go @@ -20,6 +20,7 @@ import ( "gvisor.dev/gvisor/pkg/abi/linux" "gvisor.dev/gvisor/pkg/sentry/contexttest" "gvisor.dev/gvisor/pkg/usermem" + "gvisor.dev/gvisor/pkg/waiter" ) func TestSimpleMasterToReplica(t *testing.T) { @@ -54,3 +55,36 @@ func TestSimpleMasterToReplica(t *testing.T) { t.Fatalf("written and read strings do not match: got %q, want %q", outStr, inStr) } } + +type callback func(*waiter.Entry, waiter.EventMask) + +func (cb callback) Callback(entry *waiter.Entry, mask waiter.EventMask) { + cb(entry, mask) +} + +func TestEchoDeadlock(t *testing.T) { + ctx := contexttest.Context(t) + termios := linux.DefaultReplicaTermios + termios.LocalFlags |= linux.ECHO + ld := newLineDiscipline(termios) + outBytes := make([]byte, 32) + dst := usermem.BytesIOSequence(outBytes) + entry := &waiter.Entry{Callback: callback(func(*waiter.Entry, waiter.EventMask) { + ld.inputQueueRead(ctx, dst) + })} + ld.masterWaiter.EventRegister(entry, waiter.ReadableEvents) + defer ld.masterWaiter.EventUnregister(entry) + inBytes := []byte("hello, tty\n") + n, err := ld.inputQueueWrite(ctx, usermem.BytesIOSequence(inBytes)) + if err != nil { + t.Fatalf("inputQueueWrite: %v", err) + } + if int(n) != len(inBytes) { + t.Fatalf("read wrong length: got %d, want %d", n, len(inBytes)) + } + outStr := string(outBytes[:n]) + inStr := string(inBytes) + if outStr != inStr { + t.Fatalf("written and read strings do not match: got %q, want %q", outStr, inStr) + } +} diff --git a/pkg/sentry/fsimpl/devpts/line_discipline.go b/pkg/sentry/fsimpl/devpts/line_discipline.go index e94a5bac3..9cb21e83b 100644 --- a/pkg/sentry/fsimpl/devpts/line_discipline.go +++ b/pkg/sentry/fsimpl/devpts/line_discipline.go @@ -70,6 +70,10 @@ const ( // +------------------------| output queue |<--------------------------+ // (outputQueueRead) +--------------+ (outputQueueWrite) // +// There is special handling for the ECHO option, where bytes written to the +// input queue are also output back to the terminal by being written to +// l.outQueue by the input queue transformer. +// // Lock order: // termiosMu // inQueue.mu @@ -126,7 +130,6 @@ func (l *lineDiscipline) getTermios(task *kernel.Task, args arch.SyscallArgument // setTermios sets a linux.Termios for the tty. func (l *lineDiscipline) setTermios(task *kernel.Task, args arch.SyscallArguments) (uintptr, error) { l.termiosMu.Lock() - defer l.termiosMu.Unlock() oldCanonEnabled := l.termios.LEnabled(linux.ICANON) // We must copy a Termios struct, not KernelTermios. var t linux.Termios @@ -141,7 +144,10 @@ func (l *lineDiscipline) setTermios(task *kernel.Task, args arch.SyscallArgument l.inQueue.pushWaitBufLocked(l) l.inQueue.readable = true l.inQueue.mu.Unlock() + l.termiosMu.Unlock() l.replicaWaiter.Notify(waiter.ReadableEvents) + } else { + l.termiosMu.Unlock() } return 0, err @@ -179,28 +185,37 @@ func (l *lineDiscipline) inputQueueReadSize(t *kernel.Task, io usermem.IO, args func (l *lineDiscipline) inputQueueRead(ctx context.Context, dst usermem.IOSequence) (int64, error) { l.termiosMu.RLock() - defer l.termiosMu.RUnlock() - n, pushed, err := l.inQueue.read(ctx, dst, l) + n, pushed, notifyEcho, err := l.inQueue.read(ctx, dst, l) + l.termiosMu.RUnlock() if err != nil { return 0, err } if n > 0 { - l.masterWaiter.Notify(waiter.WritableEvents) + if notifyEcho { + l.masterWaiter.Notify(waiter.ReadableEvents | waiter.WritableEvents) + } else { + l.masterWaiter.Notify(waiter.WritableEvents) + } if pushed { l.replicaWaiter.Notify(waiter.ReadableEvents) } return n, nil + } else if notifyEcho { + l.masterWaiter.Notify(waiter.ReadableEvents) } return 0, syserror.ErrWouldBlock } func (l *lineDiscipline) inputQueueWrite(ctx context.Context, src usermem.IOSequence) (int64, error) { l.termiosMu.RLock() - defer l.termiosMu.RUnlock() - n, err := l.inQueue.write(ctx, src, l) + n, notifyEcho, err := l.inQueue.write(ctx, src, l) + l.termiosMu.RUnlock() if err != nil { return 0, err } + if notifyEcho { + l.masterWaiter.Notify(waiter.ReadableEvents) + } if n > 0 { l.replicaWaiter.Notify(waiter.ReadableEvents) return n, nil @@ -214,8 +229,9 @@ func (l *lineDiscipline) outputQueueReadSize(t *kernel.Task, io usermem.IO, args func (l *lineDiscipline) outputQueueRead(ctx context.Context, dst usermem.IOSequence) (int64, error) { l.termiosMu.RLock() - defer l.termiosMu.RUnlock() - n, pushed, err := l.outQueue.read(ctx, dst, l) + // Ignore notifyEcho, as it cannot happen when reading from the output queue. + n, pushed, _, err := l.outQueue.read(ctx, dst, l) + l.termiosMu.RUnlock() if err != nil { return 0, err } @@ -231,8 +247,9 @@ func (l *lineDiscipline) outputQueueRead(ctx context.Context, dst usermem.IOSequ func (l *lineDiscipline) outputQueueWrite(ctx context.Context, src usermem.IOSequence) (int64, error) { l.termiosMu.RLock() - defer l.termiosMu.RUnlock() - n, err := l.outQueue.write(ctx, src, l) + // Ignore notifyEcho, as it cannot happen when writing to the output queue. + n, _, err := l.outQueue.write(ctx, src, l) + l.termiosMu.RUnlock() if err != nil { return 0, err } @@ -246,7 +263,8 @@ func (l *lineDiscipline) outputQueueWrite(ctx context.Context, src usermem.IOSeq // transformer is a helper interface to make it easier to stateify queue. type transformer interface { // transform functions require queue's mutex to be held. - transform(*lineDiscipline, *queue, []byte) int + // The boolean indicates whether there was any echoed bytes. + transform(*lineDiscipline, *queue, []byte) (int, bool) } // outputQueueTransformer implements transformer. It performs line discipline @@ -261,7 +279,7 @@ type outputQueueTransformer struct{} // Preconditions: // * l.termiosMu must be held for reading. // * q.mu must be held. -func (*outputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte) int { +func (*outputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte) (int, bool) { // transformOutput is effectively always in noncanonical mode, as the // master termios never has ICANON set. @@ -270,7 +288,7 @@ func (*outputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte if len(q.readBuf) > 0 { q.readable = true } - return len(buf) + return len(buf), false } var ret int @@ -321,7 +339,7 @@ func (*outputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte if len(q.readBuf) > 0 { q.readable = true } - return ret + return ret, false } // inputQueueTransformer implements transformer. It performs line discipline @@ -334,15 +352,17 @@ type inputQueueTransformer struct{} // transformed according to flags set in the termios struct. See // drivers/tty/n_tty.c:n_tty_receive_char_special for an analogous kernel // function. +// It returns an extra boolean indicating whether any characters need to be +// echoed, in which case we need to notify readers. // // Preconditions: // * l.termiosMu must be held for reading. // * q.mu must be held. -func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte) int { +func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte) (int, bool) { // If there's a line waiting to be read in canonical mode, don't write // anything else to the read buffer. if l.termios.LEnabled(linux.ICANON) && q.readable { - return 0 + return 0, false } maxBytes := nonCanonMaxBytes @@ -351,6 +371,7 @@ func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte) } var ret int + var notifyEcho bool for len(buf) > 0 && len(q.readBuf) < canonMaxBytes { size := l.peek(buf) cBytes := append([]byte{}, buf[:size]...) @@ -397,7 +418,7 @@ func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte) // Anything written to the readBuf will have to be echoed. if l.termios.LEnabled(linux.ECHO) { l.outQueue.writeBytes(cBytes, l) - l.masterWaiter.Notify(waiter.ReadableEvents) + notifyEcho = true } // If we finish a line, make it available for reading. @@ -412,7 +433,7 @@ func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte) q.readable = true } - return ret + return ret, notifyEcho } // shouldDiscard returns whether c should be discarded. In canonical mode, if diff --git a/pkg/sentry/fsimpl/devpts/queue.go b/pkg/sentry/fsimpl/devpts/queue.go index 47b0f1599..ff1d89955 100644 --- a/pkg/sentry/fsimpl/devpts/queue.go +++ b/pkg/sentry/fsimpl/devpts/queue.go @@ -98,17 +98,19 @@ func (q *queue) readableSize(t *kernel.Task, io usermem.IO, args arch.SyscallArg } -// read reads from q to userspace. It returns the number of bytes read as well -// as whether the read caused more readable data to become available (whether +// read reads from q to userspace. It returns: +// - The number of bytes read +// - Whether the read caused more readable data to become available (whether // data was pushed from the wait buffer to the read buffer). +// - Whether any data was echoed back (need to notify readers). // // Preconditions: l.termiosMu must be held for reading. -func (q *queue) read(ctx context.Context, dst usermem.IOSequence, l *lineDiscipline) (int64, bool, error) { +func (q *queue) read(ctx context.Context, dst usermem.IOSequence, l *lineDiscipline) (int64, bool, bool, error) { q.mu.Lock() defer q.mu.Unlock() if !q.readable { - return 0, false, syserror.ErrWouldBlock + return 0, false, false, syserror.ErrWouldBlock } if dst.NumBytes() > canonMaxBytes { @@ -131,19 +133,20 @@ func (q *queue) read(ctx context.Context, dst usermem.IOSequence, l *lineDiscipl return n, nil })) if err != nil { - return 0, false, err + return 0, false, false, err } // Move data from the queue's wait buffer to its read buffer. - nPushed := q.pushWaitBufLocked(l) + nPushed, notifyEcho := q.pushWaitBufLocked(l) - return int64(n), nPushed > 0, nil + return int64(n), nPushed > 0, notifyEcho, nil } // write writes to q from userspace. +// The returned boolean indicates whether any data was echoed back. // // Preconditions: l.termiosMu must be held for reading. -func (q *queue) write(ctx context.Context, src usermem.IOSequence, l *lineDiscipline) (int64, error) { +func (q *queue) write(ctx context.Context, src usermem.IOSequence, l *lineDiscipline) (int64, bool, error) { q.mu.Lock() defer q.mu.Unlock() @@ -173,44 +176,49 @@ func (q *queue) write(ctx context.Context, src usermem.IOSequence, l *lineDiscip return n, nil })) if err != nil { - return 0, err + return 0, false, err } // Push data from the wait to the read buffer. - q.pushWaitBufLocked(l) + _, notifyEcho := q.pushWaitBufLocked(l) - return n, nil + return n, notifyEcho, nil } // writeBytes writes to q from b. +// The returned boolean indicates whether any data was echoed back. // // Preconditions: l.termiosMu must be held for reading. -func (q *queue) writeBytes(b []byte, l *lineDiscipline) { +func (q *queue) writeBytes(b []byte, l *lineDiscipline) bool { q.mu.Lock() defer q.mu.Unlock() // Write to the wait buffer. q.waitBufAppend(b) - q.pushWaitBufLocked(l) + _, notifyEcho := q.pushWaitBufLocked(l) + return notifyEcho } // pushWaitBufLocked fills the queue's read buffer with data from the wait // buffer. +// The returned boolean indicates whether any data was echoed back. // // Preconditions: // * l.termiosMu must be held for reading. // * q.mu must be locked. -func (q *queue) pushWaitBufLocked(l *lineDiscipline) int { +func (q *queue) pushWaitBufLocked(l *lineDiscipline) (int, bool) { if q.waitBufLen == 0 { - return 0 + return 0, false } // Move data from the wait to the read buffer. var total int var i int + var notifyEcho bool for i = 0; i < len(q.waitBuf); i++ { - n := q.transform(l, q, q.waitBuf[i]) + n, echo := q.transform(l, q, q.waitBuf[i]) total += n + notifyEcho = notifyEcho || echo if n != len(q.waitBuf[i]) { // The read buffer filled up without consuming the // entire buffer. @@ -223,7 +231,7 @@ func (q *queue) pushWaitBufLocked(l *lineDiscipline) int { q.waitBuf = q.waitBuf[i:] q.waitBufLen -= uint64(total) - return total + return total, notifyEcho } // Precondition: q.mu must be locked. -- cgit v1.2.3