[vfs] overlayfs: Do not unlink non-existing whiteout during file creation.

We can reuse information about whether a whiteout exists on a given file path
from stepLocked when creating a file at that path. This helps save an Unlink
call to the upper filesystem if the whiteout does NOT exist (common case).

Plumbs this information from lookupLocked() -> getChildLocked() -> stepLocked().
This also helped save a Lookup in RenameAt().

Fixes #1199

PiperOrigin-RevId: 341105351

3 files changed, 70 insertions, 65 deletions

diff --git a/pkg/sentry/fsimpl/overlay/copy_up.go b/pkg/sentry/fsimpl/overlay/copy_up.go
index 4506642ca..469f3a33d 100644
--- a/pkg/sentry/fsimpl/overlay/copy_up.go
+++ b/pkg/sentry/fsimpl/overlay/copy_up.go
@@ -409,7 +409,7 @@ func (d *dentry) copyUpDescendantsLocked(ctx context.Context, ds **[]*dentry) er
 		if dirent.Name == "." || dirent.Name == ".." {
 			continue
 		}
-		child, err := d.fs.getChildLocked(ctx, d, dirent.Name, ds)
+		child, _, err := d.fs.getChildLocked(ctx, d, dirent.Name, ds)
 		if err != nil {
 			return err
 		}
