devpts: Notify of echo'd input queue bytes only after locks have been released.

PiperOrigin-RevId: 383684320

pkg/sentry/fsimpl/devpts/BUILD

@@ -60,5 +60,6 @@ go_test(
         "//pkg/abi/linux",
         "//pkg/sentry/contexttest",
         "//pkg/usermem",
+        "//pkg/waiter",
     ],
 )

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)
+	}
+}

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

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.