From 445fa6f40c896ca80013695f90ccea9bac37d2a7 Mon Sep 17 00:00:00 2001 From: Etienne Perot Date: Mon, 28 Nov 2022 17:49:29 -0800 Subject: [PATCH] Lockdep: Print more info in the "unbalanced unlock" case. This CL does the following: - Add the ability for nested locks to have names. - Give names to all current uses of nested locks in the codebase. - Truncate `lockdep` debug stack traces to avoid the clutter from the `lockdep` code itself - Simplify `lockdep` to not longer require `classMap`. PiperOrigin-RevId: 491486620 --- pkg/log/log.go | 17 ++++ pkg/sentry/fsimpl/overlay/BUILD | 5 + pkg/sentry/fsimpl/overlay/filesystem.go | 12 +-- pkg/sentry/kernel/BUILD | 2 + pkg/sentry/kernel/auth/BUILD | 8 +- pkg/sentry/kernel/auth/id_map.go | 4 +- pkg/sentry/kernel/futex/BUILD | 10 +- pkg/sentry/kernel/futex/futex.go | 53 ++++++----- pkg/sentry/kernel/pipe/BUILD | 21 +---- pkg/sentry/kernel/pipe/pipe_unsafe.go | 14 ++- pkg/sentry/kernel/pipe/vfs.go | 6 +- pkg/sentry/kernel/sessions.go | 8 +- pkg/sentry/kernel/task_cgroup.go | 4 +- pkg/sentry/kernel/thread_group.go | 4 +- pkg/sentry/mm/BUILD | 1 + pkg/sentry/mm/lifecycle.go | 8 +- pkg/sentry/socket/unix/transport/BUILD | 23 ++--- .../socket/unix/transport/connectioned.go | 18 ++-- pkg/sync/locking/BUILD | 36 +------ pkg/sync/locking/generic_mutex.go | 33 +++++-- pkg/sync/locking/generic_rwmutex.go | 35 +++++-- pkg/sync/locking/lockdep.go | 93 ++++++++++++------- pkg/sync/locking/lockdep_norace.go | 6 +- pkg/sync/locking/lockdep_test.go | 53 +++++++++-- pkg/sync/locking/locking.bzl | 22 +++-- tools/go_generics/defs.bzl | 6 +- tools/go_generics/main.go | 38 +++++--- 27 files changed, 327 insertions(+), 213 deletions(-) diff --git a/pkg/log/log.go b/pkg/log/log.go index 93d698924..af95fb327 100644 --- a/pkg/log/log.go +++ b/pkg/log/log.go @@ -38,6 +38,7 @@ import ( "io" stdlog "log" "os" + "regexp" "runtime" "sync/atomic" "time" @@ -325,6 +326,22 @@ func Stacks(all bool) []byte { return trace } +// stackRegexp matches one level within a stack trace. +var stackRegexp = regexp.MustCompile("(?m)^\\S+\\(.*\\)$\\r?\\n^\\t\\S+:\\d+.*$\\r?\\n") + +// LocalStack returns the local goroutine stack, excluding the top N entries. +// LocalStack's own entry is excluded by default and does not need to be counted in excludeTopN. +func LocalStack(excludeTopN int) []byte { + replaceNext := excludeTopN + 1 + return stackRegexp.ReplaceAllFunc(Stacks(false), func(s []byte) []byte { + if replaceNext > 0 { + replaceNext-- + return nil + } + return s + }) +} + // Traceback logs the given message and dumps a stacktrace of the current // goroutine. // diff --git a/pkg/sentry/fsimpl/overlay/BUILD b/pkg/sentry/fsimpl/overlay/BUILD index 11a279276..e8135f715 100644 --- a/pkg/sentry/fsimpl/overlay/BUILD +++ b/pkg/sentry/fsimpl/overlay/BUILD @@ -7,6 +7,11 @@ licenses(["notice"]) declare_mutex( name = "dir_mutex", out = "dir_mutex.go", + nested_lock_names = [ + "new", + "replaced", + "child", + ], package = "overlay", prefix = "dir", ) diff --git a/pkg/sentry/fsimpl/overlay/filesystem.go b/pkg/sentry/fsimpl/overlay/filesystem.go index 03838c848..f62d46029 100644 --- a/pkg/sentry/fsimpl/overlay/filesystem.go +++ b/pkg/sentry/fsimpl/overlay/filesystem.go @@ -1137,8 +1137,8 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa if err := newParent.checkPermissions(creds, vfs.MayWrite|vfs.MayExec); err != nil { return err } - newParent.dirMu.NestedLock() - defer newParent.dirMu.NestedUnlock() + newParent.dirMu.NestedLock(dirLockNew) + defer newParent.dirMu.NestedUnlock(dirLockNew) } if newParent.vfsd.IsDead() { return linuxerr.ENOENT @@ -1165,8 +1165,8 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa if genericIsAncestorDentry(replaced, renamed) { return linuxerr.ENOTEMPTY } - replaced.dirMu.NestedLock() - defer replaced.dirMu.NestedUnlock() + replaced.dirMu.NestedLock(dirLockReplaced) + defer replaced.dirMu.NestedUnlock(dirLockReplaced) whiteouts, err = replaced.collectWhiteoutsForRmdirLocked(ctx) if err != nil { return err @@ -1376,8 +1376,8 @@ func (fs *filesystem) RmdirAt(ctx context.Context, rp *vfs.ResolvingPath) error if err := parent.mayDelete(rp.Credentials(), child); err != nil { return err } - child.dirMu.NestedLock() - defer child.dirMu.NestedUnlock() + child.dirMu.NestedLock(dirLockChild) + defer child.dirMu.NestedUnlock(dirLockChild) whiteouts, err := child.collectWhiteoutsForRmdirLocked(ctx) if err != nil { return err diff --git a/pkg/sentry/kernel/BUILD b/pkg/sentry/kernel/BUILD index c4e3701da..6b83f81d5 100644 --- a/pkg/sentry/kernel/BUILD +++ b/pkg/sentry/kernel/BUILD @@ -28,6 +28,7 @@ declare_rwmutex( declare_mutex( name = "task_mutex", out = "task_mutex.go", + nested_lock_names = ["child"], package = "kernel", prefix = "task", ) @@ -56,6 +57,7 @@ declare_mutex( declare_mutex( name = "signal_handlers_mutex", out = "signal_handlers_mutex.go", + nested_lock_names = ["tg"], package = "kernel", prefix = "signalHandlers", ) diff --git a/pkg/sentry/kernel/auth/BUILD b/pkg/sentry/kernel/auth/BUILD index 51827d147..e8a3204b3 100644 --- a/pkg/sentry/kernel/auth/BUILD +++ b/pkg/sentry/kernel/auth/BUILD @@ -1,5 +1,6 @@ load("//tools:defs.bzl", "go_library") load("//tools/go_generics:defs.bzl", "go_template_instance") +load("//pkg/sync/locking:locking.bzl", "declare_mutex") package(licenses = ["notice"]) @@ -42,15 +43,12 @@ go_template_instance( }, ) -go_template_instance( +declare_mutex( name = "user_namespace_mutex", out = "user_namespace_mutex.go", + nested_lock_names = ["ns"], package = "auth", prefix = "userNamespace", - substrs = { - "genericMark": "userNamespace", - }, - template = "//pkg/sync/locking:generic_mutex", ) go_library( diff --git a/pkg/sentry/kernel/auth/id_map.go b/pkg/sentry/kernel/auth/id_map.go index 59b407c13..640a857da 100644 --- a/pkg/sentry/kernel/auth/id_map.go +++ b/pkg/sentry/kernel/auth/id_map.go @@ -69,8 +69,8 @@ func (ns *UserNamespace) mapID(m *idMapSet, id uint32) uint32 { // // Preconditions: end >= start. func (ns *UserNamespace) allIDsMapped(m *idMapSet, start, end uint32) bool { - ns.mu.NestedLock() - defer ns.mu.NestedUnlock() + ns.mu.NestedLock(userNamespaceLockNs) + defer ns.mu.NestedUnlock(userNamespaceLockNs) return m.SpanRange(idMapRange{start, end}) == end-start } diff --git a/pkg/sentry/kernel/futex/BUILD b/pkg/sentry/kernel/futex/BUILD index 11bfe8e5d..a0dd3e3ef 100644 --- a/pkg/sentry/kernel/futex/BUILD +++ b/pkg/sentry/kernel/futex/BUILD @@ -1,17 +1,17 @@ load("//tools:defs.bzl", "go_library", "go_test") load("//tools/go_generics:defs.bzl", "go_template_instance") +load("//pkg/sync/locking:locking.bzl", "declare_mutex") package(licenses = ["notice"]) -go_template_instance( +declare_mutex( name = "futex_mutex", out = "futex_mutex.go", + nested_lock_names = [ + "b", + ], package = "futex", prefix = "futexBucket", - substrs = { - "genericMark": "futexBucket", - }, - template = "//pkg/sync/locking:generic_mutex", ) go_template_instance( diff --git a/pkg/sentry/kernel/futex/futex.go b/pkg/sentry/kernel/futex/futex.go index 6e6687ec4..d2b3eb8e4 100644 --- a/pkg/sentry/kernel/futex/futex.go +++ b/pkg/sentry/kernel/futex/futex.go @@ -408,9 +408,12 @@ func (m *Manager) lockBucket(k *Key) (b *bucket) { } // lockBuckets returns locked buckets for the given keys. -// +checklocksacquire:b1.mu -// +checklocksacquire:b2.mu -func (m *Manager) lockBuckets(k1, k2 *Key) (b1 *bucket, b2 *bucket) { +// It returns which bucket was locked first and second. They may be nil in case the buckets are +// identical or they did not need locking. +// +// +checklocksacquire:lockedFirst.mu +// +checklocksacquire:lockedSecond.mu +func (m *Manager) lockBuckets(k1, k2 *Key) (b1, b2, lockedFirst, lockedSecond *bucket) { // Buckets must be consistently ordered to avoid circular lock // dependencies. We order buckets in m.privateBuckets by index (lowest // index first), and all buckets in m.privateBuckets precede @@ -425,14 +428,16 @@ func (m *Manager) lockBuckets(k1, k2 *Key) (b1 *bucket, b2 *bucket) { switch { case i1 < i2: b1.mu.Lock() - b2.mu.NestedLock() + b2.mu.NestedLock(futexBucketLockB) + return b1, b2, b1, b2 case i2 < i1: b2.mu.Lock() - b1.mu.NestedLock() + b1.mu.NestedLock(futexBucketLockB) + return b1, b2, b2, b1 default: b1.mu.Lock() + return b1, b2, b1, nil // +checklocksforce } - return b1, b2 // +checklocksforce } // At least one of b1 or b2 should be m.sharedBucket. @@ -440,22 +445,28 @@ func (m *Manager) lockBuckets(k1, k2 *Key) (b1 *bucket, b2 *bucket) { b2 = m.sharedBucket if k1.Kind != KindSharedMappable { b1 = m.lockBucket(k1) - } else if k2.Kind != KindSharedMappable { - b2 = m.lockBucket(k2) + b2.mu.NestedLock(futexBucketLockB) + return b1, b2, b1, b2 } - m.sharedBucket.mu.Lock() - return b1, b2 // +checklocksforce + if k2.Kind != KindSharedMappable { + b2 = m.lockBucket(k2) + b1.mu.NestedLock(futexBucketLockB) + return b1, b2, b2, b1 + } + return b1, b2, nil, nil // +checklocksforce } // unlockBuckets unlocks two buckets. -// +checklocksrelease:b1.mu -// +checklocksrelease:b2.mu -func (m *Manager) unlockBuckets(b1, b2 *bucket) { - b1.mu.NestedUnlock() - if b1 != b2 { - b2.mu.Unlock() +// +checklocksrelease:lockedFirst.mu +// +checklocksrelease:lockedSecond.mu +func (m *Manager) unlockBuckets(lockedFirst, lockedSecond *bucket) { + if lockedSecond != nil { + lockedSecond.mu.NestedUnlock(futexBucketLockB) } - return // +checklocksforce + if lockedFirst != nil && lockedFirst != lockedSecond { + lockedFirst.mu.Unlock() + } + return } // Wake wakes up to n waiters matching the bitmask on the given addr. @@ -487,8 +498,8 @@ func (m *Manager) doRequeue(t Target, addr, naddr hostarch.Addr, private bool, c } defer k2.release(t) - b1, b2 := m.lockBuckets(&k1, &k2) - defer m.unlockBuckets(b1, b2) + b1, b2, lockedFirst, lockedSecond := m.lockBuckets(&k1, &k2) + defer m.unlockBuckets(lockedFirst, lockedSecond) if checkval { if err := check(t, addr, val); err != nil { @@ -534,8 +545,8 @@ func (m *Manager) WakeOp(t Target, addr1, addr2 hostarch.Addr, private bool, nwa } defer k2.release(t) - b1, b2 := m.lockBuckets(&k1, &k2) - defer m.unlockBuckets(b1, b2) + b1, b2, lockedFirst, lockedSecond := m.lockBuckets(&k1, &k2) + defer m.unlockBuckets(lockedFirst, lockedSecond) done := 0 cond, err := atomicOp(t, addr2, op) diff --git a/pkg/sentry/kernel/pipe/BUILD b/pkg/sentry/kernel/pipe/BUILD index 430c85a88..7279414f2 100644 --- a/pkg/sentry/kernel/pipe/BUILD +++ b/pkg/sentry/kernel/pipe/BUILD @@ -1,39 +1,28 @@ load("//tools:defs.bzl", "go_library", "go_test") -load("//tools/go_generics:defs.bzl", "go_template_instance") +load("//pkg/sync/locking:locking.bzl", "declare_mutex") package(licenses = ["notice"]) -go_template_instance( +declare_mutex( name = "vfs_mutex", out = "vfs_mutex.go", package = "pipe", prefix = "vfs", - substrs = { - "genericMark": "vfs", - }, - template = "//pkg/sync/locking:generic_mutex", ) -go_template_instance( +declare_mutex( name = "pipe_mutex", out = "pipe_mutex.go", + nested_lock_names = ["pipe"], package = "pipe", prefix = "pipe", - substrs = { - "genericMark": "pipe", - }, - template = "//pkg/sync/locking:generic_mutex", ) -go_template_instance( +declare_mutex( name = "inode_mutex", out = "inode_mutex.go", package = "pipe", prefix = "inode", - substrs = { - "genericMark": "inode", - }, - template = "//pkg/sync/locking:generic_mutex", ) go_library( diff --git a/pkg/sentry/kernel/pipe/pipe_unsafe.go b/pkg/sentry/kernel/pipe/pipe_unsafe.go index 5fdab6587..62b5b143d 100644 --- a/pkg/sentry/kernel/pipe/pipe_unsafe.go +++ b/pkg/sentry/kernel/pipe/pipe_unsafe.go @@ -22,16 +22,20 @@ import ( // consistent for both lockTwoPipes(x, y) and lockTwoPipes(y, x), such that // concurrent calls cannot deadlock. // +// Returns the two pipes in order (first locked pipe, second locked pipe). +// The caller should unlock the second pipe first. +// // Preconditions: x != y. // +checklocksacquire:x.mu // +checklocksacquire:y.mu -func lockTwoPipes(x, y *Pipe) { +func lockTwoPipes(x, y *Pipe) (*Pipe, *Pipe) { // Lock the two pipes in order of increasing address. if uintptr(unsafe.Pointer(x)) < uintptr(unsafe.Pointer(y)) { x.mu.Lock() - y.mu.NestedLock() - } else { - y.mu.Lock() - x.mu.NestedLock() + y.mu.NestedLock(pipeLockPipe) + return x, y } + y.mu.Lock() + x.mu.NestedLock(pipeLockPipe) + return y, x } diff --git a/pkg/sentry/kernel/pipe/vfs.go b/pkg/sentry/kernel/pipe/vfs.go index efdf50bd4..a8320cf83 100644 --- a/pkg/sentry/kernel/pipe/vfs.go +++ b/pkg/sentry/kernel/pipe/vfs.go @@ -404,7 +404,7 @@ func spliceOrTee(ctx context.Context, dst, src *VFSPipeFD, count int64, removeFr return 0, linuxerr.EINVAL } - lockTwoPipes(dst.pipe, src.pipe) + firstLocked, secondLocked := lockTwoPipes(dst.pipe, src.pipe) n, err := dst.pipe.writeLocked(count, func(dsts safemem.BlockSeq) (uint64, error) { n, err := src.pipe.peekLocked(int64(dsts.NumBytes()), func(srcs safemem.BlockSeq) (uint64, error) { return safemem.CopySeq(dsts, srcs) @@ -414,8 +414,8 @@ func spliceOrTee(ctx context.Context, dst, src *VFSPipeFD, count int64, removeFr } return uint64(n), err }) - dst.pipe.mu.Unlock() - src.pipe.mu.NestedUnlock() + secondLocked.mu.NestedUnlock(pipeLockPipe) + firstLocked.mu.Unlock() if n > 0 { dst.pipe.queue.Notify(waiter.ReadableEvents) diff --git a/pkg/sentry/kernel/sessions.go b/pkg/sentry/kernel/sessions.go index 47536213b..3e567f751 100644 --- a/pkg/sentry/kernel/sessions.go +++ b/pkg/sentry/kernel/sessions.go @@ -202,11 +202,11 @@ func (pg *ProcessGroup) handleOrphan() { if tg.processGroup != pg { return } - tg.signalHandlers.mu.NestedLock() + tg.signalHandlers.mu.NestedLock(signalHandlersLockTg) if tg.groupStopComplete { hasStopped = true } - tg.signalHandlers.mu.NestedUnlock() + tg.signalHandlers.mu.NestedUnlock(signalHandlersLockTg) }) if !hasStopped { return @@ -217,10 +217,10 @@ func (pg *ProcessGroup) handleOrphan() { if tg.processGroup != pg { return } - tg.signalHandlers.mu.NestedLock() + tg.signalHandlers.mu.NestedLock(signalHandlersLockTg) tg.leader.sendSignalLocked(SignalInfoPriv(linux.SIGHUP), true /* group */) tg.leader.sendSignalLocked(SignalInfoPriv(linux.SIGCONT), true /* group */) - tg.signalHandlers.mu.NestedUnlock() + tg.signalHandlers.mu.NestedUnlock(signalHandlersLockTg) }) return diff --git a/pkg/sentry/kernel/task_cgroup.go b/pkg/sentry/kernel/task_cgroup.go index 85819a751..2bd6d97f1 100644 --- a/pkg/sentry/kernel/task_cgroup.go +++ b/pkg/sentry/kernel/task_cgroup.go @@ -38,8 +38,8 @@ func (t *Task) EnterInitialCgroups(parent *Task) { } joinSet := t.k.cgroupRegistry.computeInitialGroups(inherit) - t.mu.NestedLock() - defer t.mu.NestedUnlock() + t.mu.NestedLock(taskLockChild) + defer t.mu.NestedUnlock(taskLockChild) // Transfer ownership of joinSet refs to the task's cgset. t.cgroups = joinSet for c := range t.cgroups { diff --git a/pkg/sentry/kernel/thread_group.go b/pkg/sentry/kernel/thread_group.go index 218a808dd..87c89872d 100644 --- a/pkg/sentry/kernel/thread_group.go +++ b/pkg/sentry/kernel/thread_group.go @@ -368,9 +368,9 @@ func (tg *ThreadGroup) SetControllingTTY(tty *TTY, steal bool, isReadable bool) // the same session as the tty's controlling thread // group. if othertg.processGroup.session == tty.tg.processGroup.session { - othertg.signalHandlers.mu.NestedLock() + othertg.signalHandlers.mu.NestedLock(signalHandlersLockTg) othertg.tty = nil - othertg.signalHandlers.mu.NestedUnlock() + othertg.signalHandlers.mu.NestedUnlock(signalHandlersLockTg) } } } diff --git a/pkg/sentry/mm/BUILD b/pkg/sentry/mm/BUILD index ad89ac4c9..055d0f7b1 100644 --- a/pkg/sentry/mm/BUILD +++ b/pkg/sentry/mm/BUILD @@ -28,6 +28,7 @@ declare_rwmutex( declare_rwmutex( name = "active_mutex", out = "active_mutex.go", + nested_lock_names = ["forked"], package = "mm", prefix = "active", ) diff --git a/pkg/sentry/mm/lifecycle.go b/pkg/sentry/mm/lifecycle.go index f70571aec..088a77da8 100644 --- a/pkg/sentry/mm/lifecycle.go +++ b/pkg/sentry/mm/lifecycle.go @@ -139,10 +139,10 @@ func (mm *MemoryManager) Fork(ctx context.Context) (*MemoryManager, error) { // regenerated by calling memmap.Mappable.Translate is a waste of time. // (Linux does the same; compare kernel/fork.c:dup_mmap() => // mm/memory.c:copy_page_range().) - mm2.activeMu.Lock() - defer mm2.activeMu.Unlock() - mm.activeMu.NestedLock() - defer mm.activeMu.NestedUnlock() + mm.activeMu.Lock() + defer mm.activeMu.Unlock() + mm2.activeMu.NestedLock(activeLockForked) + defer mm2.activeMu.NestedUnlock(activeLockForked) if dontforks { defer mm.pmas.MergeRange(mm.applicationAddrRange()) } diff --git a/pkg/sentry/socket/unix/transport/BUILD b/pkg/sentry/socket/unix/transport/BUILD index b2f6d79e8..7d74be1cd 100644 --- a/pkg/sentry/socket/unix/transport/BUILD +++ b/pkg/sentry/socket/unix/transport/BUILD @@ -1,39 +1,32 @@ load("//tools:defs.bzl", "go_library") +load("//pkg/sync/locking:locking.bzl", "declare_mutex") load("//tools/go_generics:defs.bzl", "go_template_instance") package(licenses = ["notice"]) -go_template_instance( +declare_mutex( name = "queue_mutex", out = "queue_mutex.go", package = "transport", prefix = "queue", - substrs = { - "genericMark": "unixQueue", - }, - template = "//pkg/sync/locking:generic_mutex", ) -go_template_instance( +declare_mutex( name = "stream_queue_receiver_mutex", out = "stream_queue_receiver_mutex.go", package = "transport", prefix = "streamQueueReceiver", - substrs = { - "genericMark": "streamQueueReceiver", - }, - template = "//pkg/sync/locking:generic_mutex", ) -go_template_instance( +declare_mutex( name = "endpoint_mutex", out = "endpoint_mutex.go", + nested_lock_names = [ + "e", + "ce", + ], package = "transport", prefix = "endpoint", - substrs = { - "genericMark": "unixEndpoint", - }, - template = "//pkg/sync/locking:generic_mutex", ) go_template_instance( diff --git a/pkg/sentry/socket/unix/transport/connectioned.go b/pkg/sentry/socket/unix/transport/connectioned.go index f609825c3..a61c145b6 100644 --- a/pkg/sentry/socket/unix/transport/connectioned.go +++ b/pkg/sentry/socket/unix/transport/connectioned.go @@ -28,8 +28,8 @@ import ( type locker interface { Lock() Unlock() - NestedLock() - NestedUnlock() + NestedLock(endpointlockNameIndex) + NestedUnlock(endpointlockNameIndex) } // A ConnectingEndpoint is a connectioned unix endpoint that is attempting to @@ -288,27 +288,27 @@ func (e *connectionedEndpoint) BidirectionalConnect(ctx context.Context, ce Conn // Do a dance to safely acquire locks on both endpoints. if e.id < ce.ID() { e.Lock() - ce.NestedLock() + ce.NestedLock(endpointLockCe) } else { ce.Lock() - e.NestedLock() + e.NestedLock(endpointLockE) } // Check connecting state. if ce.Connected() { - e.NestedUnlock() + e.NestedUnlock(endpointLockE) ce.Unlock() return syserr.ErrAlreadyConnected } if ce.ListeningLocked() { - e.NestedUnlock() + e.NestedUnlock(endpointLockE) ce.Unlock() return syserr.ErrInvalidEndpointState } // Check bound state. if !e.ListeningLocked() { - e.NestedUnlock() + e.NestedUnlock(endpointLockE) ce.Unlock() return syserr.ErrConnectionRefused } @@ -359,7 +359,7 @@ func (e *connectionedEndpoint) BidirectionalConnect(ctx context.Context, ce Conn } // Notify can deadlock if we are holding these locks. - e.NestedUnlock() + e.NestedUnlock(endpointLockE) ce.Unlock() // Notify on both ends. @@ -369,7 +369,7 @@ func (e *connectionedEndpoint) BidirectionalConnect(ctx context.Context, ce Conn return nil default: // Busy; return EAGAIN per spec. - e.NestedUnlock() + e.NestedUnlock(endpointLockE) ce.Unlock() ne.Close(ctx) return syserr.ErrTryAgain diff --git a/pkg/sync/locking/BUILD b/pkg/sync/locking/BUILD index cf91c0a2b..9ce256024 100644 --- a/pkg/sync/locking/BUILD +++ b/pkg/sync/locking/BUILD @@ -11,9 +11,7 @@ go_library( name = "locking", srcs = [ "atomicptrmap_ancestors_unsafe.go", - "atomicptrmap_class_unsafe.go", "atomicptrmap_goroutine_unsafe.go", - "atomicptrmap_subclass_unsafe.go", "lockdep.go", "lockdep_norace.go", "locking.go", @@ -41,36 +39,6 @@ go_template_instance( }, ) -go_template_instance( - name = "atomicptrmap_class", - out = "atomicptrmap_class_unsafe.go", - imports = { - "reflect": "reflect", - }, - package = "locking", - prefix = "class", - template = "//pkg/sync/atomicptrmap:generic_atomicptrmap", - types = { - "Key": "*MutexClass", - "Value": "reflect.Type", - }, -) - -go_template_instance( - name = "atomicptrmap_subclass", - out = "atomicptrmap_subclass_unsafe.go", - imports = { - "reflect": "reflect", - }, - package = "locking", - prefix = "subclass", - template = "//pkg/sync/atomicptrmap:generic_atomicptrmap", - types = { - "Key": "uint32", - "Value": "MutexClass", - }, -) - go_template_instance( name = "atomicptrmap_ancestors", out = "atomicptrmap_ancestors_unsafe.go", @@ -101,6 +69,10 @@ go_template( declare_mutex( name = "mutex_test", out = "mutex_test.go", + nested_lock_names = [ + "m2", + "m3", + ], package = "locking_test", prefix = "test", ) diff --git a/pkg/sync/locking/generic_mutex.go b/pkg/sync/locking/generic_mutex.go index fb5e383ba..ba6e56a61 100644 --- a/pkg/sync/locking/generic_mutex.go +++ b/pkg/sync/locking/generic_mutex.go @@ -26,36 +26,53 @@ type Mutex struct { mu sync.Mutex } +var genericMarkIndex *locking.MutexClass + +// lockNames is a list of user-friendly lock names. +// Populated in init. +var lockNames []string + +// lockNameIndex is used as an index passed to NestedLock and NestedUnlock, +// refering to an index within lockNames. +// Values are specified using the "consts" field of go_template_instance. +type lockNameIndex int + +// DO NOT REMOVE: The following function automatically replaced with lock index constants. +// LOCK_NAME_INDEX_CONSTANTS +const () + // Lock locks m. // +checklocksignore func (m *Mutex) Lock() { - locking.AddGLock(genericMarkIndex, 0) + locking.AddGLock(genericMarkIndex, -1) m.mu.Lock() } // NestedLock locks m knowing that another lock of the same type is held. // +checklocksignore -func (m *Mutex) NestedLock() { - locking.AddGLock(genericMarkIndex, 1) +func (m *Mutex) NestedLock(i lockNameIndex) { + locking.AddGLock(genericMarkIndex, int(i)) m.mu.Lock() } // Unlock unlocks m. // +checklocksignore func (m *Mutex) Unlock() { - locking.DelGLock(genericMarkIndex, 0) + locking.DelGLock(genericMarkIndex, -1) m.mu.Unlock() } // NestedUnlock unlocks m knowing that another lock of the same type is held. // +checklocksignore -func (m *Mutex) NestedUnlock() { - locking.DelGLock(genericMarkIndex, 1) +func (m *Mutex) NestedUnlock(i lockNameIndex) { + locking.DelGLock(genericMarkIndex, int(i)) m.mu.Unlock() } -var genericMarkIndex *locking.MutexClass +// DO NOT REMOVE: The following function is automatically replaced. +func initLockNames() {} func init() { - genericMarkIndex = locking.NewMutexClass(reflect.TypeOf(Mutex{})) + initLockNames() + genericMarkIndex = locking.NewMutexClass(reflect.TypeOf(Mutex{}), lockNames) } diff --git a/pkg/sync/locking/generic_rwmutex.go b/pkg/sync/locking/generic_rwmutex.go index 9fe6a72ab..c8ee2268c 100644 --- a/pkg/sync/locking/generic_rwmutex.go +++ b/pkg/sync/locking/generic_rwmutex.go @@ -26,17 +26,30 @@ type RWMutex struct { mu sync.RWMutex } +// lockNames is a list of user-friendly lock names. +// Populated in init. +var lockNames []string + +// lockNameIndex is used as an index passed to NestedLock and NestedUnlock, +// refering to an index within lockNames. +// Values are specified using the "consts" field of go_template_instance. +type lockNameIndex int + +// DO NOT REMOVE: The following function automatically replaced with lock index constants. +// LOCK_NAME_INDEX_CONSTANTS +const () + // Lock locks m. // +checklocksignore func (m *RWMutex) Lock() { - locking.AddGLock(genericMarkIndex, 0) + locking.AddGLock(genericMarkIndex, -1) m.mu.Lock() } // NestedLock locks m knowing that another lock of the same type is held. // +checklocksignore -func (m *RWMutex) NestedLock() { - locking.AddGLock(genericMarkIndex, 1) +func (m *RWMutex) NestedLock(i lockNameIndex) { + locking.AddGLock(genericMarkIndex, int(i)) m.mu.Lock() } @@ -44,20 +57,20 @@ func (m *RWMutex) NestedLock() { // +checklocksignore func (m *RWMutex) Unlock() { m.mu.Unlock() - locking.DelGLock(genericMarkIndex, 0) + locking.DelGLock(genericMarkIndex, -1) } // NestedUnlock unlocks m knowing that another lock of the same type is held. // +checklocksignore -func (m *RWMutex) NestedUnlock() { +func (m *RWMutex) NestedUnlock(i lockNameIndex) { m.mu.Unlock() - locking.DelGLock(genericMarkIndex, 1) + locking.DelGLock(genericMarkIndex, int(i)) } // RLock locks m for reading. // +checklocksignore func (m *RWMutex) RLock() { - locking.AddGLock(genericMarkIndex, 0) + locking.AddGLock(genericMarkIndex, -1) m.mu.RLock() } @@ -65,7 +78,7 @@ func (m *RWMutex) RLock() { // +checklocksignore func (m *RWMutex) RUnlock() { m.mu.RUnlock() - locking.DelGLock(genericMarkIndex, 0) + locking.DelGLock(genericMarkIndex, -1) } // RLockBypass locks m for reading without executing the validator. @@ -88,6 +101,10 @@ func (m *RWMutex) DowngradeLock() { var genericMarkIndex *locking.MutexClass +// DO NOT REMOVE: The following function is automatically replaced. +func initLockNames() {} + func init() { - genericMarkIndex = locking.NewMutexClass(reflect.TypeOf(RWMutex{})) + initLockNames() + genericMarkIndex = locking.NewMutexClass(reflect.TypeOf(RWMutex{}), lockNames) } diff --git a/pkg/sync/locking/lockdep.go b/pkg/sync/locking/lockdep.go index f5739f874..092a5765e 100644 --- a/pkg/sync/locking/lockdep.go +++ b/pkg/sync/locking/lockdep.go @@ -26,46 +26,71 @@ import ( "gvisor.dev/gvisor/pkg/log" ) -var classMap classAtomicPtrMap - // NewMutexClass allocates a new mutex class. -func NewMutexClass(t reflect.Type) *MutexClass { - c := &MutexClass{} - classMap.Store(c, &t) +func NewMutexClass(t reflect.Type, lockNames []string) *MutexClass { + c := &MutexClass{ + typ: t, + nestedLockNames: lockNames, + nestedLockClasses: make([]*MutexClass, len(lockNames)), + } + for i := range lockNames { + c.nestedLockClasses[i] = NewMutexClass(t, nil) + c.nestedLockClasses[i].lockName = lockNames[i] + } return c } // MutexClass describes dependencies of a specific class. type MutexClass struct { + // The type of the mutex. + typ reflect.Type + + // Name of the nested lock of the above type. + lockName string + // ancestors are locks that are locked before the current class. ancestors ancestorsAtomicPtrMap - // subclasses is the list of sub-classes that are used to handle nested locks. - subclasses subclassAtomicPtrMap + // nestedLockNames is a list of names for nested locks which are considered difference instances + // of the same lock class. + nestedLockNames []string + // namedLockClasses is a list of MutexClass instances of the same mutex class, but that are + // considered OK to lock simultaneously with each other, as well as with this mutex class. + // This is used for nested locking, where multiple instances of the same lock class are used + // simultaneously. + // Maps one-to-one with nestedLockNames. + nestedLockClasses []*MutexClass +} + +func (m *MutexClass) String() string { + if m.lockName == "" { + return m.typ.String() + } + return fmt.Sprintf("%s[%s]", m.typ.String(), m.lockName) } type goroutineLocks map[*MutexClass]bool var routineLocks goroutineLocksAtomicPtrMap -// checkLock checks that class isn't in ancestors of prevClass. +// checkLock checks that class isn't in the ancestors of prevClass. func checkLock(class *MutexClass, prevClass *MutexClass, chain []*MutexClass) { chain = append(chain, prevClass) if c := prevClass.ancestors.Load(class); c != nil { var b strings.Builder fmt.Fprintf(&b, "WARNING: circular locking detected: %s -> %s:\n%s\n", - *classMap.Load(chain[0]), *classMap.Load(class), log.Stacks(false)) + chain[0], class, log.LocalStack(3)) fmt.Fprintf(&b, "known lock chain: ") c := class for i := len(chain) - 1; i >= 0; i-- { - fmt.Fprintf(&b, "%s -> ", *classMap.Load(c)) + fmt.Fprintf(&b, "%s -> ", c) c = chain[i] } - fmt.Fprintf(&b, "%s\n", *classMap.Load(chain[0])) + fmt.Fprintf(&b, "%s\n", chain[0]) c = class for i := len(chain) - 1; i >= 0; i-- { fmt.Fprintf(&b, "\n====== %s -> %s =====\n%s", - *classMap.Load(c), *classMap.Load(chain[i]), *chain[i].ancestors.Load(c)) + c, chain[i], *chain[i].ancestors.Load(c)) c = chain[i] } panic(b.String()) @@ -78,18 +103,12 @@ func checkLock(class *MutexClass, prevClass *MutexClass, chain []*MutexClass) { }) } -// AddGLock records a lock to the current goroutine and updates dependences. -func AddGLock(class *MutexClass, subclass uint32) { +// AddGLock records a lock to the current goroutine and updates dependencies. +func AddGLock(class *MutexClass, lockNameIndex int) { gid := goid.Get() - if subclass != 0 { - var c *MutexClass - if c = class.subclasses.Load(subclass); c == nil { - t := classMap.Load(class) - c = NewMutexClass(*t) - class.subclasses.Store(subclass, c) - } - class = c + if lockNameIndex != -1 { + class = class.nestedLockClasses[lockNameIndex] } currentLocks := routineLocks.Load(gid) if currentLocks == nil { @@ -102,24 +121,22 @@ func AddGLock(class *MutexClass, subclass uint32) { // Check dependencies and add locked mutexes to the ancestors list. for prevClass := range *currentLocks { if prevClass == class { - panic(fmt.Sprintf("nested locking: %s:\n%s", *classMap.Load(class), log.Stacks(false))) + panic(fmt.Sprintf("nested locking: %s:\n%s", class, log.LocalStack(2))) } checkLock(class, prevClass, nil) if c := class.ancestors.Load(prevClass); c == nil { - stacks := string(log.Stacks(false)) + stacks := string(log.LocalStack(2)) class.ancestors.Store(prevClass, &stacks) } } (*currentLocks)[class] = true - } // DelGLock deletes a lock from the current goroutine. -func DelGLock(class *MutexClass, subclass uint32) { - origClass := class - if subclass != 0 { - class = class.subclasses.Load(subclass) +func DelGLock(class *MutexClass, lockNameIndex int) { + if lockNameIndex != -1 { + class = class.nestedLockClasses[lockNameIndex] } gid := goid.Get() currentLocks := routineLocks.Load(gid) @@ -128,11 +145,25 @@ func DelGLock(class *MutexClass, subclass uint32) { } if _, ok := (*currentLocks)[class]; !ok { var b strings.Builder - fmt.Fprintf(&b, "unbalance unlock: %s:%d:\n", *classMap.Load(origClass), subclass) + fmt.Fprintf(&b, "Lock not held: %s:\n", class) + fmt.Fprintf(&b, "Current stack:\n%s\n", string(log.LocalStack(2))) fmt.Fprintf(&b, "Current locks:\n") for c := range *currentLocks { - fmt.Fprintf(&b, "\t%s\n", *classMap.Load(c)) + heldToClass := class.ancestors.Load(c) + classToHeld := c.ancestors.Load(class) + if heldToClass == nil && classToHeld == nil { + fmt.Fprintf(&b, "\t- Holding lock: %s (no dependency to/from %s found)\n", c, class) + } else if heldToClass != nil && classToHeld != nil { + fmt.Fprintf(&b, "\t- Holding lock: %s (mutual dependency with %s found, this should never happen)\n", c, class) + } else if heldToClass != nil && classToHeld == nil { + fmt.Fprintf(&b, "\t- Holding lock: %s (dependency: %s -> %s)\n", c, c, class) + fmt.Fprintf(&b, "%s\n\n", *heldToClass) + } else if heldToClass == nil && classToHeld != nil { + fmt.Fprintf(&b, "\t- Holding lock: %s (dependency: %s -> %s)\n", c, class, c) + fmt.Fprintf(&b, "%s\n\n", *classToHeld) + } } + fmt.Fprintf(&b, "** End of locks held **\n") panic(b.String()) } diff --git a/pkg/sync/locking/lockdep_norace.go b/pkg/sync/locking/lockdep_norace.go index 5a3b7e9ad..379dc9edf 100644 --- a/pkg/sync/locking/lockdep_norace.go +++ b/pkg/sync/locking/lockdep_norace.go @@ -27,16 +27,16 @@ type goroutineLocks map[*MutexClass]bool type MutexClass struct{} // NewMutexClass is no-op without the lockdep tag. -func NewMutexClass(t reflect.Type) *MutexClass { +func NewMutexClass(reflect.Type, []string) *MutexClass { return nil } // AddGLock is no-op without the lockdep tag. // //go:inline -func AddGLock(class *MutexClass, subclass uint32) {} +func AddGLock(*MutexClass, int) {} // DelGLock is no-op without the lockdep tag. // //go:inline -func DelGLock(class *MutexClass, subclass uint32) {} +func DelGLock(*MutexClass, int) {} diff --git a/pkg/sync/locking/lockdep_test.go b/pkg/sync/locking/lockdep_test.go index c00d39b91..c74add875 100644 --- a/pkg/sync/locking/lockdep_test.go +++ b/pkg/sync/locking/lockdep_test.go @@ -87,9 +87,9 @@ func TestReverseNested(t *testing.T) { m1 := testMutex{} m2 := testMutex{} m1.Lock() - m2.NestedLock() + m2.NestedLock(testLockM2) m1.Unlock() - m2.NestedUnlock() + m2.NestedUnlock(testLockM2) defer func() { if r := recover(); r != nil { @@ -97,24 +97,63 @@ func TestReverseNested(t *testing.T) { } }() - m2.NestedLock() + m2.NestedLock(testLockM2) m1.Lock() - m1.NestedUnlock() + m1.NestedUnlock(testLockM2) m2.Unlock() t.Error("The reverse lock order hasn't been detected") } +func TestReverseNestedDeeper(t *testing.T) { + m1 := testMutex{} + m2 := testMutex{} + m3 := testMutex{} + m1.Lock() + m2.NestedLock(testLockM2) + m3.NestedLock(testLockM3) + m1.Unlock() + m3.NestedUnlock(testLockM3) + m2.NestedUnlock(testLockM2) + + m1.Lock() + m2.NestedLock(testLockM2) + m3.NestedLock(testLockM3) + m1.Unlock() + m2.NestedUnlock(testLockM2) + m3.NestedUnlock(testLockM3) + + defer func() { + if r := recover(); r != nil { + t.Logf("Got expected panic: %s", r) + } + }() + + m2.NestedLock(testLockM2) + m3.NestedLock(testLockM3) + m1.Lock() + m1.Unlock() + m3.NestedUnlock(testLockM3) + m2.NestedUnlock(testLockM2) + + t.Error("The reverse lock order hasn't been detected") +} + func TestUnknownLock(t *testing.T) { m1 := testMutex{} - m2 := test2RWMutex{} + m2 := testMutex{} + m1.Lock() - m2.Lock() + m2.NestedLock(testLockM2) + m2.NestedUnlock(testLockM2) + m1.Unlock() + defer func() { if r := recover(); r != nil { t.Logf("Got expected panic: %s", r) } }() - m2.NestedUnlock() + m1.Lock() + m2.NestedUnlock(testLockM2) t.Error("An unknown lock has not been detected.") } diff --git a/pkg/sync/locking/locking.bzl b/pkg/sync/locking/locking.bzl index 8c65b029f..fe518e951 100644 --- a/pkg/sync/locking/locking.bzl +++ b/pkg/sync/locking/locking.bzl @@ -2,26 +2,32 @@ load("//tools/go_generics:defs.bzl", "go_template_instance") -def declare_mutex(package, name, out, prefix): +def _substrs(nested_lock_names): + substrs = {"genericMark": "prefix"} + if nested_lock_names == None or len(nested_lock_names) == 0: + return substrs + quoted_names = ["\"%s\"" % (n,) for n in nested_lock_names] + constant_names = ["Lock%s = lockNameIndex(%d)" % (n.title(), idx) for idx, n in enumerate(nested_lock_names)] + substrs["func initLockNames() {}"] = "func initLockNames() { lockNames = []string{%s} }" % (", ".join(quoted_names),) + substrs["/" + "/ LOCK_NAME_INDEX_CONSTANTS"] = "const (\n\t%s\n)" % ("\n\t".join(constant_names),) + return substrs + +def declare_mutex(package, name, out, prefix, nested_lock_names = None): go_template_instance( name = name, out = out, package = package, prefix = prefix, - substrs = { - "genericMark": "prefix", - }, + input_substrs = _substrs(nested_lock_names = nested_lock_names), template = "//pkg/sync/locking:generic_mutex", ) -def declare_rwmutex(package, name, out, prefix): +def declare_rwmutex(package, name, out, prefix, nested_lock_names = None): go_template_instance( name = name, out = out, package = package, prefix = prefix, - substrs = { - "genericMark": "prefix", - }, + input_substrs = _substrs(nested_lock_names = nested_lock_names), template = "//pkg/sync/locking:generic_rwmutex", ) diff --git a/tools/go_generics/defs.bzl b/tools/go_generics/defs.bzl index 2443673d0..0318d8ac5 100644 --- a/tools/go_generics/defs.bzl +++ b/tools/go_generics/defs.bzl @@ -93,7 +93,8 @@ def _go_template_instance_impl(ctx): args += [("-t=%s=%s" % (p[0], p[1])) for p in ctx.attr.types.items()] args += [("-c=%s=%s" % (p[0], p[1])) for p in ctx.attr.consts.items()] args += [("-import=%s=%s" % (p[0], p[1])) for p in ctx.attr.imports.items()] - args += [("-s=%s=%s" % (p[0], p[1])) for p in ctx.attr.substrs.items()] + args += [("-in-substr=%s=%s" % (p[0], p[1])) for p in ctx.attr.input_substrs.items()] + args += [("-out-substr=%s=%s" % (p[0], p[1])) for p in ctx.attr.substrs.items()] if ctx.attr.anon: args.append("-anon") @@ -120,7 +121,8 @@ go_template_instance = rule( "types": attr.string_dict(doc = "the map from generic type names to concrete ones"), "consts": attr.string_dict(doc = "the map from constant names to their values"), "imports": attr.string_dict(doc = "the map from imports used in types/consts to their import paths"), - "substrs": attr.string_dict(doc = "the map from sub-strings to their replacements"), + "input_substrs": attr.string_dict(doc = "the map from sub-strings to their replacements, applied just after reading the template code"), + "substrs": attr.string_dict(doc = "the map from sub-strings to their replacements, applied just before writing the template instance code"), "anon": attr.bool(doc = "whether anoymous fields should be processed", mandatory = False, default = False), "package": attr.string(doc = "the package for the generated source file", mandatory = False), "out": attr.output(doc = "output file", mandatory = True), diff --git a/tools/go_generics/main.go b/tools/go_generics/main.go index 60494067c..8387e39c6 100644 --- a/tools/go_generics/main.go +++ b/tools/go_generics/main.go @@ -107,17 +107,18 @@ import ( ) var ( - input = flag.String("i", "", "input `file`") - output = flag.String("o", "", "output `file`") - suffix = flag.String("suffix", "", "`suffix` to add to each global symbol") - prefix = flag.String("prefix", "", "`prefix` to add to each global symbol") - packageName = flag.String("p", "main", "output package `name`") - printAST = flag.Bool("ast", false, "prints the AST") - processAnon = flag.Bool("anon", false, "process anonymous fields") - types = make(mapValue) - consts = make(mapValue) - imports = make(mapValue) - substr = make(mapValue) + input = flag.String("i", "", "input `file`") + output = flag.String("o", "", "output `file`") + suffix = flag.String("suffix", "", "`suffix` to add to each global symbol") + prefix = flag.String("prefix", "", "`prefix` to add to each global symbol") + packageName = flag.String("p", "main", "output package `name`") + printAST = flag.Bool("ast", false, "prints the AST") + processAnon = flag.Bool("anon", false, "process anonymous fields") + types = make(mapValue) + consts = make(mapValue) + imports = make(mapValue) + inputSubstr = make(mapValue) + outputSubstr = make(mapValue) ) // mapValue implements flag.Value. We use a mapValue flag instead of a regular @@ -166,7 +167,8 @@ func main() { flag.Var(types, "t", "rename type A to B when `A=B` is passed in. Multiple such mappings are allowed.") flag.Var(consts, "c", "reassign constant A to value B when `A=B` is passed in. Multiple such mappings are allowed.") flag.Var(imports, "import", "specifies the import libraries to use when types are not local. `name=path` specifies that 'name', used in types as name.type, refers to the package living in 'path'.") - flag.Var(substr, "s", "replace sub-string A with B when `A=B` is passed in. Multiple such mappings are allowed.") + flag.Var(inputSubstr, "in-substr", "replace input sub-string A with B when `A=B` is passed in. Multiple such mappings are allowed.") + flag.Var(outputSubstr, "out-substr", "replace output sub-string A with B when `A=B` is passed in. Multiple such mappings are allowed.") flag.Parse() if *input == "" || *output == "" { @@ -176,7 +178,15 @@ func main() { // Parse the input file. fset := token.NewFileSet() - f, err := parser.ParseFile(fset, *input, nil, parser.ParseComments|parser.DeclarationErrors|parser.SpuriousErrors) + inputBytes, err := os.ReadFile(*input) + if err != nil { + fmt.Fprintf(os.Stderr, "%v\n", err) + os.Exit(1) + } + for old, new := range inputSubstr { + inputBytes = bytes.ReplaceAll(inputBytes, []byte(old), []byte(new)) + } + f, err := parser.ParseFile(fset, *input, inputBytes, parser.ParseComments|parser.DeclarationErrors|parser.SpuriousErrors) if err != nil { fmt.Fprintf(os.Stderr, "%v\n", err) os.Exit(1) @@ -282,7 +292,7 @@ func main() { } byteBuf := buf.Bytes() - for old, new := range substr { + for old, new := range outputSubstr { byteBuf = bytes.ReplaceAll(byteBuf, []byte(old), []byte(new)) }