Merge release-20210628.0-19-g16b751b6c (automated)

7 files changed, 34 insertions, 22 deletions

diff --git a/pkg/sentry/fsimpl/gofer/filesystem.go b/pkg/sentry/fsimpl/gofer/filesystem.go
index 237d17921..652e5fe77 100644
--- a/pkg/sentry/fsimpl/gofer/filesystem.go
+++ b/pkg/sentry/fsimpl/gofer/filesystem.go
@@ -147,6 +147,7 @@ func putDentrySlice(ds *[]*dentry) {
 // but dentry slices are allocated lazily, and it's much easier to say "defer
 // fs.renameMuRUnlockAndCheckCaching(&ds)" than "defer func() {
 // fs.renameMuRUnlockAndCheckCaching(ds) }()" to work around this.
+// +checklocksrelease:fs.renameMu
 func (fs *filesystem) renameMuRUnlockAndCheckCaching(ctx context.Context, dsp **[]*dentry) {
 	fs.renameMu.RUnlock()
 	if *dsp == nil {
@@ -159,6 +160,7 @@ func (fs *filesystem) renameMuRUnlockAndCheckCaching(ctx context.Context, dsp **
 	putDentrySlice(*dsp)
 }
 
+// +checklocksrelease:fs.renameMu
 func (fs *filesystem) renameMuUnlockAndCheckCaching(ctx context.Context, ds **[]*dentry) {
 	if *ds == nil {
 		fs.renameMu.Unlock()
@@ -540,7 +542,7 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b
 			if child.syntheticChildren != 0 {
 				// This is definitely not an empty directory, irrespective of
 				// fs.opts.interop.
-				vfsObj.AbortDeleteDentry(&child.vfsd)
+				vfsObj.AbortDeleteDentry(&child.vfsd) // +checklocksforce: PrepareDeleteDentry called if child != nil.
 				return linuxerr.ENOTEMPTY
 			}
 			// If InteropModeShared is in effect and the first call to
@@ -550,12 +552,12 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b
 			// still exist) would be a waste of time.
 			if child.cachedMetadataAuthoritative() {
 				if !child.isDir() {
-					vfsObj.AbortDeleteDentry(&child.vfsd)
+					vfsObj.AbortDeleteDentry(&child.vfsd) // +checklocksforce: see above.
 					return syserror.ENOTDIR
 				}
 				for _, grandchild := range child.children {
 					if grandchild != nil {
-						vfsObj.AbortDeleteDentry(&child.vfsd)
+						vfsObj.AbortDeleteDentry(&child.vfsd) // +checklocksforce: see above.
 						return linuxerr.ENOTEMPTY
 					}
 				}
@@ -565,12 +567,12 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b
 	} else {
 		// child must be a non-directory file.
 		if child != nil && child.isDir() {
-			vfsObj.AbortDeleteDentry(&child.vfsd)
+			vfsObj.AbortDeleteDentry(&child.vfsd) // +checklocksforce: see above.
 			return syserror.EISDIR
 		}
 		if rp.MustBeDir() {
 			if child != nil {
-				vfsObj.AbortDeleteDentry(&child.vfsd)
+				vfsObj.AbortDeleteDentry(&child.vfsd) // +checklocksforce: see above.
 			}
 			return syserror.ENOTDIR
 		}
@@ -583,7 +585,7 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b
 		err = parent.file.unlinkAt(ctx, name, flags)
 		if err != nil {
 			if child != nil {
-				vfsObj.AbortDeleteDentry(&child.vfsd)
+				vfsObj.AbortDeleteDentry(&child.vfsd) // +checklocksforce: see above.
 			}
 			return err
 		}
@@ -601,7 +603,7 @@ func (fs *filesystem) unlinkAt(ctx context.Context, rp *vfs.ResolvingPath, dir b
 	}
 
 	if child != nil {
-		vfsObj.CommitDeleteDentry(ctx, &child.vfsd)
+		vfsObj.CommitDeleteDentry(ctx, &child.vfsd) // +checklocksforce: see above.
 		child.setDeleted()
 		if child.isSynthetic() {
 			parent.syntheticChildren--
diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go
index fe4c2e0e1..2f85215d9 100644
--- a/pkg/sentry/fsimpl/gofer/gofer.go
+++ b/pkg/sentry/fsimpl/gofer/gofer.go
@@ -947,10 +947,10 @@ func (d *dentry) cachedMetadataAuthoritative() bool {
 // updateFromP9Attrs is called to update d's metadata after an update from the
 // remote filesystem.
 // Precondition: d.metadataMu must be locked.
+// +checklocks:d.metadataMu
 func (d *dentry) updateFromP9AttrsLocked(mask p9.AttrMask, attr *p9.Attr) {
 	if mask.Mode {
 		if got, want := uint32(attr.Mode.FileType()), d.fileType(); got != want {
-			d.metadataMu.Unlock()
 			panic(fmt.Sprintf("gofer.dentry file type changed from %#o to %#o", want, got))
 		}
 		atomic.StoreUint32(&d.mode, uint32(attr.Mode))
@@ -989,6 +989,7 @@ func (d *dentry) updateFromP9AttrsLocked(mask p9.AttrMask, attr *p9.Attr) {
 
 // Preconditions: !d.isSynthetic().
 // Preconditions: d.metadataMu is locked.
+// +checklocks:d.metadataMu
 func (d *dentry) refreshSizeLocked(ctx context.Context) error {
 	d.handleMu.RLock()
 
@@ -1020,6 +1021,7 @@ func (d *dentry) updateFromGetattr(ctx context.Context) error {
 // Preconditions:
 // * !d.isSynthetic().
 // * d.metadataMu is locked.
+// +checklocks:d.metadataMu
 func (d *dentry) updateFromGetattrLocked(ctx context.Context) error {
 	// Use d.readFile or d.writeFile, which represent 9P FIDs that have been
 	// opened, in preference to d.file, which represents a 9P fid that has not.
@@ -1044,7 +1046,8 @@ func (d *dentry) updateFromGetattrLocked(ctx context.Context) error {
 
 	_, attrMask, attr, err := file.getAttr(ctx, dentryAttrMask())
 	if handleMuRLocked {
-		d.handleMu.RUnlock() // must be released before updateFromP9AttrsLocked()
+		// handleMu must be released before updateFromP9AttrsLocked().
+		d.handleMu.RUnlock() // +checklocksforce: complex case.
 	}
 	if err != nil {
 		return err
@@ -1470,7 +1473,7 @@ func (d *dentry) checkCachingLocked(ctx context.Context, renameMuWriteLocked boo
 		if d.isDeleted() {
 			d.watches.HandleDeletion(ctx)
 		}
-		d.destroyLocked(ctx)
+		d.destroyLocked(ctx) // +checklocksforce: renameMu must be acquired at this point.
 		return
 	}
 	// If d still has inotify watches and it is not deleted or invalidated, it
@@ -1498,7 +1501,7 @@ func (d *dentry) checkCachingLocked(ctx context.Context, renameMuWriteLocked boo
 			delete(d.parent.children, d.name)
 			d.parent.dirMu.Unlock()
 		}
-		d.destroyLocked(ctx)
+		d.destroyLocked(ctx) // +checklocksforce: see above.
 		return
 	}
 
@@ -1527,7 +1530,7 @@ func (d *dentry) checkCachingLocked(ctx context.Context, renameMuWriteLocked boo
 			d.fs.renameMu.Lock()
 			defer d.fs.renameMu.Unlock()
 		}
-		d.fs.evictCachedDentryLocked(ctx)
+		d.fs.evictCachedDentryLocked(ctx) // +checklocksforce: see above.
 	}
 }
 
@@ -1544,6 +1547,7 @@ func (d *dentry) removeFromCacheLocked() {
 
 // Precondition: fs.renameMu must be locked for writing; it may be temporarily
 // unlocked.
+// +checklocks:fs.renameMu
 func (fs *filesystem) evictAllCachedDentriesLocked(ctx context.Context) {
 	for fs.cachedDentriesLen != 0 {
 		fs.evictCachedDentryLocked(ctx)
@@ -1552,6 +1556,7 @@ func (fs *filesystem) evictAllCachedDentriesLocked(ctx context.Context) {
 
 // Preconditions:
 // * fs.renameMu must be locked for writing; it may be temporarily unlocked.
+// +checklocks:fs.renameMu
 func (fs *filesystem) evictCachedDentryLocked(ctx context.Context) {
 	fs.cacheMu.Lock()
 	victim := fs.cachedDentries.Back()
@@ -1588,7 +1593,7 @@ func (fs *filesystem) evictCachedDentryLocked(ctx context.Context) {
 	// will try to acquire fs.renameMu (which we have already acquired). Hence,
 	// fs.renameMu will synchronize the destroy attempts.
 	victim.cachingMu.Unlock()
-	victim.destroyLocked(ctx)
+	victim.destroyLocked(ctx) // +checklocksforce: owned as precondition, victim.fs == fs.
 }
 
 // destroyLocked destroys the dentry.
@@ -1598,6 +1603,7 @@ func (fs *filesystem) evictCachedDentryLocked(ctx context.Context) {
 // * d.refs == 0.
 // * d.parent.children[d.name] != d, i.e. d is not reachable by path traversal
 //   from its former parent dentry.
+// +checklocks:d.fs.renameMu
 func (d *dentry) destroyLocked(ctx context.Context) {
 	switch atomic.LoadInt64(&d.refs) {
 	case 0:
diff --git a/pkg/sentry/fsimpl/gofer/revalidate.go b/pkg/sentry/fsimpl/gofer/revalidate.go
index 8f81f0822..226790a11 100644
--- a/pkg/sentry/fsimpl/gofer/revalidate.go
+++ b/pkg/sentry/fsimpl/gofer/revalidate.go
@@ -247,16 +247,16 @@ func (fs *filesystem) revalidateHelper(ctx context.Context, vfsObj *vfs.VirtualF
 			if found && !d.isSynthetic() {
 				// First dentry is where the search is starting, just update attributes
 				// since it cannot be replaced.
-				d.updateFromP9AttrsLocked(stats[i].Valid, &stats[i].Attr)
+				d.updateFromP9AttrsLocked(stats[i].Valid, &stats[i].Attr) // +checklocksforce: acquired by lockAllMetadata.
 			}
-			d.metadataMu.Unlock()
+			d.metadataMu.Unlock() // +checklocksforce: see above.
 			continue
 		}
 
 		// Note that synthetic dentries will always fails the comparison check
 		// below.
 		if !found || d.qidPath != stats[i].QID.Path {
-			d.metadataMu.Unlock()
+			d.metadataMu.Unlock() // +checklocksforce: see above.
 			if !found && d.isSynthetic() {
 				// We have a synthetic file, and no remote file has arisen to replace
 				// it.
@@ -298,7 +298,7 @@ func (fs *filesystem) revalidateHelper(ctx context.Context, vfsObj *vfs.VirtualF
 		}
 
 		// The file at this path hasn't changed. Just update cached metadata.
-		d.updateFromP9AttrsLocked(stats[i].Valid, &stats[i].Attr)
+		d.updateFromP9AttrsLocked(stats[i].Valid, &stats[i].Attr) // +checklocksforce: see above.
 		d.metadataMu.Unlock()
 	}
 
@@ -354,6 +354,7 @@ func (r *revalidateState) add(name string, d *dentry) {
 	r.dentries = append(r.dentries, d)
 }
 
+// +checklocksignore
 func (r *revalidateState) lockAllMetadata() {
 	for _, d := range r.dentries {
 		d.metadataMu.Lock()
@@ -372,6 +373,7 @@ func (r *revalidateState) popFront() *dentry {
 
 // reset releases all metadata locks and resets all fields to allow this
 // instance to be reused.
+// +checklocksignore
 func (r *revalidateState) reset() {
 	if r.locked {
 		// Unlock any remaining dentries.
diff --git a/pkg/sentry/fsimpl/gofer/symlink.go b/pkg/sentry/fsimpl/gofer/symlink.go
index 2ec819f86..dbd834c67 100644
--- a/pkg/sentry/fsimpl/gofer/symlink.go
+++ b/pkg/sentry/fsimpl/gofer/symlink.go
@@ -41,7 +41,7 @@ func (d *dentry) readlink(ctx context.Context, mnt *vfs.Mount) (string, error) {
 			d.haveTarget = true
 			d.target = target
 		}
-		d.dataMu.Unlock()
+		d.dataMu.Unlock() // +checklocksforce: guaranteed locked from above.
 	}
 	return target, err
 }
diff --git a/pkg/sentry/fsimpl/kernfs/filesystem.go b/pkg/sentry/fsimpl/kernfs/filesystem.go
index 38c2b6df1..20d2526ad 100644
--- a/pkg/sentry/fsimpl/kernfs/filesystem.go
+++ b/pkg/sentry/fsimpl/kernfs/filesystem.go
@@ -752,7 +752,7 @@ func (fs *Filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
 		fs.deferDecRef(replaced)
 		replaceVFSD = replaced.VFSDentry()
 	}
-	virtfs.CommitRenameReplaceDentry(ctx, srcVFSD, replaceVFSD)
+	virtfs.CommitRenameReplaceDentry(ctx, srcVFSD, replaceVFSD) // +checklocksforce: to may be nil, that's okay.
 	return nil
 }
 
@@ -788,7 +788,7 @@ func (fs *Filesystem) RmdirAt(ctx context.Context, rp *vfs.ResolvingPath) error
 	defer mntns.DecRef(ctx)
 	vfsd := d.VFSDentry()
 	if err := virtfs.PrepareDeleteDentry(mntns, vfsd); err != nil {
-		return err
+		return err // +checklocksforce: vfsd is not locked.
 	}
 
 	if err := parentDentry.inode.RmDir(ctx, d.name, d.inode); err != nil {
diff --git a/pkg/sentry/fsimpl/overlay/filesystem.go b/pkg/sentry/fsimpl/overlay/filesystem.go
index 41207211a..77f9affc1 100644
--- a/pkg/sentry/fsimpl/overlay/filesystem.go
+++ b/pkg/sentry/fsimpl/overlay/filesystem.go
@@ -87,7 +87,7 @@ func putDentrySlice(ds *[]*dentry) {
 // fs.renameMuRUnlockAndCheckDrop(&ds)" than "defer func() {
 // fs.renameMuRUnlockAndCheckDrop(ds) }()" to work around this.
 //
-// +checklocks:fs.renameMu
+// +checklocksrelease:fs.renameMu
 func (fs *filesystem) renameMuRUnlockAndCheckDrop(ctx context.Context, dsp **[]*dentry) {
 	fs.renameMu.RUnlock()
 	if *dsp == nil {
@@ -113,7 +113,7 @@ func (fs *filesystem) renameMuRUnlockAndCheckDrop(ctx context.Context, dsp **[]*
 	putDentrySlice(*dsp)
 }
 
-// +checklocks:fs.renameMu
+// +checklocksrelease:fs.renameMu
 func (fs *filesystem) renameMuUnlockAndCheckDrop(ctx context.Context, ds **[]*dentry) {
 	if *ds == nil {
 		fs.renameMu.Unlock()
diff --git a/pkg/sentry/fsimpl/verity/filesystem.go b/pkg/sentry/fsimpl/verity/filesystem.go
index e4bfbd3c9..358a66072 100644
--- a/pkg/sentry/fsimpl/verity/filesystem.go
+++ b/pkg/sentry/fsimpl/verity/filesystem.go
@@ -75,6 +75,7 @@ func putDentrySlice(ds *[]*dentry) {
 // but dentry slices are allocated lazily, and it's much easier to say "defer
 // fs.renameMuRUnlockAndCheckDrop(&ds)" than "defer func() {
 // fs.renameMuRUnlockAndCheckDrop(ds) }()" to work around this.
+// +checklocksrelease:fs.renameMu
 func (fs *filesystem) renameMuRUnlockAndCheckDrop(ctx context.Context, ds **[]*dentry) {
 	fs.renameMu.RUnlock()
 	if *ds == nil {
@@ -90,6 +91,7 @@ func (fs *filesystem) renameMuRUnlockAndCheckDrop(ctx context.Context, ds **[]*d
 	putDentrySlice(*ds)
 }
 
+// +checklocksrelease:fs.renameMu
 func (fs *filesystem) renameMuUnlockAndCheckDrop(ctx context.Context, ds **[]*dentry) {
 	if *ds == nil {
 		fs.renameMu.Unlock()