diff options
author | Nicolas Lacasse <nlacasse@google.com> | 2021-02-19 17:36:44 -0800 |
---|---|---|
committer | gVisor bot <gvisor-bot@google.com> | 2021-02-19 17:38:51 -0800 |
commit | 93fc09248a2fa8b840d8ce47800414980d74bdb0 (patch) | |
tree | 6006fc6b336315a4ae272782b8afb5c1130a9b2b | |
parent | 7544eeb242d0aba2da054a1663e043feaedb9618 (diff) |
Don't hold baseEndpoint.mu while calling EventUpdate().
This removes a three-lock deadlock between fdnotifier.notifier.mu,
epoll.EventPoll.listsMu, and baseEndpoint.mu.
A lock order comment was added to epoll/epoll.go.
Also fix unsafe access of baseEndpoint.connected/receiver.
PiperOrigin-RevId: 358515191
-rw-r--r-- | pkg/sentry/kernel/epoll/epoll.go | 6 | ||||
-rw-r--r-- | pkg/sentry/socket/unix/transport/unix.go | 47 | ||||
-rw-r--r-- | pkg/sentry/vfs/vfs.go | 4 |
3 files changed, 37 insertions, 20 deletions
diff --git a/pkg/sentry/kernel/epoll/epoll.go b/pkg/sentry/kernel/epoll/epoll.go index 61aeca044..407b6e917 100644 --- a/pkg/sentry/kernel/epoll/epoll.go +++ b/pkg/sentry/kernel/epoll/epoll.go @@ -14,6 +14,12 @@ // Package epoll provides an implementation of Linux's IO event notification // facility. See epoll(7) for more details. +// +// Lock order: +// EventPoll.mu +// fdnotifier.notifier.mu +// EventPoll.listsMu +// unix.baseEndpoint.Mutex package epoll import ( diff --git a/pkg/sentry/socket/unix/transport/unix.go b/pkg/sentry/socket/unix/transport/unix.go index ceada54a8..359a5995b 100644 --- a/pkg/sentry/socket/unix/transport/unix.go +++ b/pkg/sentry/socket/unix/transport/unix.go @@ -742,8 +742,8 @@ func (e *connectedEndpoint) SetSendBufferSize(v int64) (newSz int64) { return v } -// baseEndpoint is an embeddable unix endpoint base used in both the connected and connectionless -// unix domain socket Endpoint implementations. +// baseEndpoint is an embeddable unix endpoint base used in both the connected +// and connectionless unix domain socket Endpoint implementations. // // Not to be used on its own. // @@ -753,6 +753,9 @@ type baseEndpoint struct { tcpip.DefaultSocketOptionsHandler // Mutex protects the below fields. + // + // See the lock ordering comment in package kernel/epoll regarding when + // this lock can safely be held. sync.Mutex `state:"nosave"` // receiver allows Messages to be received. @@ -774,20 +777,22 @@ type baseEndpoint struct { func (e *baseEndpoint) EventRegister(we *waiter.Entry, mask waiter.EventMask) { e.Queue.EventRegister(we, mask) e.Lock() - if e.connected != nil { - e.connected.EventUpdate() - } + c := e.connected e.Unlock() + if c != nil { + c.EventUpdate() + } } // EventUnregister implements waiter.Waitable.EventUnregister. func (e *baseEndpoint) EventUnregister(we *waiter.Entry) { e.Queue.EventUnregister(we) e.Lock() - if e.connected != nil { - e.connected.EventUpdate() - } + c := e.connected e.Unlock() + if c != nil { + c.EventUpdate() + } } // Passcred implements Credentialer.Passcred. @@ -942,22 +947,26 @@ func (e *baseEndpoint) Shutdown(flags tcpip.ShutdownFlags) *syserr.Error { return syserr.ErrNotConnected } - if flags&tcpip.ShutdownRead != 0 { - e.receiver.CloseRecv() + var ( + r = e.receiver + c = e.connected + shutdownRead = flags&tcpip.ShutdownRead != 0 + shutdownWrite = flags&tcpip.ShutdownWrite != 0 + ) + if shutdownRead { + r.CloseRecv() } - - if flags&tcpip.ShutdownWrite != 0 { - e.connected.CloseSend() + if shutdownWrite { + c.CloseSend() } - e.Unlock() - if flags&tcpip.ShutdownRead != 0 { - e.receiver.CloseNotify() + // Don't hold e.Mutex while calling CloseNotify. + if shutdownRead { + r.CloseNotify() } - - if flags&tcpip.ShutdownWrite != 0 { - e.connected.CloseNotify() + if shutdownWrite { + c.CloseNotify() } return nil diff --git a/pkg/sentry/vfs/vfs.go b/pkg/sentry/vfs/vfs.go index b0e13cdab..00f1847d8 100644 --- a/pkg/sentry/vfs/vfs.go +++ b/pkg/sentry/vfs/vfs.go @@ -23,7 +23,9 @@ // Dentry.mu // Locks acquired by FilesystemImpls between Prepare{Delete,Rename}Dentry and Commit{Delete,Rename*}Dentry // VirtualFilesystem.filesystemsMu -// EpollInstance.mu +// fdnotifier.notifier.mu +// EpollInstance.mu +// Locks acquired by FileDescriptionImpl.Readiness // Inotify.mu // Watches.mu // Inotify.evMu |