Skip to content

checklocks: enforce terminal state locks #14291

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tamird wants to merge 1 commit into from
+77 −38
Closed

checklocks: enforce terminal state locks #14291

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 33 additions & 14 deletions pkg/sentry/fsimpl/devpts/line_discipline.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,10 +85,11 @@ const (
//
// +stateify savable
type lineDiscipline struct {
// sizeMu protects size.
sizeMu sync.Mutex `state:"nosave"`

// size is the terminal size (width and height).
//
// +checklocks:sizeMu
size linux.Winsize

// inQueue is the input queue of the terminal.
Expand All @@ -101,13 +102,19 @@ type lineDiscipline struct {
termiosMu sync.RWMutex `state:"nosave"`

// termios is the terminal configuration used by the lineDiscipline.
//
// +checklocks:termiosMu
termios linux.KernelTermios

// column is the location in a row of the cursor. This is important for
// handling certain special characters like backspace.
//
// +checklocks:outQueue.mu
column int

// numReplicas is the number of replica file descriptors.
//
// +checklocks:termiosMu
numReplicas int

// masterWaiter is used to wait on the master end of the TTY.
Expand All @@ -120,13 +127,17 @@ type lineDiscipline struct {
terminal *Terminal

// packet indicates the master is in packet mode.
//
// +checklocks:termiosMu
packet bool

// packetStatus contains pending TIOCPKT_* status bits for the next master
// read while packet mode is enabled.
//
// Currently only TIOCPKT_FLUSHREAD and TIOCPKT_FLUSHWRITE are emitted
// through packetStatus.
//
// +checklocks:termiosMu
packetStatus uint8
}

Expand Down Expand Up @@ -375,8 +386,12 @@ func (l *lineDiscipline) replicaClose() {

// transformer is a helper interface to make it easier to stateify queue.
type transformer interface {
// transform functions require queue's mutex to be held.
// The boolean indicates whether there was any echoed bytes.
// transform requires the line discipline's termiosMu to be held for reading
// and the queue's mu to be held. checklocks does not propagate these lock
// contracts through this interface.
//
// The boolean indicates whether to notify master readers of possible echo
// output.
transform(*lineDiscipline, *queue, []byte) (int, bool)
}

Expand All @@ -389,9 +404,12 @@ type outputQueueTransformer struct{}
// transform does output processing for one end of the pty. See
// drivers/tty/n_tty.c:do_output_char for an analogous kernel function.
//
// Preconditions:
// - l.termiosMu must be held for reading.
// - q.mu must be held.
// q must be &l.outQueue. checklocks does not infer that identity from the
// queue's transformer, so both mutex paths are annotated below.
//
// +checklocksread:l.termiosMu
// +checklocks:q.mu
// +checklocks:l.outQueue.mu
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.
Expand Down Expand Up @@ -486,12 +504,12 @@ 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.
// It returns an extra boolean indicating whether to notify master readers of
// possible echo output.
//
// +checklocksread:l.termiosMu
// +checklocks:q.mu
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.
Expand Down Expand Up @@ -685,15 +703,16 @@ func (*inputQueueTransformer) transform(l *lineDiscipline, q *queue, buf []byte)
// too many bytes are enqueued, we keep reading input and discarding it until
// we find a terminating character. Signal/echo processing still occurs.
//
// Precondition:
// - l.termiosMu must be held for reading.
// - q.mu must be held.
// +checklocksread:l.termiosMu
// +checklocks:q.mu
func (l *lineDiscipline) shouldDiscard(q *queue, cBytes []byte) bool {
return l.termios.LEnabled(linux.ICANON) && len(q.readBuf)+len(cBytes) >= canonMaxBytes && !l.termios.IsTerminating(cBytes)
}

// peek returns the size in bytes of the next character to process. As long as
// b isn't empty, peek returns a value of at least 1.
//
// +checklocksread:l.termiosMu
func (l *lineDiscipline) peek(b []byte) int {
size := 1
// If UTF-8 support is enabled, runes might be multiple bytes.
Expand Down
68 changes: 44 additions & 24 deletions pkg/sentry/fsimpl/devpts/queue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,27 +40,37 @@ const waitBufMaxBytes = 131072
//
// +stateify savable
type queue struct {
// mu protects everything in queue.
mu sync.Mutex `state:"nosave"`

// readBuf is buffer of data ready to be read when readable is true.
// This data has been processed.
//
// +checklocks:mu
readBuf []byte

// waitBuf contains data that can't fit into readBuf. It is put here
// until it can be loaded into the read buffer. waitBuf contains data
// that hasn't been processed.
waitBuf [][]byte
//
// +checklocks:mu
waitBuf [][]byte

// waitBufLen is the number of bytes in waitBuf.
//
// +checklocks:mu
waitBufLen uint64

// readable indicates whether the read buffer can be read from. In
// canonical mode, there can be an unterminated line in the read buffer,
// so readable must be checked.
//
// +checklocks:mu
readable bool

// transform is the queue's function for transforming bytes
// entering the queue. For example, transform might convert all '\r's
// entering the queue to '\n's.
// transformer processes bytes entering the queue. For example, its
// transform method might convert '\r' to '\n'.
//
// It is immutable after initialization.
transformer
}

Expand Down Expand Up @@ -100,11 +110,10 @@ 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
// - 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).
// - Whether any bytes were processed from the wait buffer.
// - Whether to notify master readers of possible echo output.
//
// Preconditions: l.termiosMu must be held for reading.
// +checklocksread:l.termiosMu
func (q *queue) read(ctx context.Context, dst usermem.IOSequence, l *lineDiscipline, packet bool) (int64, bool, bool, error) {
q.mu.Lock()
defer q.mu.Unlock()
Expand Down Expand Up @@ -133,16 +142,18 @@ func (q *queue) read(ctx context.Context, dst usermem.IOSequence, l *lineDiscipl
}

n, err := dst.CopyOutFrom(ctx, safemem.ReaderFunc(func(dst safemem.BlockSeq) (uint64, error) {
src := safemem.BlockSeqOf(safemem.BlockFromSafeSlice(q.readBuf))
// CopyOutFrom invokes this callback synchronously with q.mu held.
// checklocks does not propagate that lock state into a passed callback.
src := safemem.BlockSeqOf(safemem.BlockFromSafeSlice(q.readBuf)) // +checklocksignore
n, err := safemem.CopySeq(dst, src)
if err != nil {
return 0, err
}
q.readBuf = q.readBuf[n:]
q.readBuf = q.readBuf[n:] // +checklocksignore

// If we read everything, this queue is no longer readable.
if len(q.readBuf) == 0 {
q.readable = false
if len(q.readBuf) == 0 { // +checklocksignore
q.readable = false // +checklocksignore
}

return n, nil
Expand All @@ -158,17 +169,21 @@ func (q *queue) read(ctx context.Context, dst usermem.IOSequence, l *lineDiscipl
}

// write writes to q from userspace.
// The returned boolean indicates whether any data was echoed back.
//
// Preconditions: l.termiosMu must be held for reading.
// The returned boolean indicates whether to notify master readers of possible
// echo output.
//
// +checklocksread:l.termiosMu
func (q *queue) write(ctx context.Context, src usermem.IOSequence, l *lineDiscipline) (int64, bool, error) {
q.mu.Lock()
defer q.mu.Unlock()

// Copy data into the wait buffer.
n, err := src.CopyInTo(ctx, safemem.WriterFunc(func(src safemem.BlockSeq) (uint64, error) {
// CopyInTo invokes this callback synchronously with q.mu held.
// checklocks does not propagate that lock state into a passed callback.
copyLen := src.NumBytes()
room := waitBufMaxBytes - q.waitBufLen
room := waitBufMaxBytes - q.waitBufLen // +checklocksignore
// If out of room, return EAGAIN.
if room == 0 && copyLen > 0 {
return 0, linuxerr.ErrWouldBlock
Expand All @@ -186,7 +201,7 @@ func (q *queue) write(ctx context.Context, src usermem.IOSequence, l *lineDiscip
if err != nil {
return 0, err
}
q.waitBufAppend(buf)
q.waitBufAppend(buf) // +checklocksignore

return n, nil
}))
Expand All @@ -201,9 +216,11 @@ func (q *queue) write(ctx context.Context, src usermem.IOSequence, l *lineDiscip
}

// writeBytes writes to q from b.
// The returned boolean indicates whether any data was echoed back.
//
// Preconditions: l.termiosMu must be held for reading.
// The returned boolean indicates whether to notify master readers of possible
// echo output.
//
// +checklocksread:l.termiosMu
func (q *queue) writeBytes(b []byte, l *lineDiscipline) bool {
q.mu.Lock()
defer q.mu.Unlock()
Expand All @@ -216,11 +233,12 @@ func (q *queue) writeBytes(b []byte, l *lineDiscipline) bool {

// 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.
// The returned boolean indicates whether to notify master readers of possible
// echo output.
//
// +checklocksread:l.termiosMu
// +checklocks:q.mu
func (q *queue) pushWaitBufLocked(l *lineDiscipline) (int, bool) {
if q.waitBufLen == 0 {
return 0, false
Expand Down Expand Up @@ -249,7 +267,9 @@ func (q *queue) pushWaitBufLocked(l *lineDiscipline) (int, bool) {
return total, notifyEcho
}

// Precondition: q.mu must be locked.
// waitBufAppend appends unprocessed data to the wait buffer.
//
// +checklocks:q.mu
func (q *queue) waitBufAppend(b []byte) {
q.waitBuf = append(q.waitBuf, b)
q.waitBufLen += uint64(len(b))
Expand Down