Merge release-20210628.0-19-g16b751b6c (automated)

6 files changed, 131 insertions, 95 deletions

diff --git a/pkg/sentry/fs/dirent.go b/pkg/sentry/fs/dirent.go
index 3a45e9041..8d7660e79 100644
--- a/pkg/sentry/fs/dirent.go
+++ b/pkg/sentry/fs/dirent.go
@@ -488,11 +488,11 @@ func (d *Dirent) walk(ctx context.Context, root *Dirent, name string, walkMayUnl
 	// Slow path: load the InodeOperations into memory. Since this is a hot path and the lookup may be
 	// expensive, if possible release the lock and re-acquire it.
 	if walkMayUnlock {
-		d.mu.Unlock()
+		d.mu.Unlock() // +checklocksforce: results in an inconsistent block.
 	}
 	c, err := d.Inode.Lookup(ctx, name)
 	if walkMayUnlock {
-		d.mu.Lock()
+		d.mu.Lock() // +checklocksforce: see above.
 	}
 	// No dice.
 	if err != nil {
@@ -594,21 +594,27 @@ func (d *Dirent) exists(ctx context.Context, root *Dirent, name string) bool {
 
 // lockDirectory should be called for any operation that changes this `d`s
 // children (creating or removing them).
-func (d *Dirent) lockDirectory() func() {
+// +checklocksacquire:d.dirMu
+// +checklocksacquire:d.mu
+func (d *Dirent) lockDirectory() {
 	renameMu.RLock()
 	d.dirMu.Lock()
 	d.mu.Lock()
-	return func() {
-		d.mu.Unlock()
-		d.dirMu.Unlock()
-		renameMu.RUnlock()
-	}
+}
+
+// unlockDirectory is the reverse of lockDirectory.
+// +checklocksrelease:d.dirMu
+// +checklocksrelease:d.mu
+func (d *Dirent) unlockDirectory() {
+	d.mu.Unlock()
+	d.dirMu.Unlock()
+	renameMu.RUnlock() // +checklocksforce: see lockDirectory.
 }
 
 // Create creates a new regular file in this directory.
 func (d *Dirent) Create(ctx context.Context, root *Dirent, name string, flags FileFlags, perms FilePermissions) (*File, error) {
-	unlock := d.lockDirectory()
-	defer unlock()
+	d.lockDirectory()
+	defer d.unlockDirectory()
 
 	// Does something already exist?
 	if d.exists(ctx, root, name) {
@@ -670,8 +676,8 @@ func (d *Dirent) finishCreate(ctx context.Context, child *Dirent, name string) {
 // genericCreate executes create if name does not exist. Removes a negative Dirent at name if
 // create succeeds.
 func (d *Dirent) genericCreate(ctx context.Context, root *Dirent, name string, create func() error) error {
-	unlock := d.lockDirectory()
-	defer unlock()
+	d.lockDirectory()
+	defer d.unlockDirectory()
 
 	// Does something already exist?
 	if d.exists(ctx, root, name) {
@@ -1021,8 +1027,8 @@ func (d *Dirent) Remove(ctx context.Context, root *Dirent, name string, dirPath
 		panic("Dirent.Remove: root must not be nil")
 	}
 
-	unlock := d.lockDirectory()
-	defer unlock()
+	d.lockDirectory()
+	defer d.unlockDirectory()
 
 	// Try to walk to the node.
 	child, err := d.walk(ctx, root, name, false /* may unlock */)
@@ -1082,8 +1088,8 @@ func (d *Dirent) RemoveDirectory(ctx context.Context, root *Dirent, name string)
 		panic("Dirent.Remove: root must not be nil")
 	}
 
-	unlock := d.lockDirectory()
-	defer unlock()
+	d.lockDirectory()
+	defer d.unlockDirectory()
 
 	// Check for dots.
 	if name == "." {
@@ -1259,17 +1265,15 @@ func (d *Dirent) dropExtendedReference() {
 	d.Inode.MountSource.fscache.Remove(d)
 }
 
-// lockForRename takes locks on oldParent and newParent as required by Rename
-// and returns a function that will unlock the locks taken. The returned
-// function must be called even if a non-nil error is returned.
-func lockForRename(oldParent *Dirent, oldName string, newParent *Dirent, newName string) (func(), error) {
+// lockForRename takes locks on oldParent and newParent as required by Rename.
+// On return, unlockForRename must always be called, even with an error.
+// +checklocksacquire:oldParent.mu
+// +checklocksacquire:newParent.mu
+func lockForRename(oldParent *Dirent, oldName string, newParent *Dirent, newName string) error {
 	renameMu.Lock()
 	if oldParent == newParent {
 		oldParent.mu.Lock()
-		return func() {
-			oldParent.mu.Unlock()
-			renameMu.Unlock()
-		}, nil
+		return nil // +checklocksforce: only one lock exists.
 	}
 
 	// Renaming between directories is a bit subtle:
@@ -1297,11 +1301,7 @@ func lockForRename(oldParent *Dirent, oldName string, newParent *Dirent, newName
 				// itself.
 				err = unix.EINVAL
 			}
-			return func() {
-				newParent.mu.Unlock()
-				oldParent.mu.Unlock()
-				renameMu.Unlock()
-			}, err
+			return err
 		}
 		child = p
 	}
@@ -1310,11 +1310,21 @@ func lockForRename(oldParent *Dirent, oldName string, newParent *Dirent, newName
 	// have no relationship; in either case we can do this:
 	newParent.mu.Lock()
 	oldParent.mu.Lock()
-	return func() {
+	return nil
+}
+
+// unlockForRename is the opposite of lockForRename.
+// +checklocksrelease:oldParent.mu
+// +checklocksrelease:newParent.mu
+func unlockForRename(oldParent, newParent *Dirent) {
+	if oldParent == newParent {
 		oldParent.mu.Unlock()
-		newParent.mu.Unlock()
-		renameMu.Unlock()
-	}, nil
+		renameMu.Unlock() // +checklocksforce: only one lock exists.
+		return
+	}
+	newParent.mu.Unlock()
+	oldParent.mu.Unlock()
+	renameMu.Unlock() // +checklocksforce: not tracked.
 }
 
 func (d *Dirent) checkSticky(ctx context.Context, victim *Dirent) error {
@@ -1353,8 +1363,8 @@ func (d *Dirent) MayDelete(ctx context.Context, root *Dirent, name string) error
 		return err
 	}
 
-	unlock := d.lockDirectory()
-	defer unlock()
+	d.lockDirectory()
+	defer d.unlockDirectory()
 
 	victim, err := d.walk(ctx, root, name, true /* may unlock */)
 	if err != nil {
@@ -1392,8 +1402,8 @@ func Rename(ctx context.Context, root *Dirent, oldParent *Dirent, oldName string
 	}
 
 	// Acquire global renameMu lock, and mu locks on oldParent/newParent.
-	unlock, err := lockForRename(oldParent, oldName, newParent, newName)
-	defer unlock()
+	err := lockForRename(oldParent, oldName, newParent, newName)
+	defer unlockForRename(oldParent, newParent)
 	if err != nil {
 		return err
 	}
diff --git a/pkg/sentry/fs/fs.go b/pkg/sentry/fs/fs.go
index 44587bb37..a346c316b 100644
--- a/pkg/sentry/fs/fs.go
+++ b/pkg/sentry/fs/fs.go
@@ -80,23 +80,33 @@ func AsyncBarrier() {
 // Async executes a function asynchronously.
 //
 // Async must not be called recursively.
+// +checklocksignore
 func Async(f func()) {
 	workMu.RLock()
-	go func() { // S/R-SAFE: AsyncBarrier must be called.
-		defer workMu.RUnlock() // Ensure RUnlock in case of panic.
-		f()
-	}()
+	go asyncWork(f) // S/R-SAFE: AsyncBarrier must be called.
+}
+
+// +checklocksignore
+func asyncWork(f func()) {
+	// Ensure RUnlock in case of panic.
+	defer workMu.RUnlock()
+	f()
 }
 
 // AsyncWithContext is just like Async, except that it calls the asynchronous
 // function with the given context as argument. This function exists to avoid
 // needing to allocate an extra function on the heap in a hot path.
+// +checklocksignore
 func AsyncWithContext(ctx context.Context, f func(context.Context)) {
 	workMu.RLock()
-	go func() { // S/R-SAFE: AsyncBarrier must be called.
-		defer workMu.RUnlock() // Ensure RUnlock in case of panic.
-		f(ctx)
-	}()
+	go asyncWorkWithContext(ctx, f)
+}
+
+// +checklocksignore
+func asyncWorkWithContext(ctx context.Context, f func(context.Context)) {
+	// Ensure RUnlock in case of panic.
+	defer workMu.RUnlock()
+	f(ctx)
 }
 
 // AsyncErrorBarrier waits for all outstanding asynchronous work to complete, or
diff --git a/pkg/sentry/fs/gofer/inode_state.go b/pkg/sentry/fs/gofer/inode_state.go
index e2af1d2ae..19f91f010 100644
--- a/pkg/sentry/fs/gofer/inode_state.go
+++ b/pkg/sentry/fs/gofer/inode_state.go
@@ -112,13 +112,6 @@ func (i *inodeFileState) loadLoading(_ struct{}) {
 // +checklocks:i.loading
 func (i *inodeFileState) afterLoad() {
 	load := func() (err error) {
-		// See comment on i.loading().
-		defer func() {
-			if err == nil {
-				i.loading.Unlock()
-			}
-		}()
-
 		// Manually restore the p9.File.
 		name, ok := i.s.inodeMappings[i.sattr.InodeID]
 		if !ok {
@@ -167,6 +160,9 @@ func (i *inodeFileState) afterLoad() {
 			i.savedUAttr = nil
 		}
 
+		// See comment on i.loading(). This only unlocks on the
+		// non-error path.
+		i.loading.Unlock() // +checklocksforce: per comment.
 		return nil
 	}
 
diff --git a/pkg/sentry/fs/gofer/path.go b/pkg/sentry/fs/gofer/path.go
index aa2405f68..958f46bd6 100644
--- a/pkg/sentry/fs/gofer/path.go
+++ b/pkg/sentry/fs/gofer/path.go
@@ -47,7 +47,8 @@ func (i *inodeOperations) Lookup(ctx context.Context, dir *fs.Inode, name string
 		return nil, linuxerr.ENAMETOOLONG
 	}
 
-	cp := i.session().cachePolicy
+	s := i.session()
+	cp := s.cachePolicy
 	if cp.cacheReaddir() {
 		// Check to see if we have readdirCache that indicates the
 		// child does not exist.  Avoid holding readdirMu longer than
@@ -78,7 +79,7 @@ func (i *inodeOperations) Lookup(ctx context.Context, dir *fs.Inode, name string
 		return nil, err
 	}
 
-	if i.session().overrides != nil {
+	if s.overrides != nil {
 		// Check if file belongs to a internal named pipe. Note that it doesn't need
 		// to check for sockets because it's done in newInodeOperations below.
 		deviceKey := device.MultiDeviceKey{
@@ -86,13 +87,13 @@ func (i *inodeOperations) Lookup(ctx context.Context, dir *fs.Inode, name string
 			SecondaryDevice: i.session().connID,
 			Inode:           qids[0].Path,
 		}
-		unlock := i.session().overrides.lock()
-		if pipeInode := i.session().overrides.getPipe(deviceKey); pipeInode != nil {
-			unlock()
+		s.overrides.lock()
+		if pipeInode := s.overrides.getPipe(deviceKey); pipeInode != nil {
+			s.overrides.unlock()
 			pipeInode.IncRef()
 			return fs.NewDirent(ctx, pipeInode, name), nil
 		}
-		unlock()
+		s.overrides.unlock()
 	}
 
 	// Construct the Inode operations.
@@ -221,17 +222,20 @@ func (i *inodeOperations) CreateHardLink(ctx context.Context, inode *fs.Inode, t
 	if err := i.fileState.file.link(ctx, &targetOpts.fileState.file, newName); err != nil {
 		return err
 	}
-	if i.session().cachePolicy.cacheUAttrs(inode) {
+
+	s := i.session()
+	if s.cachePolicy.cacheUAttrs(inode) {
 		// Increase link count.
 		targetOpts.cachingInodeOps.IncLinks(ctx)
 	}
+
 	i.touchModificationAndStatusChangeTime(ctx, inode)
 	return nil
 }
 
 // CreateDirectory uses Create to create a directory named s under inodeOperations.
-func (i *inodeOperations) CreateDirectory(ctx context.Context, dir *fs.Inode, s string, perm fs.FilePermissions) error {
-	if len(s) > maxFilenameLen {
+func (i *inodeOperations) CreateDirectory(ctx context.Context, dir *fs.Inode, name string, perm fs.FilePermissions) error {
+	if len(name) > maxFilenameLen {
 		return linuxerr.ENAMETOOLONG
 	}
 
@@ -247,16 +251,18 @@ func (i *inodeOperations) CreateDirectory(ctx context.Context, dir *fs.Inode, s
 		perm.SetGID = true
 	}
 
-	if _, err := i.fileState.file.mkdir(ctx, s, p9.FileMode(perm.LinuxMode()), p9.UID(owner.UID), p9.GID(owner.GID)); err != nil {
+	if _, err := i.fileState.file.mkdir(ctx, name, p9.FileMode(perm.LinuxMode()), p9.UID(owner.UID), p9.GID(owner.GID)); err != nil {
 		return err
 	}
-	if i.session().cachePolicy.cacheUAttrs(dir) {
+
+	s := i.session()
+	if s.cachePolicy.cacheUAttrs(dir) {
 		// Increase link count.
 		//
 		// N.B. This will update the modification time.
 		i.cachingInodeOps.IncLinks(ctx)
 	}
-	if i.session().cachePolicy.cacheReaddir() {
+	if s.cachePolicy.cacheReaddir() {
 		// Invalidate readdir cache.
 		i.markDirectoryDirty()
 	}
@@ -269,13 +275,14 @@ func (i *inodeOperations) Bind(ctx context.Context, dir *fs.Inode, name string,
 		return nil, linuxerr.ENAMETOOLONG
 	}
 
-	if i.session().overrides == nil {
+	s := i.session()
+	if s.overrides == nil {
 		return nil, syserror.EOPNOTSUPP
 	}
 
 	// Stabilize the override map while creation is in progress.
-	unlock := i.session().overrides.lock()
-	defer unlock()
+	s.overrides.lock()
+	defer s.overrides.unlock()
 
 	sattr, iops, err := i.createEndpointFile(ctx, dir, name, perm, p9.ModeSocket)
 	if err != nil {
@@ -284,7 +291,7 @@ func (i *inodeOperations) Bind(ctx context.Context, dir *fs.Inode, name string,
 
 	// Construct the positive Dirent.
 	childDir := fs.NewDirent(ctx, fs.NewInode(ctx, iops, dir.MountSource, sattr), name)
-	i.session().overrides.addBoundEndpoint(iops.fileState.key, childDir, ep)
+	s.overrides.addBoundEndpoint(iops.fileState.key, childDir, ep)
 	return childDir, nil
 }
 
@@ -298,8 +305,9 @@ func (i *inodeOperations) CreateFifo(ctx context.Context, dir *fs.Inode, name st
 	mode := p9.FileMode(perm.LinuxMode()) | p9.ModeNamedPipe
 
 	// N.B. FIFOs use major/minor numbers 0.
+	s := i.session()
 	if _, err := i.fileState.file.mknod(ctx, name, mode, 0, 0, p9.UID(owner.UID), p9.GID(owner.GID)); err != nil {
-		if i.session().overrides == nil || !linuxerr.Equals(linuxerr.EPERM, err) {
+		if s.overrides == nil || !linuxerr.Equals(linuxerr.EPERM, err) {
 			return err
 		}
 		// If gofer doesn't support mknod, check if we can create an internal fifo.
@@ -311,13 +319,14 @@ func (i *inodeOperations) CreateFifo(ctx context.Context, dir *fs.Inode, name st
 }
 
 func (i *inodeOperations) createInternalFifo(ctx context.Context, dir *fs.Inode, name string, owner fs.FileOwner, perm fs.FilePermissions) error {
-	if i.session().overrides == nil {
+	s := i.session()
+	if s.overrides == nil {
 		return linuxerr.EPERM
 	}
 
 	// Stabilize the override map while creation is in progress.
-	unlock := i.session().overrides.lock()
-	defer unlock()
+	s.overrides.lock()
+	defer s.overrides.unlock()
 
 	sattr, fileOps, err := i.createEndpointFile(ctx, dir, name, perm, p9.ModeNamedPipe)
 	if err != nil {
@@ -336,7 +345,7 @@ func (i *inodeOperations) createInternalFifo(ctx context.Context, dir *fs.Inode,
 
 	// Construct the positive Dirent.
 	childDir := fs.NewDirent(ctx, fs.NewInode(ctx, iops, dir.MountSource, sattr), name)
-	i.session().overrides.addPipe(fileOps.fileState.key, childDir, inode)
+	s.overrides.addPipe(fileOps.fileState.key, childDir, inode)
 	return nil
 }
 
@@ -386,8 +395,9 @@ func (i *inodeOperations) Remove(ctx context.Context, dir *fs.Inode, name string
 		return linuxerr.ENAMETOOLONG
 	}
 
+	s := i.session()
 	var key *device.MultiDeviceKey
-	if i.session().overrides != nil {
+	if s.overrides != nil {
 		// Find out if file being deleted is a socket or pipe that needs to be
 		// removed from endpoint map.
 		if d, err := i.Lookup(ctx, dir, name); err == nil {
@@ -402,8 +412,8 @@ func (i *inodeOperations) Remove(ctx context.Context, dir *fs.Inode, name string
 				}
 
 				// Stabilize the override map while deletion is in progress.
-				unlock := i.session().overrides.lock()
-				defer unlock()
+				s.overrides.lock()
+				defer s.overrides.unlock()
 			}
 		}
 	}
@@ -412,7 +422,7 @@ func (i *inodeOperations) Remove(ctx context.Context, dir *fs.Inode, name string
 		return err
 	}
 	if key != nil {
-		i.session().overrides.remove(ctx, *key)
+		s.overrides.remove(ctx, *key)
 	}
 	i.touchModificationAndStatusChangeTime(ctx, dir)
 
@@ -429,11 +439,13 @@ func (i *inodeOperations) RemoveDirectory(ctx context.Context, dir *fs.Inode, na
 	if err := i.fileState.file.unlinkAt(ctx, name, 0x200); err != nil {
 		return err
 	}
-	if i.session().cachePolicy.cacheUAttrs(dir) {
+
+	s := i.session()
+	if s.cachePolicy.cacheUAttrs(dir) {
 		// Decrease link count and updates atime.
 		i.cachingInodeOps.DecLinks(ctx)
 	}
-	if i.session().cachePolicy.cacheReaddir() {
+	if s.cachePolicy.cacheReaddir() {
 		// Invalidate readdir cache.
 		i.markDirectoryDirty()
 	}
@@ -463,12 +475,13 @@ func (i *inodeOperations) Rename(ctx context.Context, inode *fs.Inode, oldParent
 	}
 
 	// Is the renamed entity a directory? Fix link counts.
+	s := i.session()
 	if fs.IsDir(i.fileState.sattr) {
 		// Update cached state.
-		if i.session().cachePolicy.cacheUAttrs(oldParent) {
+		if s.cachePolicy.cacheUAttrs(oldParent) {
 			oldParentInodeOperations.cachingInodeOps.DecLinks(ctx)
 		}
-		if i.session().cachePolicy.cacheUAttrs(newParent) {
+		if s.cachePolicy.cacheUAttrs(newParent) {
 			// Only IncLinks if there is a new addition to
 			// newParent. If this is replacement, then the total
 			// count remains the same.
@@ -477,7 +490,7 @@ func (i *inodeOperations) Rename(ctx context.Context, inode *fs.Inode, oldParent
 			}
 		}
 	}
-	if i.session().cachePolicy.cacheReaddir() {
+	if s.cachePolicy.cacheReaddir() {
 		// Mark old directory dirty.
 		oldParentInodeOperations.markDirectoryDirty()
 		if oldParent != newParent {
@@ -487,17 +500,18 @@ func (i *inodeOperations) Rename(ctx context.Context, inode *fs.Inode, oldParent
 	}
 
 	// Rename always updates ctime.
-	if i.session().cachePolicy.cacheUAttrs(inode) {
+	if s.cachePolicy.cacheUAttrs(inode) {
 		i.cachingInodeOps.TouchStatusChangeTime(ctx)
 	}
 	return nil
 }
 
 func (i *inodeOperations) touchModificationAndStatusChangeTime(ctx context.Context, inode *fs.Inode) {
-	if i.session().cachePolicy.cacheUAttrs(inode) {
+	s := i.session()
+	if s.cachePolicy.cacheUAttrs(inode) {
 		i.cachingInodeOps.TouchModificationAndStatusChangeTime(ctx)
 	}
-	if i.session().cachePolicy.cacheReaddir() {
+	if s.cachePolicy.cacheReaddir() {
 		// Invalidate readdir cache.
 		i.markDirectoryDirty()
 	}
diff --git a/pkg/sentry/fs/gofer/session.go b/pkg/sentry/fs/gofer/session.go
index 7cf3522ff..b7debeecb 100644
--- a/pkg/sentry/fs/gofer/session.go
+++ b/pkg/sentry/fs/gofer/session.go
@@ -98,9 +98,14 @@ func (e *overrideMaps) remove(ctx context.Context, key device.MultiDeviceKey) {
 // lock blocks other addition and removal operations from happening while
 // the backing file is being created or deleted. Returns a function that unlocks
 // the endpoint map.
-func (e *overrideMaps) lock() func() {
+// +checklocksacquire:e.mu
+func (e *overrideMaps) lock() {
 	e.mu.Lock()
-	return func() { e.mu.Unlock() }
+}
+
+// +checklocksrelease:e.mu
+func (e *overrideMaps) unlock() {
+	e.mu.Unlock()
 }
 
 // getBoundEndpoint returns the bound endpoint mapped to the given key.
@@ -366,8 +371,8 @@ func newOverrideMaps() *overrideMaps {
 
 // fillKeyMap populates key and dirent maps upon restore from saved pathmap.
 func (s *session) fillKeyMap(ctx context.Context) error {
-	unlock := s.overrides.lock()
-	defer unlock()
+	s.overrides.lock()
+	defer s.overrides.unlock()
 
 	for ep, dirPath := range s.overrides.pathMap {
 		_, file, err := s.attach.walk(ctx, splitAbsolutePath(dirPath))
@@ -394,8 +399,8 @@ func (s *session) fillKeyMap(ctx context.Context) error {
 // fillPathMap populates paths for overrides from dirents in direntMap
 // before save.
 func (s *session) fillPathMap(ctx context.Context) error {
-	unlock := s.overrides.lock()
-	defer unlock()
+	s.overrides.lock()
+	defer s.overrides.unlock()
 
 	for _, endpoint := range s.overrides.keyMap {
 		mountRoot := endpoint.dirent.MountRoot()
diff --git a/pkg/sentry/fs/gofer/socket.go b/pkg/sentry/fs/gofer/socket.go
index 8a1c69ac2..1fd8a0910 100644
--- a/pkg/sentry/fs/gofer/socket.go
+++ b/pkg/sentry/fs/gofer/socket.go
@@ -32,10 +32,11 @@ func (i *inodeOperations) BoundEndpoint(inode *fs.Inode, path string) transport.
 		return nil
 	}
 
-	if i.session().overrides != nil {
-		unlock := i.session().overrides.lock()
-		defer unlock()
-		ep := i.session().overrides.getBoundEndpoint(i.fileState.key)
+	s := i.session()
+	if s.overrides != nil {
+		s.overrides.lock()
+		defer s.overrides.unlock()
+		ep := s.overrides.getBoundEndpoint(i.fileState.key)
 		if ep != nil {
 			return ep
 		}