diff --git a/pkg/sentry/fsimpl/overlay/filesystem.go b/pkg/sentry/fsimpl/overlay/filesystem.go
index 04ca85f1a..30601e76d 100644
--- a/pkg/sentry/fsimpl/overlay/filesystem.go
+++ b/pkg/sentry/fsimpl/overlay/filesystem.go
@@ -22,6 +22,7 @@ import (
 	"gvisor.dev/gvisor/pkg/abi/linux"
 	"gvisor.dev/gvisor/pkg/context"
 	"gvisor.dev/gvisor/pkg/fspath"
+	"gvisor.dev/gvisor/pkg/log"
 	"gvisor.dev/gvisor/pkg/sentry/kernel/auth"
 	"gvisor.dev/gvisor/pkg/sentry/socket/unix/transport"
 	"gvisor.dev/gvisor/pkg/sentry/vfs"
@@ -121,63 +122,63 @@ func (fs *filesystem) renameMuUnlockAndCheckDrop(ctx context.Context, ds **[]*de
 // * fs.renameMu must be locked.
 // * d.dirMu must be locked.
 // * !rp.Done().
-func (fs *filesystem) stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry, mayFollowSymlinks bool, ds **[]*dentry) (*dentry, error) {
+func (fs *filesystem) stepLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry, mayFollowSymlinks bool, ds **[]*dentry) (*dentry, lookupLayer, error) {
 	if !d.isDir() {
-		return nil, syserror.ENOTDIR
+		return nil, lookupLayerNone, syserror.ENOTDIR
 	}
 	if err := d.checkPermissions(rp.Credentials(), vfs.MayExec); err != nil {
-		return nil, err
+		return nil, lookupLayerNone, err
 	}
 afterSymlink:
 	name := rp.Component()
 	if name == "." {
 		rp.Advance()
-		return d, nil
+		return d, d.topLookupLayer(), nil
 	}
 	if name == ".." {
 		if isRoot, err := rp.CheckRoot(ctx, &d.vfsd); err != nil {
-			return nil, err
+			return nil, lookupLayerNone, err
 		} else if isRoot || d.parent == nil {
 			rp.Advance()
-			return d, nil
+			return d, d.topLookupLayer(), nil
 		}
 		if err := rp.CheckMount(ctx, &d.parent.vfsd); err != nil {
-			return nil, err
+			return nil, lookupLayerNone, err
 		}
 		rp.Advance()
-		return d.parent, nil
+		return d.parent, d.parent.topLookupLayer(), nil
 	}
-	child, err := fs.getChildLocked(ctx, d, name, ds)
+	child, topLookupLayer, err := fs.getChildLocked(ctx, d, name, ds)
 	if err != nil {
-		return nil, err
+		return nil, topLookupLayer, err
 	}
 	if err := rp.CheckMount(ctx, &child.vfsd); err != nil {
-		return nil, err
+		return nil, lookupLayerNone, err
 	}
 	if child.isSymlink() && mayFollowSymlinks && rp.ShouldFollowSymlink() {
 		target, err := child.readlink(ctx)
 		if err != nil {
-			return nil, err
+			return nil, lookupLayerNone, err
 		}
 		if err := rp.HandleSymlink(target); err != nil {
-			return nil, err
+			return nil, topLookupLayer, err
 		}
 		goto afterSymlink // don't check the current directory again
 	}
 	rp.Advance()
-	return child, nil
+	return child, topLookupLayer, nil
 }
 
 // Preconditions:
 // * fs.renameMu must be locked.
 // * d.dirMu must be locked.
-func (fs *filesystem) getChildLocked(ctx context.Context, parent *dentry, name string, ds **[]*dentry) (*dentry, error) {
+func (fs *filesystem) getChildLocked(ctx context.Context, parent *dentry, name string, ds **[]*dentry) (*dentry, lookupLayer, error) {
 	if child, ok := parent.children[name]; ok {
-		return child, nil
+		return child, child.topLookupLayer(), nil
 	}
-	child, err := fs.lookupLocked(ctx, parent, name)
+	child, topLookupLayer, err := fs.lookupLocked(ctx, parent, name)
 	if err != nil {
-		return nil, err
+		return nil, topLookupLayer, err
 	}
 	if parent.children == nil {
 		parent.children = make(map[string]*dentry)
@@ -185,16 +186,16 @@ func (fs *filesystem) getChildLocked(ctx context.Context, parent *dentry, name s
 	parent.children[name] = child
 	// child's refcount is initially 0, so it may be dropped after traversal.
 	*ds = appendDentry(*ds, child)
-	return child, nil
+	return child, topLookupLayer, nil
 }
 
 // Preconditions:
 // * fs.renameMu must be locked.
 // * parent.dirMu must be locked.
-func (fs *filesystem) lookupLocked(ctx context.Context, parent *dentry, name string) (*dentry, error) {
+func (fs *filesystem) lookupLocked(ctx context.Context, parent *dentry, name string) (*dentry, lookupLayer, error) {
 	childPath := fspath.Parse(name)
 	child := fs.newDentry()
-	existsOnAnyLayer := false
+	topLookupLayer := lookupLayerNone
 	var lookupErr error
 
 	vfsObj := fs.vfsfs.VirtualFilesystem()
@@ -215,7 +216,7 @@ func (fs *filesystem) lookupLocked(ctx context.Context, parent *dentry, name str
 		defer childVD.DecRef(ctx)
 
 		mask := uint32(linux.STATX_TYPE)
-		if !existsOnAnyLayer {
+		if topLookupLayer == lookupLayerNone {
 			// Mode, UID, GID, and (for non-directories) inode number come from
 			// the topmost layer on which the file exists.
 			mask |= linux.STATX_MODE | linux.STATX_UID | linux.STATX_GID | linux.STATX_INO
@@ -238,10 +239,13 @@ func (fs *filesystem) lookupLocked(ctx context.Context, parent *dentry, name str
 		if isWhiteout(&stat) {
 			// This is a whiteout, so it "doesn't exist" on this layer, and
 			// layers below this one are ignored.
+			if isUpper {
+				topLookupLayer = lookupLayerUpperWhiteout
+			}
 			return false
 		}
 		isDir := stat.Mode&linux.S_IFMT == linux.S_IFDIR
-		if existsOnAnyLayer && !isDir {
+		if topLookupLayer != lookupLayerNone && !isDir {
 			// Directories are not merged with non-directory files from lower
 			// layers; instead, layers including and below the first
 			// non-directory file are ignored. (This file must be a directory
@@ -258,8 +262,12 @@ func (fs *filesystem) lookupLocked(ctx context.Context, parent *dentry, name str
 		} else {
 			child.lowerVDs = append(child.lowerVDs, childVD)
 		}
-		if !existsOnAnyLayer {
-			existsOnAnyLayer = true
+		if topLookupLayer == lookupLayerNone {
+			if isUpper {
+				topLookupLayer = lookupLayerUpper
+			} else {
+				topLookupLayer = lookupLayerLower
+			}
 			child.mode = uint32(stat.Mode)
 			child.uid = stat.UID
 			child.gid = stat.GID
@@ -288,11 +296,11 @@ func (fs *filesystem) lookupLocked(ctx context.Context, parent *dentry, name str
 
 	if lookupErr != nil {
 		child.destroyLocked(ctx)
-		return nil, lookupErr
+		return nil, topLookupLayer, lookupErr
 	}
-	if !existsOnAnyLayer {
+	if !topLookupLayer.existsInOverlay() {
 		child.destroyLocked(ctx)
-		return nil, syserror.ENOENT
+		return nil, topLookupLayer, syserror.ENOENT
 	}
 
 	// Device and inode numbers were copied from the topmost layer above;
@@ -306,7 +314,7 @@ func (fs *filesystem) lookupLocked(ctx context.Context, parent *dentry, name str
 		if err != nil {
 			ctx.Infof("overlay.filesystem.lookupLocked: failed to map lower layer device number (%d, %d) to an overlay-specific device number: %v", child.devMajor, child.devMinor, err)
 			child.destroyLocked(ctx)
-			return nil, err
+			return nil, topLookupLayer, err
 		}
 		child.devMajor = linux.UNNAMED_MAJOR
 		child.devMinor = childDevMinor
@@ -315,7 +323,7 @@ func (fs *filesystem) lookupLocked(ctx context.Context, parent *dentry, name str
 	parent.IncRef()
 	child.parent = parent
 	child.name = name
-	return child, nil
+	return child, topLookupLayer, nil
 }
 
 // lookupLayerLocked is similar to lookupLocked, but only returns information
@@ -414,7 +422,7 @@ func (ll lookupLayer) existsInOverlay() bool {
 func (fs *filesystem) walkParentDirLocked(ctx context.Context, rp *vfs.ResolvingPath, d *dentry, ds **[]*dentry) (*dentry, error) {
 	for !rp.Final() {
 		d.dirMu.Lock()
-		next, err := fs.stepLocked(ctx, rp, d, true /* mayFollowSymlinks */, ds)
+		next, _, err := fs.stepLocked(ctx, rp, d, true /* mayFollowSymlinks */, ds)
 		d.dirMu.Unlock()
 		if err != nil {
 			return nil, err
@@ -434,7 +442,7 @@ func (fs *filesystem) resolveLocked(ctx context.Context, rp *vfs.ResolvingPath,
 	d := rp.Start().Impl().(*dentry)
 	for !rp.Done() {
 		d.dirMu.Lock()
-		next, err := fs.stepLocked(ctx, rp, d, true /* mayFollowSymlinks */, ds)
+		next, _, err := fs.stepLocked(ctx, rp, d, true /* mayFollowSymlinks */, ds)
 		d.dirMu.Unlock()
 		if err != nil {
 			return nil, err
@@ -797,9 +805,9 @@ afterTrailingSymlink:
 	}
 	// Determine whether or not we need to create a file.
 	parent.dirMu.Lock()
-	child, err := fs.stepLocked(ctx, rp, parent, false /* mayFollowSymlinks */, &ds)
+	child, topLookupLayer, err := fs.stepLocked(ctx, rp, parent, false /* mayFollowSymlinks */, &ds)
 	if err == syserror.ENOENT && mayCreate {
-		fd, err := fs.createAndOpenLocked(ctx, rp, parent, &opts, &ds)
+		fd, err := fs.createAndOpenLocked(ctx, rp, parent, &opts, &ds, topLookupLayer == lookupLayerUpperWhiteout)
 		parent.dirMu.Unlock()
 		return fd, err
 	}
@@ -899,7 +907,7 @@ func (d *dentry) openCopiedUp(ctx context.Context, rp *vfs.ResolvingPath, opts *
 // Preconditions:
 // * parent.dirMu must be locked.
 // * parent does not already contain a child named rp.Component().
-func (fs *filesystem) createAndOpenLocked(ctx context.Context, rp *vfs.ResolvingPath, parent *dentry, opts *vfs.OpenOptions, ds **[]*dentry) (*vfs.FileDescription, error) {
+func (fs *filesystem) createAndOpenLocked(ctx context.Context, rp *vfs.ResolvingPath, parent *dentry, opts *vfs.OpenOptions, ds **[]*dentry, haveUpperWhiteout bool) (*vfs.FileDescription, error) {
 	creds := rp.Credentials()
 	if err := parent.checkPermissions(creds, vfs.MayWrite); err != nil {
 		return nil, err
@@ -924,19 +932,12 @@ func (fs *filesystem) createAndOpenLocked(ctx context.Context, rp *vfs.Resolving
 		Start: parent.upperVD,
 		Path:  fspath.Parse(childName),
 	}
-	// We don't know if a whiteout exists on the upper layer; speculatively
-	// unlink it.
-	//
-	// TODO(gvisor.dev/issue/1199): Modify OpenAt => stepLocked so that we do
-	// know whether a whiteout exists.
-	var haveUpperWhiteout bool
-	switch err := vfsObj.UnlinkAt(ctx, fs.creds, &pop); err {
-	case nil:
-		haveUpperWhiteout = true
-	case syserror.ENOENT:
-		haveUpperWhiteout = false
-	default:
-		return nil, err
+	// Unlink the whiteout if it exists.
+	if haveUpperWhiteout {
+		if err := vfsObj.UnlinkAt(ctx, fs.creds, &pop); err != nil {
+			log.Warningf("overlay.filesystem.createAndOpenLocked: failed to unlink whiteout: %v", err)
+			return nil, err
+		}
 	}
 	// Create the file on the upper layer, and get an FD representing it.
 	upperFD, err := vfsObj.OpenAt(ctx, fs.creds, &pop, &vfs.OpenOptions{
@@ -967,7 +968,7 @@ func (fs *filesystem) createAndOpenLocked(ctx context.Context, rp *vfs.Resolving
 	}
 	// Re-lookup to get a dentry representing the new file, which is needed for
 	// the returned FD.
-	child, err := fs.getChildLocked(ctx, parent, childName, ds)
+	child, _, err := fs.getChildLocked(ctx, parent, childName, ds)
 	if err != nil {
 		if cleanupErr := vfsObj.UnlinkAt(ctx, fs.creds, &pop); cleanupErr != nil {
 			panic(fmt.Sprintf("unrecoverable overlayfs inconsistency: failed to delete upper layer file after OpenAt(O_CREAT) dentry lookup failure: %v", cleanupErr))
@@ -1047,7 +1048,7 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
 	// directory, we need to check for write permission on it.
 	oldParent.dirMu.Lock()
 	defer oldParent.dirMu.Unlock()
-	renamed, err := fs.getChildLocked(ctx, oldParent, oldName, &ds)
+	renamed, _, err := fs.getChildLocked(ctx, oldParent, oldName, &ds)
 	if err != nil {
 		return err
 	}
@@ -1079,20 +1080,17 @@ func (fs *filesystem) RenameAt(ctx context.Context, rp *vfs.ResolvingPath, oldPa
 	if newParent.vfsd.IsDead() {
 		return syserror.ENOENT
 	}
-	replacedLayer, err := fs.lookupLayerLocked(ctx, newParent, newName)
-	if err != nil {
-		return err
-	}
 	var (
-		replaced     *dentry
-		replacedVFSD *vfs.Dentry
-		whiteouts    map[string]bool
+		replaced      *dentry
+		replacedVFSD  *vfs.Dentry
+		replacedLayer lookupLayer
+		whiteouts     map[string]bool
 	)
-	if replacedLayer.existsInOverlay() {
-		replaced, err = fs.getChildLocked(ctx, newParent, newName, &ds)
-		if err != nil {
-			return err
-		}
+	replaced, replacedLayer, err = fs.getChildLocked(ctx, newParent, newName, &ds)
+	if err != nil && err != syserror.ENOENT {
+		return err
+	}
+	if replaced != nil {
 		replacedVFSD = &replaced.vfsd
 		if replaced.isDir() {
 			if !renamed.isDir() {
@@ -1296,7 +1294,7 @@ func (fs *filesystem) RmdirAt(ctx context.Context, rp *vfs.ResolvingPath) error
 
 	// Unlike UnlinkAt, we need a dentry representing the child directory being
 	// removed in order to verify that it's empty.
-	child, err := fs.getChildLocked(ctx, parent, name, &ds)
+	child, _, err := fs.getChildLocked(ctx, parent, name, &ds)
 	if err != nil {
 		return err
 	}
@@ -1548,7 +1546,7 @@ func (fs *filesystem) UnlinkAt(ctx context.Context, rp *vfs.ResolvingPath) error
 		if parentMode&linux.S_ISVTX != 0 {
 			// If the parent's sticky bit is set, we need a child dentry to get
 			// its owner.
-			child, err = fs.getChildLocked(ctx, parent, name, &ds)
+			child, _, err = fs.getChildLocked(ctx, parent, name, &ds)
 			if err != nil {
 				return err
 			}
diff --git a/pkg/sentry/fsimpl/overlay/overlay.go b/pkg/sentry/fsimpl/overlay/overlay.go
index f6c58f2e7..73130bc8d 100644
--- a/pkg/sentry/fsimpl/overlay/overlay.go
+++ b/pkg/sentry/fsimpl/overlay/overlay.go
@@ -696,6 +696,13 @@ func (d *dentry) topLayer() vfs.VirtualDentry {
 	return vd
 }
 
+func (d *dentry) topLookupLayer() lookupLayer {
+	if d.upperVD.Ok() {
+		return lookupLayerUpper
+	}
+	return lookupLayerLower
+}
+
 func (d *dentry) checkPermissions(creds *auth.Credentials, ats vfs.AccessTypes) error {
 	return vfs.GenericCheckPermissions(creds, ats, linux.FileMode(atomic.LoadUint32(&d.mode)), auth.KUID(atomic.LoadUint32(&d.uid)), auth.KGID(atomic.LoadUint32(&d.gid)))
 }