Fix incorrect tmpfs timestamp updates

* Creation of files, directories (and other fs objects) in a directory
  should always update ctime.
* Same for removal.
* atime should not be updated on lookup, only readdir.

I've also renamed some misleading functions that update mtime and ctime.

PiperOrigin-RevId: 249115063
Change-Id: I30fa275fa7db96d01aa759ed64628c18bb3a7dc7

5 files changed, 87 insertions, 42 deletions

diff --git a/pkg/sentry/fs/fsutil/inode.go b/pkg/sentry/fs/fsutil/inode.go
index 5e1bfeb58..a22b6ce9c 100644
--- a/pkg/sentry/fs/fsutil/inode.go
+++ b/pkg/sentry/fs/fsutil/inode.go
@@ -192,6 +192,16 @@ func (i *InodeSimpleAttributes) NotifyStatusChange(ctx context.Context) {
 	i.mu.Unlock()
 }
 
+// NotifyModificationAndStatusChange updates the modification and status change
+// times.
+func (i *InodeSimpleAttributes) NotifyModificationAndStatusChange(ctx context.Context) {
+	i.mu.Lock()
+	now := ktime.NowFromContext(ctx)
+	i.unstable.ModificationTime = now
+	i.unstable.StatusChangeTime = now
+	i.mu.Unlock()
+}
+
 // InodeSimpleExtendedAttributes implements
 // fs.InodeOperations.{Get,Set,List}xattr.
 //
diff --git a/pkg/sentry/fs/fsutil/inode_cached.go b/pkg/sentry/fs/fsutil/inode_cached.go
index bc0b8c744..7bee2eb5f 100644
--- a/pkg/sentry/fs/fsutil/inode_cached.go
+++ b/pkg/sentry/fs/fsutil/inode_cached.go
@@ -299,7 +299,7 @@ func (c *CachingInodeOperations) Truncate(ctx context.Context, inode *fs.Inode,
 	}
 	oldSize := c.attr.Size
 	c.attr.Size = size
-	c.touchModificationTimeLocked(now)
+	c.touchModificationAndStatusChangeTimeLocked(now)
 
 	// We drop c.dataMu here so that we can lock c.mapsMu and invalidate
 	// mappings below. This allows concurrent calls to Read/Translate/etc.
@@ -360,7 +360,7 @@ func (c *CachingInodeOperations) Allocate(ctx context.Context, offset, length in
 	}
 
 	c.attr.Size = newSize
-	c.touchModificationTimeLocked(now)
+	c.touchModificationAndStatusChangeTimeLocked(now)
 	return nil
 }
 
@@ -394,19 +394,19 @@ func (c *CachingInodeOperations) WriteOut(ctx context.Context, inode *fs.Inode)
 	return c.backingFile.Sync(ctx)
 }
 
-// IncLinks increases the link count and updates cached access time.
+// IncLinks increases the link count and updates cached modification time.
 func (c *CachingInodeOperations) IncLinks(ctx context.Context) {
 	c.attrMu.Lock()
 	c.attr.Links++
-	c.touchModificationTimeLocked(ktime.NowFromContext(ctx))
+	c.touchModificationAndStatusChangeTimeLocked(ktime.NowFromContext(ctx))
 	c.attrMu.Unlock()
 }
 
-// DecLinks decreases the link count and updates cached access time.
+// DecLinks decreases the link count and updates cached modification time.
 func (c *CachingInodeOperations) DecLinks(ctx context.Context) {
 	c.attrMu.Lock()
 	c.attr.Links--
-	c.touchModificationTimeLocked(ktime.NowFromContext(ctx))
+	c.touchModificationAndStatusChangeTimeLocked(ktime.NowFromContext(ctx))
 	c.attrMu.Unlock()
 }
 
@@ -432,19 +432,19 @@ func (c *CachingInodeOperations) touchAccessTimeLocked(now time.Time) {
 	c.dirtyAttr.AccessTime = true
 }
 
