Merge release-20200921.0-54-gfa995da84 (automated)

2 files changed, 32 insertions, 13 deletions

diff --git a/pkg/sentry/fsimpl/kernfs/filesystem.go b/pkg/sentry/fsimpl/kernfs/filesystem.go
index 11dc24b07..5cc1c4281 100644
--- a/pkg/sentry/fsimpl/kernfs/filesystem.go
+++ b/pkg/sentry/fsimpl/kernfs/filesystem.go
@@ -207,7 +207,6 @@ func checkCreateLocked(ctx context.Context, rp *vfs.ResolvingPath, parent *Dentr
 	if len(pc) > linux.NAME_MAX {
 		return "", syserror.ENAMETOOLONG
 	}
-	// FIXME(gvisor.dev/issue/1193): Data race due to not holding dirMu.
 	if _, ok := parent.children[pc]; ok {
 		return "", syserror.EEXIST
 	}
@@ -305,6 +304,9 @@ func (fs *Filesystem) LinkAt(ctx context.Context, rp *vfs.ResolvingPath, vd vfs.
 	if err != nil {
 		return err
 	}
+
+	parent.dirMu.Lock()
+	defer parent.dirMu.Unlock()
 	pc, err := checkCreateLocked(ctx, rp, parent)
 	if err != nil {
 		return err
@@ -326,7 +328,7 @@ func (fs *Filesystem) LinkAt(ctx context.Context, rp *vfs.ResolvingPath, vd vfs.
 	if err != nil {
 		return err
 	}
-	parent.InsertChild(pc, child)
+	parent.InsertChildLocked(pc, child)
 	return nil
 }
 
@@ -342,6 +344,9 @@ func (fs *Filesystem) MkdirAt(ctx context.Context, rp *vfs.ResolvingPath, opts v
 	if err != nil {
 		return err
 	}
+
+	parent.dirMu.Lock()
+	defer parent.dirMu.Unlock()
 	pc, err := checkCreateLocked(ctx, rp, parent)
 	if err != nil {
 		return err
@@ -357,7 +362,7 @@ func (fs *Filesystem) MkdirAt(ctx context.Context, rp *vfs.ResolvingPath, opts v
 		}
 		child = newSyntheticDirectory(rp.Credentials(), opts.Mode)
 	}
-	parent.InsertChild(pc, child)
+	parent.InsertChildLocked(pc, child)
 	return nil
 }
 
@@ -373,6 +378,9 @@ func (fs *Filesystem) MknodAt(ctx context.Context, rp *vfs.ResolvingPath, opts v
 	if err != nil {
 		return err
 	}
+
+	parent.dirMu.Lock()
+	defer parent.dirMu.Unlock()
 	pc, err := checkCreateLocked(ctx, rp, parent)
 	if err != nil {
 		return err
@@ -385,7 +393,7 @@ func (fs *Filesystem) MknodAt(ctx context.Context, rp *vfs.ResolvingPath, opts v
 	if err != nil {
 		return err
 	}
-	parent.InsertChild(pc, newD)
+	parent.InsertChildLocked(pc, newD)
 	return nil
 }
 
@@ -483,6 +491,9 @@ afterTrailingSymlink:
 		if err != nil {
 			return nil, err
 		}
+		// FIXME(gvisor.dev/issue/1193): Race between checking existence with
+		// fs.stepExistingLocked and parent.InsertChild. If possible, we should hold
+		// dirMu from one to the other.
 		parent.InsertChild(pc, child)
 		child.inode.IncRef()
 		defer child.inode.DecRef(ctx)
@@ -739,6 +750,9 @@ func (fs *Filesystem) SymlinkAt(ctx context.Context, rp *vfs.ResolvingPath, targ
 	if err != nil {
 		return err
 	}
+	parent.dirMu.Lock()
+	defer parent.dirMu.Unlock()
+
 	pc, err := checkCreateLocked(ctx, rp, parent)
 	if err != nil {
 		return err
@@ -751,7 +765,7 @@ func (fs *Filesystem) SymlinkAt(ctx context.Context, rp *vfs.ResolvingPath, targ
 	if err != nil {
 		return err
 	}
-	parent.InsertChild(pc, child)
+	parent.InsertChildLocked(pc, child)
 	return nil
 }
 
diff --git a/pkg/sentry/fsimpl/kernfs/kernfs.go b/pkg/sentry/fsimpl/kernfs/kernfs.go
index f543a2065..6d3d79333 100644
--- a/pkg/sentry/fsimpl/kernfs/kernfs.go
+++ b/pkg/sentry/fsimpl/kernfs/kernfs.go
@@ -29,7 +29,7 @@
 //
 // Reference Model:
 //
-// Kernfs dentries represents named pointers to inodes. Dentries and inode have
+// Kernfs dentries represents named pointers to inodes. Dentries and inodes have
 // independent lifetimes and reference counts. A child dentry unconditionally
 // holds a reference on its parent directory's dentry. A dentry also holds a
 // reference on the inode it points to. Multiple dentries can point to the same
@@ -177,6 +177,10 @@ type Dentry struct {
 	name   string
 
 	// dirMu protects children and the names of child Dentries.
+	//
+	// Note that holding fs.mu for writing is not sufficient;
+	// revalidateChildLocked(), which is a very hot path, may modify children with
+	// fs.mu acquired for reading only.
 	dirMu    sync.Mutex `state:"nosave"`
 	children map[string]*Dentry
 
@@ -244,9 +248,8 @@ func (d *Dentry) Watches() *vfs.Watches {
 func (d *Dentry) OnZeroWatches(context.Context) {}
 
 // InsertChild inserts child into the vfs dentry cache with the given name under
-// this dentry. This does not update the directory inode, so calling this on
-// its own isn't sufficient to insert a child into a directory. InsertChild
-// updates the link count on d if required.
+// this dentry. This does not update the directory inode, so calling this on its
+// own isn't sufficient to insert a child into a directory.
 //
 // Precondition: d must represent a directory inode.
 func (d *Dentry) InsertChild(name string, child *Dentry) {
@@ -258,10 +261,12 @@ func (d *Dentry) InsertChild(name string, child *Dentry) {
 // InsertChildLocked is equivalent to InsertChild, with additional
 // preconditions.
 //
-// Precondition: d.dirMu must be locked.
+// Preconditions:
+// * d must represent a directory inode.
+// * d.dirMu must be locked.
 func (d *Dentry) InsertChildLocked(name string, child *Dentry) {
 	if !d.isDir() {
-		panic(fmt.Sprintf("InsertChild called on non-directory Dentry: %+v.", d))
+		panic(fmt.Sprintf("InsertChildLocked called on non-directory Dentry: %+v.", d))
 	}
 	d.IncRef() // DecRef in child's Dentry.destroy.
 	child.parent = d
@@ -322,7 +327,6 @@ func (d *Dentry) Inode() Inode {
 //
 // - Checking that dentries passed to methods are of the appropriate file type.
 // - Checking permissions.
-// - Updating link and reference counts.
 //
 // Specific responsibilities of implementations are documented below.
 type Inode interface {
@@ -332,7 +336,8 @@ type Inode interface {
 	inodeRefs
 
 	// Methods related to node metadata. A generic implementation is provided by
-	// InodeAttrs.
+	// InodeAttrs. Note that a concrete filesystem using kernfs is responsible for
+	// managing link counts.
 	inodeMetadata
 
 	// Method for inodes that represent symlink. InodeNotSymlink provides a