From aaaa3429f7851a35e5db8f367eedbd53f8df082d Mon Sep 17 00:00:00 2001 From: Andrei Vagin Date: Fri, 3 Jun 2022 13:00:02 -0700 Subject: [PATCH] fs: use lockdep mutexes PiperOrigin-RevId: 452826155 --- pkg/sentry/fs/BUILD | 26 ++++++++++++++++ pkg/sentry/fs/inotify.go | 5 ++- pkg/sentry/fs/mounts.go | 3 +- pkg/sentry/fsimpl/cgroupfs/BUILD | 18 +++++++++++ pkg/sentry/fsimpl/cgroupfs/cgroupfs.go | 3 +- pkg/sentry/fsimpl/cgroupfs/pids.go | 3 +- pkg/sentry/fsimpl/kernfs/BUILD | 18 +++++++++++ pkg/sentry/fsimpl/kernfs/kernfs.go | 4 +-- pkg/sentry/vfs/BUILD | 42 ++++++++++++++++++++++++++ pkg/sentry/vfs/epoll.go | 2 +- pkg/sentry/vfs/file_description.go | 2 +- pkg/sentry/vfs/inotify.go | 4 +-- pkg/sentry/vfs/vfs.go | 2 +- 13 files changed, 116 insertions(+), 16 deletions(-) diff --git a/pkg/sentry/fs/BUILD b/pkg/sentry/fs/BUILD index ae1d36c29..ada184c5b 100644 --- a/pkg/sentry/fs/BUILD +++ b/pkg/sentry/fs/BUILD @@ -1,5 +1,6 @@ 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"]) @@ -29,11 +30,14 @@ go_library( "inode_overlay.go", "inotify.go", "inotify_event.go", + "inotify_event_mutex.go", + "inotify_mutex.go", "inotify_watch.go", "mock.go", "mount.go", "mount_overlay.go", "mounts.go", + "namespace_mutex.go", "offset.go", "overlay.go", "path.go", @@ -68,6 +72,7 @@ go_library( "//pkg/sentry/usage", "//pkg/state", "//pkg/sync", + "//pkg/sync/locking", "//pkg/usermem", "//pkg/waiter", "@org_golang_x_sys//unix:go_default_library", @@ -98,6 +103,27 @@ go_template_instance( }, ) +declare_mutex( + name = "namespace_mutex", + out = "namespace_mutex.go", + package = "fs", + prefix = "namespace", +) + +declare_mutex( + name = "inotify_event_mutex", + out = "inotify_event_mutex.go", + package = "fs", + prefix = "inotifyEvent", +) + +declare_mutex( + name = "inotify_mutex", + out = "inotify_mutex.go", + package = "fs", + prefix = "inotify", +) + go_test( name = "fs_x_test", size = "small", diff --git a/pkg/sentry/fs/inotify.go b/pkg/sentry/fs/inotify.go index 6cd94db3f..e37c3ba9e 100644 --- a/pkg/sentry/fs/inotify.go +++ b/pkg/sentry/fs/inotify.go @@ -25,7 +25,6 @@ import ( "gvisor.dev/gvisor/pkg/sentry/arch" "gvisor.dev/gvisor/pkg/sentry/memmap" "gvisor.dev/gvisor/pkg/sentry/uniqueid" - "gvisor.dev/gvisor/pkg/sync" "gvisor.dev/gvisor/pkg/usermem" "gvisor.dev/gvisor/pkg/waiter" ) @@ -50,7 +49,7 @@ type Inotify struct { // while queuing events, a watch needs to lock the event queue, and using mu // for that would violate lock ordering since at that point the calling // goroutine already holds Watch.target.Watches.mu. - evMu sync.Mutex `state:"nosave"` + evMu inotifyEventMutex `state:"nosave"` // A list of pending events for this inotify instance. Protected by evMu. events eventList @@ -60,7 +59,7 @@ type Inotify struct { scratch []byte // mu protects the fields below. - mu sync.Mutex `state:"nosave"` + mu inotifyMutex `state:"nosave"` // The next watch descriptor number to use for this inotify instance. Note // that Linux starts numbering watch descriptors from 1. diff --git a/pkg/sentry/fs/mounts.go b/pkg/sentry/fs/mounts.go index 10146af4e..f1cc6a63b 100644 --- a/pkg/sentry/fs/mounts.go +++ b/pkg/sentry/fs/mounts.go @@ -23,7 +23,6 @@ import ( "gvisor.dev/gvisor/pkg/errors/linuxerr" "gvisor.dev/gvisor/pkg/refs" "gvisor.dev/gvisor/pkg/sentry/kernel/auth" - "gvisor.dev/gvisor/pkg/sync" ) // DefaultTraversalLimit provides a sensible default traversal limit that may @@ -151,7 +150,7 @@ type MountNamespace struct { root *Dirent // mu protects mounts and mountID counter. - mu sync.Mutex `state:"nosave"` + mu namespaceMutex `state:"nosave"` // mounts is a map of mounted Dirent -> Mount object. There are three // possible cases: diff --git a/pkg/sentry/fsimpl/cgroupfs/BUILD b/pkg/sentry/fsimpl/cgroupfs/BUILD index 336b7ff18..3018d1ad0 100644 --- a/pkg/sentry/fsimpl/cgroupfs/BUILD +++ b/pkg/sentry/fsimpl/cgroupfs/BUILD @@ -1,8 +1,23 @@ 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", "declare_rwmutex") licenses(["notice"]) +declare_mutex( + name = "pids_controller_mutex", + out = "pids_controller_mutex.go", + package = "cgroupfs", + prefix = "pidsController", +) + +declare_rwmutex( + name = "task_mutex", + out = "task_mutex.go", + package = "cgroupfs", + prefix = "task", +) + go_template_instance( name = "dir_refs", out = "dir_refs.go", @@ -27,6 +42,8 @@ go_library( "job.go", "memory.go", "pids.go", + "pids_controller_mutex.go", + "task_mutex.go", ], visibility = ["//pkg/sentry:internal"], deps = [ @@ -49,6 +66,7 @@ go_library( "//pkg/sentry/usage", "//pkg/sentry/vfs", "//pkg/sync", + "//pkg/sync/locking", "//pkg/usermem", ], ) diff --git a/pkg/sentry/fsimpl/cgroupfs/cgroupfs.go b/pkg/sentry/fsimpl/cgroupfs/cgroupfs.go index 2c216003d..dc993fb3b 100644 --- a/pkg/sentry/fsimpl/cgroupfs/cgroupfs.go +++ b/pkg/sentry/fsimpl/cgroupfs/cgroupfs.go @@ -72,7 +72,6 @@ import ( "gvisor.dev/gvisor/pkg/sentry/kernel" "gvisor.dev/gvisor/pkg/sentry/kernel/auth" "gvisor.dev/gvisor/pkg/sentry/vfs" - "gvisor.dev/gvisor/pkg/sync" "gvisor.dev/gvisor/pkg/usermem" ) @@ -173,7 +172,7 @@ type filesystem struct { // tasksMu serializes task membership changes across all cgroups within a // filesystem. - tasksMu sync.RWMutex `state:"nosave"` + tasksMu taskRWMutex `state:"nosave"` } // InitializeHierarchyID implements kernel.cgroupFS.InitializeHierarchyID. diff --git a/pkg/sentry/fsimpl/cgroupfs/pids.go b/pkg/sentry/fsimpl/cgroupfs/pids.go index 2e2ace7ff..ea17a45fd 100644 --- a/pkg/sentry/fsimpl/cgroupfs/pids.go +++ b/pkg/sentry/fsimpl/cgroupfs/pids.go @@ -27,7 +27,6 @@ import ( "gvisor.dev/gvisor/pkg/sentry/kernel" "gvisor.dev/gvisor/pkg/sentry/kernel/auth" "gvisor.dev/gvisor/pkg/sentry/vfs" - "gvisor.dev/gvisor/pkg/sync" "gvisor.dev/gvisor/pkg/usermem" ) @@ -61,7 +60,7 @@ type pidsController struct { isRoot bool // mu protects the fields below. - mu sync.Mutex `state:"nosave"` + mu pidsControllerMutex `state:"nosave"` // pendingTotal and pendingPool tracks the charge for processes starting // up. During startup, we check if PIDs are available by charging the diff --git a/pkg/sentry/fsimpl/kernfs/BUILD b/pkg/sentry/fsimpl/kernfs/BUILD index 2574e9c9f..c6f4cac8e 100644 --- a/pkg/sentry/fsimpl/kernfs/BUILD +++ b/pkg/sentry/fsimpl/kernfs/BUILD @@ -1,5 +1,6 @@ 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", "declare_rwmutex") licenses(["notice"]) @@ -82,13 +83,29 @@ go_template_instance( }, ) +declare_rwmutex( + name = "filesystem_mutex", + out = "filesystem_mutex.go", + package = "kernfs", + prefix = "filesystem", +) + +declare_mutex( + name = "deferred_dec_refs_mutex", + out = "deferred_dec_refs_mutex.go", + package = "kernfs", + prefix = "deferredDecRefs", +) + go_library( name = "kernfs", srcs = [ + "deferred_dec_refs_mutex.go", "dentry_list.go", "dynamic_bytes_file.go", "fd_impl_util.go", "filesystem.go", + "filesystem_mutex.go", "fstree.go", "inode_impl_util.go", "kernfs.go", @@ -120,6 +137,7 @@ go_library( "//pkg/sentry/socket/unix/transport", "//pkg/sentry/vfs", "//pkg/sync", + "//pkg/sync/locking", "//pkg/usermem", ], ) diff --git a/pkg/sentry/fsimpl/kernfs/kernfs.go b/pkg/sentry/fsimpl/kernfs/kernfs.go index 2f07a65cd..4c0a954c9 100644 --- a/pkg/sentry/fsimpl/kernfs/kernfs.go +++ b/pkg/sentry/fsimpl/kernfs/kernfs.go @@ -80,7 +80,7 @@ import ( type Filesystem struct { vfsfs vfs.Filesystem - deferredDecRefsMu sync.Mutex `state:"nosave"` + deferredDecRefsMu deferredDecRefsMutex `state:"nosave"` // deferredDecRefs is a list of dentries waiting to be DecRef()ed. This is // used to defer dentry destruction until mu can be acquired for @@ -108,7 +108,7 @@ type Filesystem struct { // defer fs.mu.RUnlock() // ... // fs.deferDecRef(dentry) - mu sync.RWMutex `state:"nosave"` + mu filesystemRWMutex `state:"nosave"` // nextInoMinusOne is used to to allocate inode numbers on this // filesystem. Must be accessed by atomic operations. diff --git a/pkg/sentry/vfs/BUILD b/pkg/sentry/vfs/BUILD index 7bac5b13c..f0b51ef31 100644 --- a/pkg/sentry/vfs/BUILD +++ b/pkg/sentry/vfs/BUILD @@ -1,8 +1,44 @@ 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") licenses(["notice"]) +declare_mutex( + name = "virtual_filesystem_mutex", + out = "virtual_filesystem_mutex.go", + package = "vfs", + prefix = "virtualFilesystem", +) + +declare_mutex( + name = "inotify_event_mutex", + out = "inotify_event_mutex.go", + package = "vfs", + prefix = "inotifyEvent", +) + +declare_mutex( + name = "inotify_mutex", + out = "inotify_mutex.go", + package = "vfs", + prefix = "inotify", +) + +declare_mutex( + name = "epoll_instance_mutex", + out = "epoll_instance_mutex.go", + package = "vfs", + prefix = "epollReadyInstance", +) + +declare_mutex( + name = "epoll_mutex", + out = "epoll_mutex.go", + package = "vfs", + prefix = "epoll", +) + go_template_instance( name = "epoll_interest_list", out = "epoll_interest_list.go", @@ -69,7 +105,9 @@ go_library( "dentry.go", "device.go", "epoll.go", + "epoll_instance_mutex.go", "epoll_interest_list.go", + "epoll_mutex.go", "event_list.go", "file_description.go", "file_description_impl_util.go", @@ -79,6 +117,8 @@ go_library( "filesystem_refs.go", "filesystem_type.go", "inotify.go", + "inotify_event_mutex.go", + "inotify_mutex.go", "lock.go", "mount.go", "mount_namespace_refs.go", @@ -90,6 +130,7 @@ go_library( "resolving_path.go", "save_restore.go", "vfs.go", + "virtual_filesystem_mutex.go", ], visibility = ["//pkg/sentry:internal"], deps = [ @@ -117,6 +158,7 @@ go_library( "//pkg/sentry/socket/unix/transport", "//pkg/sentry/uniqueid", "//pkg/sync", + "//pkg/sync/locking", "//pkg/usermem", "//pkg/waiter", "@org_golang_x_sys//unix:go_default_library", diff --git a/pkg/sentry/vfs/epoll.go b/pkg/sentry/vfs/epoll.go index dd885e579..41ef0fe3b 100644 --- a/pkg/sentry/vfs/epoll.go +++ b/pkg/sentry/vfs/epoll.go @@ -50,7 +50,7 @@ type EpollInstance struct { // readyMu protects ready, readySeq, epollInterest.ready, and // epollInterest.epollInterestEntry. ready is analogous to Linux's struct // eventpoll::lock. - readyMu sync.Mutex `state:"nosave"` + readyMu epollReadyInstanceMutex `state:"nosave"` // ready is the set of file descriptors that may be "ready" for I/O. Note // that this must be an ordered list, not a map: "If more than maxevents diff --git a/pkg/sentry/vfs/file_description.go b/pkg/sentry/vfs/file_description.go index ccfbd722f..c210a52b8 100644 --- a/pkg/sentry/vfs/file_description.go +++ b/pkg/sentry/vfs/file_description.go @@ -60,7 +60,7 @@ type FileDescription struct { // epolls is the set of epollInterests registered for this FileDescription. // epolls is protected by epollMu. - epollMu sync.Mutex `state:"nosave"` + epollMu epollMutex `state:"nosave"` epolls map[*epollInterest]struct{} // vd is the filesystem location at which this FileDescription was opened. diff --git a/pkg/sentry/vfs/inotify.go b/pkg/sentry/vfs/inotify.go index 28b51538d..2a9dda9d5 100644 --- a/pkg/sentry/vfs/inotify.go +++ b/pkg/sentry/vfs/inotify.go @@ -71,7 +71,7 @@ type Inotify struct { // evMu *only* protects the events list. We need a separate lock while // queuing events: using mu may violate lock ordering, since at that point // the calling goroutine may already hold Watches.mu. - evMu sync.Mutex `state:"nosave"` + evMu inotifyEventMutex `state:"nosave"` // A list of pending events for this inotify instance. Protected by evMu. events eventList @@ -81,7 +81,7 @@ type Inotify struct { scratch []byte // mu protects the fields below. - mu sync.Mutex `state:"nosave"` + mu inotifyMutex `state:"nosave"` // nextWatchMinusOne is used to allocate watch descriptors on this Inotify // instance. Note that Linux starts numbering watch descriptors from 1. diff --git a/pkg/sentry/vfs/vfs.go b/pkg/sentry/vfs/vfs.go index acf03fb27..3921d4acf 100644 --- a/pkg/sentry/vfs/vfs.go +++ b/pkg/sentry/vfs/vfs.go @@ -60,7 +60,7 @@ type VirtualFilesystem struct { // mountMu serializes mount mutations. // // mountMu is analogous to Linux's namespace_sem. - mountMu sync.Mutex `state:"nosave"` + mountMu virtualFilesystemMutex `state:"nosave"` // mounts maps (mount parent, mount point) pairs to mounts. (Since mounts // are uniquely namespaced, including mount parent in the key correctly