-// TouchModificationTime updates the cached modification and status change time
-// in-place to the current time.
-func (c *CachingInodeOperations) TouchModificationTime(ctx context.Context) {
+// TouchModificationAndStatusChangeTime updates the cached modification and
+// status change times in-place to the current time.
+func (c *CachingInodeOperations) TouchModificationAndStatusChangeTime(ctx context.Context) {
 	c.attrMu.Lock()
-	c.touchModificationTimeLocked(ktime.NowFromContext(ctx))
+	c.touchModificationAndStatusChangeTimeLocked(ktime.NowFromContext(ctx))
 	c.attrMu.Unlock()
 }
 
-// touchModificationTimeLocked updates the cached modification and status
-// change time in-place to the current time.
+// touchModificationAndStatusChangeTimeLocked updates the cached modification
+// and status change times in-place to the current time.
 //
 // Preconditions: c.attrMu is locked for writing.
-func (c *CachingInodeOperations) touchModificationTimeLocked(now time.Time) {
+func (c *CachingInodeOperations) touchModificationAndStatusChangeTimeLocked(now time.Time) {
 	c.attr.ModificationTime = now
 	c.dirtyAttr.ModificationTime = true
 	c.attr.StatusChangeTime = now
@@ -554,7 +554,7 @@ func (c *CachingInodeOperations) Write(ctx context.Context, src usermem.IOSequen
 
 	c.attrMu.Lock()
 	// Compare Linux's mm/filemap.c:__generic_file_write_iter() => file_update_time().
-	c.touchModificationTimeLocked(ktime.NowFromContext(ctx))
+	c.touchModificationAndStatusChangeTimeLocked(ktime.NowFromContext(ctx))
 	n, err := src.CopyInTo(ctx, &inodeReadWriter{ctx, c, offset})
 	c.attrMu.Unlock()
 	return n, err
diff --git a/pkg/sentry/fs/gofer/path.go b/pkg/sentry/fs/gofer/path.go
index 148e2f038..6ed50a77f 100644
--- a/pkg/sentry/fs/gofer/path.go
+++ b/pkg/sentry/fs/gofer/path.go
@@ -113,7 +113,7 @@ func (i *inodeOperations) Create(ctx context.Context, dir *fs.Inode, name string
 		return nil, err
 	}
 
-	i.touchModificationTime(ctx, dir)
+	i.touchModificationAndStatusChangeTime(ctx, dir)
 
 	// Get an unopened p9.File for the file we created so that it can be cloned
 	// and re-opened multiple times after creation, while also getting its
@@ -167,7 +167,7 @@ func (i *inodeOperations) CreateLink(ctx context.Context, dir *fs.Inode, oldname
 	if _, err := i.fileState.file.symlink(ctx, oldname, newname, p9.UID(owner.UID), p9.GID(owner.GID)); err != nil {
 		return err
 	}
-	i.touchModificationTime(ctx, dir)
+	i.touchModificationAndStatusChangeTime(ctx, dir)
 	return nil
 }
 
@@ -189,7 +189,7 @@ func (i *inodeOperations) CreateHardLink(ctx context.Context, inode *fs.Inode, t
 		// Increase link count.
 		targetOpts.cachingInodeOps.IncLinks(ctx)
 	}
-	i.touchModificationTime(ctx, inode)
+	i.touchModificationAndStatusChangeTime(ctx, inode)
 	return nil
 }
 
@@ -205,6 +205,8 @@ func (i *inodeOperations) CreateDirectory(ctx context.Context, dir *fs.Inode, s
 	}
 	if i.session().cachePolicy.cacheUAttrs(dir) {
 		// Increase link count.
+		//
+		// N.B. This will update the modification time.
 		i.cachingInodeOps.IncLinks(ctx)
 	}
 	if i.session().cachePolicy.cacheReaddir() {
@@ -246,7 +248,7 @@ func (i *inodeOperations) Bind(ctx context.Context, dir *fs.Inode, name string,
 	// We're not going to use this file.
 	hostFile.Close()
 
-	i.touchModificationTime(ctx, dir)
+	i.touchModificationAndStatusChangeTime(ctx, dir)
 
 	// Get the attributes of the file to create inode key.
 	qid, mask, attr, err := getattr(ctx, newFile)
@@ -317,7 +319,7 @@ func (i *inodeOperations) Remove(ctx context.Context, dir *fs.Inode, name string
 	if removeSocket {
 		i.session().endpoints.remove(key)
 	}
-	i.touchModificationTime(ctx, dir)
+	i.touchModificationAndStatusChangeTime(ctx, dir)
 
 	return nil
 }
@@ -397,9 +399,9 @@ func (i *inodeOperations) Rename(ctx context.Context, inode *fs.Inode, oldParent
 	return nil
 }
 
-func (i *inodeOperations) touchModificationTime(ctx context.Context, inode *fs.Inode) {
+func (i *inodeOperations) touchModificationAndStatusChangeTime(ctx context.Context, inode *fs.Inode) {
 	if i.session().cachePolicy.cacheUAttrs(inode) {
-		i.cachingInodeOps.TouchModificationTime(ctx)
+		i.cachingInodeOps.TouchModificationAndStatusChangeTime(ctx)
 	}
 	if i.session().cachePolicy.cacheReaddir() {
 		// Invalidate readdir cache.
diff --git a/pkg/sentry/fs/ramfs/dir.go b/pkg/sentry/fs/ramfs/dir.go
index c97ad26f5..cd6e03d66 100644
--- a/pkg/sentry/fs/ramfs/dir.go
+++ b/pkg/sentry/fs/ramfs/dir.go
@@ -112,7 +112,7 @@ func NewDir(ctx context.Context, contents map[string]*fs.Inode, owner fs.FileOwn
 }
 
 // addChildLocked add the child inode, inheriting its reference.
-func (d *Dir) addChildLocked(name string, inode *fs.Inode) {
+func (d *Dir) addChildLocked(ctx context.Context, name string, inode *fs.Inode) {
 	d.children[name] = inode
 	d.dentryMap.Add(name, fs.DentAttr{
 		Type:    inode.StableAttr.Type,
@@ -123,18 +123,25 @@ func (d *Dir) addChildLocked(name string, inode *fs.Inode) {
 	// corresponding to '..' from the subdirectory.
 	if fs.IsDir(inode.StableAttr) {
 		d.AddLink()
+		// ctime updated below.
 	}
 
 	// Given we're now adding this inode to the directory we must also
-	// increase its link count. Similarly we decremented it in removeChildLocked.
+	// increase its link count. Similarly we decrement it in removeChildLocked.
+	//
+	// Changing link count updates ctime.
 	inode.AddLink()
+	inode.InodeOperations.NotifyStatusChange(ctx)
+
+	// We've change the directory. This always updates our mtime and ctime.
+	d.NotifyModificationAndStatusChange(ctx)
 }
 
 // AddChild adds a child to this dir.
 func (d *Dir) AddChild(ctx context.Context, name string, inode *fs.Inode) {
 	d.mu.Lock()
 	defer d.mu.Unlock()
-	d.addChildLocked(name, inode)
+	d.addChildLocked(ctx, name, inode)
 }
 
 // FindChild returns (child, true) if the directory contains name.
@@ -179,14 +186,18 @@ func (d *Dir) removeChildLocked(ctx context.Context, name string) (*fs.Inode, er
 	// link count which was the child's ".." directory entry.
 	if fs.IsDir(inode.StableAttr) {
 		d.DropLink()
+		// ctime changed below.
 	}
 
-	// Update ctime.
-	inode.InodeOperations.NotifyStatusChange(ctx)
-
 	// Given we're now removing this inode to the directory we must also
 	// decrease its link count. Similarly it is increased in addChildLocked.
+	//
+	// Changing link count updates ctime.
 	inode.DropLink()
+	inode.InodeOperations.NotifyStatusChange(ctx)
+
+	// We've change the directory. This always updates our mtime and ctime.
+	d.NotifyModificationAndStatusChange(ctx)
 
 	return inode, nil
 }
@@ -263,8 +274,6 @@ func (d *Dir) Lookup(ctx context.Context, _ *fs.Inode, p string) (*fs.Dirent, er
 
 // walkLocked must be called with d.mu held.
 func (d *Dir) walkLocked(ctx context.Context, p string) (*fs.Inode, error) {
-	d.NotifyAccess(ctx)
-
 	// Lookup a child node.
 	if inode, ok := d.children[p]; ok {
 		return inode, nil
@@ -290,8 +299,7 @@ func (d *Dir) createInodeOperationsCommon(ctx context.Context, name string, make
 		return nil, err
 	}
 
-	d.addChildLocked(name, inode)
-	d.NotifyModification(ctx)
+	d.addChildLocked(ctx, name, inode)
 
 	return inode, nil
 }
@@ -342,11 +350,7 @@ func (d *Dir) CreateHardLink(ctx context.Context, dir *fs.Inode, target *fs.Inod
 	target.IncRef()
 
 	// The link count will be incremented in addChildLocked.
-	d.addChildLocked(name, target)
-	d.NotifyModification(ctx)
-
-	// Update ctime.
-	target.InodeOperations.NotifyStatusChange(ctx)
+	d.addChildLocked(ctx, name, target)
 
 	return nil
 }
@@ -359,8 +363,6 @@ func (d *Dir) CreateDirectory(ctx context.Context, dir *fs.Inode, name string, p
 	_, err := d.createInodeOperationsCommon(ctx, name, func() (*fs.Inode, error) {
 		return d.NewDir(ctx, dir, perms)
 	})
-	// TODO(nlacasse): Support updating status times, as those should be
-	// updated by links.
 	return err
 }
 
@@ -526,10 +528,7 @@ func Rename(ctx context.Context, oldParent fs.InodeOperations, oldName string, n
 	// Do the swap.
 	n := op.children[oldName]
 	op.removeChildLocked(ctx, oldName)
-	np.addChildLocked(newName, n)
-
-	// Update ctime.
-	n.InodeOperations.NotifyStatusChange(ctx)
+	np.addChildLocked(ctx, newName, n)
 
 	return nil
 }
diff --git a/test/syscalls/linux/stat_times.cc b/test/syscalls/linux/stat_times.cc
index 195c87ca5..68c0bef09 100644
--- a/test/syscalls/linux/stat_times.cc
+++ b/test/syscalls/linux/stat_times.cc
@@ -263,6 +263,40 @@ TEST(StatTimesTest, DirList) {
              CtimeEffect::Unchanged);
 }
 
+// Creating a file in a directory changes mtime and ctime.
+TEST(StatTimesTest, DirCreateFile) {
+  const TempPath dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+
+  TempPath file;
+  auto fn = [&] {
+    file = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFileIn(dir.path()));
+  };
+  CheckTimes(dir, fn, AtimeEffect::Unchanged, MtimeEffect::Changed,
+             CtimeEffect::Changed);
+}
+
+// Creating a directory in a directory changes mtime and ctime.
+TEST(StatTimesTest, DirCreateDir) {
+  const TempPath dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+
+  TempPath dir2;
+  auto fn = [&] {
+    dir2 = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDirIn(dir.path()));
+  };
+  CheckTimes(dir, fn, AtimeEffect::Unchanged, MtimeEffect::Changed,
+             CtimeEffect::Changed);
+}
+
+// Removing a file from a directory changes mtime and ctime.
+TEST(StatTimesTest, DirRemoveFile) {
+  const TempPath dir = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateDir());
+
+  TempPath file = ASSERT_NO_ERRNO_AND_VALUE(TempPath::CreateFileIn(dir.path()));
+  auto fn = [&] { file.reset(); };
+  CheckTimes(dir, fn, AtimeEffect::Unchanged, MtimeEffect::Changed,
+             CtimeEffect::Changed);
+}
+
 }  // namespace
 
 }  // namespace testing