Fix data race in InodeSimpleAttributes.Unstable.

We were modifying InodeSimpleAttributes.Unstable.AccessTime without holding
the necessary lock.  Luckily for us, InodeSimpleAttributes already has a
NotifyAccess method that will do the update while holding the lock.

In addition, we were holding dfo.dir.mu.Lock while setting AccessTime, which
is unnecessary, so that lock has been removed.

PiperOrigin-RevId: 231278447
Change-Id: I81ed6d3dbc0b18e3f90c1df5e5a9c06132761769

pkg/sentry/fs/fsutil/inode.go

@@ -88,30 +88,37 @@ func (*NoReadWriteFileInode) GetFile(ctx context.Context, dirent *fs.Dirent, fla
 //
 // +stateify savable
 type InodeSimpleAttributes struct {
-	// FSType is the immutable filesystem type that will be returned by
+	// fsType is the immutable filesystem type that will be returned by
 	// StatFS.
-	FSType uint64
+	fsType uint64
 
 	// mu protects unstable.
 	mu       sync.RWMutex `state:"nosave"`
-	Unstable fs.UnstableAttr
+	unstable fs.UnstableAttr
 }
 
-// NewInodeSimpleAttributes returns a new InodeSimpleAttributes.
+// NewInodeSimpleAttributes returns a new InodeSimpleAttributes with the given
+// owner and permissions, and all timestamps set to the current time.
 func NewInodeSimpleAttributes(ctx context.Context, owner fs.FileOwner, perms fs.FilePermissions, typ uint64) InodeSimpleAttributes {
+	return NewInodeSimpleAttributesWithUnstable(fs.WithCurrentTime(ctx, fs.UnstableAttr{
+		Owner: owner,
+		Perms: perms,
+	}), typ)
+}
+
+// NewInodeSimpleAttributesWithUnstable returns a new InodeSimpleAttributes
+// with the given unstable attributes.
+func NewInodeSimpleAttributesWithUnstable(uattr fs.UnstableAttr, typ uint64) InodeSimpleAttributes {
 	return InodeSimpleAttributes{
-		FSType: typ,
+		fsType:   typ,
-		Unstable: fs.WithCurrentTime(ctx, fs.UnstableAttr{
+		unstable: uattr,
-			Owner: owner,
-			Perms: perms,
-		}),
 	}
 }
 
 // UnstableAttr implements fs.InodeOperations.UnstableAttr.
 func (i *InodeSimpleAttributes) UnstableAttr(ctx context.Context, _ *fs.Inode) (fs.UnstableAttr, error) {
 	i.mu.RLock()
-	u := i.Unstable
+	u := i.unstable
 	i.mu.RUnlock()
 	return u, nil
 }
@@ -119,7 +126,7 @@ func (i *InodeSimpleAttributes) UnstableAttr(ctx context.Context, _ *fs.Inode) (
 // SetPermissions implements fs.InodeOperations.SetPermissions.
 func (i *InodeSimpleAttributes) SetPermissions(ctx context.Context, _ *fs.Inode, p fs.FilePermissions) bool {
 	i.mu.Lock()
-	i.Unstable.SetPermissions(ctx, p)
+	i.unstable.SetPermissions(ctx, p)
 	i.mu.Unlock()
 	return true
 }
@@ -127,7 +134,7 @@ func (i *InodeSimpleAttributes) SetPermissions(ctx context.Context, _ *fs.Inode,
 // SetOwner implements fs.InodeOperations.SetOwner.
 func (i *InodeSimpleAttributes) SetOwner(ctx context.Context, _ *fs.Inode, owner fs.FileOwner) error {
 	i.mu.Lock()
-	i.Unstable.SetOwner(ctx, owner)
+	i.unstable.SetOwner(ctx, owner)
 	i.mu.Unlock()
 	return nil
 }
@@ -135,7 +142,7 @@ func (i *InodeSimpleAttributes) SetOwner(ctx context.Context, _ *fs.Inode, owner
 // SetTimestamps implements fs.InodeOperations.SetTimestamps.
 func (i *InodeSimpleAttributes) SetTimestamps(ctx context.Context, _ *fs.Inode, ts fs.TimeSpec) error {
 	i.mu.Lock()
-	i.Unstable.SetTimestamps(ctx, ts)
+	i.unstable.SetTimestamps(ctx, ts)
 	i.mu.Unlock()
 	return nil
 }
@@ -143,43 +150,43 @@ func (i *InodeSimpleAttributes) SetTimestamps(ctx context.Context, _ *fs.Inode,
 // AddLink implements fs.InodeOperations.AddLink.
 func (i *InodeSimpleAttributes) AddLink() {
 	i.mu.Lock()
-	i.Unstable.Links++
+	i.unstable.Links++
 	i.mu.Unlock()
 }
 
 // DropLink implements fs.InodeOperations.DropLink.
 func (i *InodeSimpleAttributes) DropLink() {
 	i.mu.Lock()
-	i.Unstable.Links--
+	i.unstable.Links--
 	i.mu.Unlock()
 }
 
 // StatFS implements fs.InodeOperations.StatFS.
 func (i *InodeSimpleAttributes) StatFS(context.Context) (fs.Info, error) {
-	if i.FSType == 0 {
+	if i.fsType == 0 {
 		return fs.Info{}, syserror.ENOSYS
 	}
-	return fs.Info{Type: i.FSType}, nil
+	return fs.Info{Type: i.fsType}, nil
 }
 
 // NotifyAccess updates the access time.
 func (i *InodeSimpleAttributes) NotifyAccess(ctx context.Context) {
 	i.mu.Lock()
-	i.Unstable.AccessTime = ktime.NowFromContext(ctx)
+	i.unstable.AccessTime = ktime.NowFromContext(ctx)
 	i.mu.Unlock()
 }
 
 // NotifyModification updates the modification time.
 func (i *InodeSimpleAttributes) NotifyModification(ctx context.Context) {
 	i.mu.Lock()
-	i.Unstable.ModificationTime = ktime.NowFromContext(ctx)
+	i.unstable.ModificationTime = ktime.NowFromContext(ctx)
 	i.mu.Unlock()
 }
 
 // NotifyStatusChange updates the status change time.
 func (i *InodeSimpleAttributes) NotifyStatusChange(ctx context.Context) {
 	i.mu.Lock()
-	i.Unstable.StatusChangeTime = ktime.NowFromContext(ctx)
+	i.unstable.StatusChangeTime = ktime.NowFromContext(ctx)
 	i.mu.Unlock()
 }
 

pkg/sentry/fs/ramfs/BUILD

@@ -18,7 +18,6 @@ go_library(
         "//pkg/sentry/fs",
         "//pkg/sentry/fs/anon",
         "//pkg/sentry/fs/fsutil",
-        "//pkg/sentry/kernel/time",
         "//pkg/sentry/socket/unix/transport",
         "//pkg/sentry/usermem",
         "//pkg/syserror",

pkg/sentry/fs/ramfs/dir.go

@@ -22,7 +22,6 @@ import (
 	"gvisor.googlesource.com/gvisor/pkg/sentry/context"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/fs"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/fs/fsutil"
-	ktime "gvisor.googlesource.com/gvisor/pkg/sentry/kernel/time"
 	"gvisor.googlesource.com/gvisor/pkg/sentry/socket/unix/transport"
 	"gvisor.googlesource.com/gvisor/pkg/syserror"
 )
@@ -415,9 +414,7 @@ func (dfo *dirFileOperations) Readdir(ctx context.Context, file *fs.File, serial
 		Serializer: serializer,
 		DirCursor:  &dfo.dirCursor,
 	}
-	dfo.dir.mu.Lock()
+	dfo.dir.InodeSimpleAttributes.NotifyAccess(ctx)
-	dfo.dir.InodeSimpleAttributes.Unstable.AccessTime = ktime.NowFromContext(ctx)
-	dfo.dir.mu.Unlock()
 	return fs.DirentReaddir(ctx, file.Dirent, dfo, root, dirCtx, file.Offset())
 }