From 04105781ad558662e1e48bad17197df244ff7841 Mon Sep 17 00:00:00 2001 From: Michael Pratt Date: Fri, 17 May 2019 13:04:44 -0700 Subject: Fix gofer rename ctime and cleanup stat_times test There is a lot of redundancy that we can simplify in the stat_times test. This will make it easier to add new tests. However, the simplification reveals that cached uattrs on goferfs don't properly update ctime on rename. PiperOrigin-RevId: 248773425 Change-Id: I52662728e1e9920981555881f9a85f9ce04041cf --- pkg/sentry/fs/fsutil/inode.go | 4 ++-- pkg/sentry/fs/fsutil/inode_cached.go | 8 ++++++++ pkg/sentry/fs/gofer/path.go | 7 ++++++- pkg/sentry/fs/host/inode.go | 2 +- pkg/sentry/fs/inode.go | 2 +- pkg/sentry/fs/inode_operations.go | 5 +++-- pkg/sentry/fs/inode_overlay.go | 2 +- pkg/sentry/fs/mock.go | 2 +- pkg/sentry/fs/ramfs/dir.go | 2 +- pkg/sentry/fs/tmpfs/inode_file.go | 2 +- pkg/sentry/fs/tmpfs/tmpfs.go | 8 ++++---- 11 files changed, 29 insertions(+), 15 deletions(-) (limited to 'pkg/sentry') diff --git a/pkg/sentry/fs/fsutil/inode.go b/pkg/sentry/fs/fsutil/inode.go index 151be1d0d..5e1bfeb58 100644 --- a/pkg/sentry/fs/fsutil/inode.go +++ b/pkg/sentry/fs/fsutil/inode.go @@ -341,7 +341,7 @@ func (InodeNotDirectory) RemoveDirectory(context.Context, *fs.Inode, string) err } // Rename implements fs.FileOperations.Rename. -func (InodeNotDirectory) Rename(context.Context, *fs.Inode, string, *fs.Inode, string, bool) error { +func (InodeNotDirectory) Rename(context.Context, *fs.Inode, *fs.Inode, string, *fs.Inode, string, bool) error { return syserror.EINVAL } @@ -381,7 +381,7 @@ func (InodeNoopTruncate) Truncate(context.Context, *fs.Inode, int64) error { type InodeNotRenameable struct{} // Rename implements fs.InodeOperations.Rename. -func (InodeNotRenameable) Rename(context.Context, *fs.Inode, string, *fs.Inode, string, bool) error { +func (InodeNotRenameable) Rename(context.Context, *fs.Inode, *fs.Inode, string, *fs.Inode, string, bool) error { return syserror.EINVAL } diff --git a/pkg/sentry/fs/fsutil/inode_cached.go b/pkg/sentry/fs/fsutil/inode_cached.go index 03cad37f3..bc0b8c744 100644 --- a/pkg/sentry/fs/fsutil/inode_cached.go +++ b/pkg/sentry/fs/fsutil/inode_cached.go @@ -451,6 +451,14 @@ func (c *CachingInodeOperations) touchModificationTimeLocked(now time.Time) { c.dirtyAttr.StatusChangeTime = true } +// TouchStatusChangeTime updates the cached status change time in-place to the +// current time. +func (c *CachingInodeOperations) TouchStatusChangeTime(ctx context.Context) { + c.attrMu.Lock() + c.touchStatusChangeTimeLocked(ktime.NowFromContext(ctx)) + c.attrMu.Unlock() +} + // touchStatusChangeTimeLocked updates the cached status change time // in-place to the current time. // diff --git a/pkg/sentry/fs/gofer/path.go b/pkg/sentry/fs/gofer/path.go index 875de8b5f..babfa4560 100644 --- a/pkg/sentry/fs/gofer/path.go +++ b/pkg/sentry/fs/gofer/path.go @@ -344,7 +344,7 @@ func (i *inodeOperations) RemoveDirectory(ctx context.Context, dir *fs.Inode, na } // Rename renames this node. -func (i *inodeOperations) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { +func (i *inodeOperations) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { if len(newName) > maxFilenameLen { return syserror.ENAMETOOLONG } @@ -389,6 +389,11 @@ func (i *inodeOperations) Rename(ctx context.Context, oldParent *fs.Inode, oldNa newParentInodeOperations.markDirectoryDirty() } } + + // Rename always updates ctime. + if i.session().cachePolicy.cacheUAttrs(inode) { + i.cachingInodeOps.TouchStatusChangeTime(ctx) + } return nil } diff --git a/pkg/sentry/fs/host/inode.go b/pkg/sentry/fs/host/inode.go index d36ac9a87..ebf2154bc 100644 --- a/pkg/sentry/fs/host/inode.go +++ b/pkg/sentry/fs/host/inode.go @@ -301,7 +301,7 @@ func (i *inodeOperations) RemoveDirectory(ctx context.Context, dir *fs.Inode, na } // Rename implements fs.InodeOperations.Rename. -func (i *inodeOperations) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { +func (i *inodeOperations) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { op, ok := oldParent.InodeOperations.(*inodeOperations) if !ok { return syscall.EXDEV diff --git a/pkg/sentry/fs/inode.go b/pkg/sentry/fs/inode.go index 22f316daf..aef1a1cb9 100644 --- a/pkg/sentry/fs/inode.go +++ b/pkg/sentry/fs/inode.go @@ -216,7 +216,7 @@ func (i *Inode) Rename(ctx context.Context, oldParent *Dirent, renamed *Dirent, if i.overlay != nil { return overlayRename(ctx, i.overlay, oldParent, renamed, newParent, newName, replacement) } - return i.InodeOperations.Rename(ctx, oldParent.Inode, renamed.name, newParent.Inode, newName, replacement) + return i.InodeOperations.Rename(ctx, renamed.Inode, oldParent.Inode, renamed.name, newParent.Inode, newName, replacement) } // Bind calls i.InodeOperations.Bind with i as the directory. diff --git a/pkg/sentry/fs/inode_operations.go b/pkg/sentry/fs/inode_operations.go index abafe4791..3211f1817 100644 --- a/pkg/sentry/fs/inode_operations.go +++ b/pkg/sentry/fs/inode_operations.go @@ -131,14 +131,15 @@ type InodeOperations interface { RemoveDirectory(ctx context.Context, dir *Inode, name string) error // Rename atomically renames oldName under oldParent to newName under - // newParent where oldParent and newParent are directories. + // newParent where oldParent and newParent are directories. inode is + // the Inode of this InodeOperations. // // If replacement is true, then newName already exists and this call // will replace it with oldName. // // Implementations are responsible for rejecting renames that replace // non-empty directories. - Rename(ctx context.Context, oldParent *Inode, oldName string, newParent *Inode, newName string, replacement bool) error + Rename(ctx context.Context, inode *Inode, oldParent *Inode, oldName string, newParent *Inode, newName string, replacement bool) error // Bind binds a new socket under dir at the given name. // diff --git a/pkg/sentry/fs/inode_overlay.go b/pkg/sentry/fs/inode_overlay.go index ead487097..ea574224f 100644 --- a/pkg/sentry/fs/inode_overlay.go +++ b/pkg/sentry/fs/inode_overlay.go @@ -389,7 +389,7 @@ func overlayRename(ctx context.Context, o *overlayEntry, oldParent *Dirent, rena return err } oldName := renamed.name - if err := o.upper.InodeOperations.Rename(ctx, oldParent.Inode.overlay.upper, oldName, newParent.Inode.overlay.upper, newName, replacement); err != nil { + if err := o.upper.InodeOperations.Rename(ctx, renamed.Inode.overlay.upper, oldParent.Inode.overlay.upper, oldName, newParent.Inode.overlay.upper, newName, replacement); err != nil { return err } if renamed.Inode.overlay.lowerExists { diff --git a/pkg/sentry/fs/mock.go b/pkg/sentry/fs/mock.go index a71144b2c..064943c5b 100644 --- a/pkg/sentry/fs/mock.go +++ b/pkg/sentry/fs/mock.go @@ -132,7 +132,7 @@ func (n *MockInodeOperations) CreateDirectory(context.Context, *Inode, string, F } // Rename implements fs.InodeOperations.Rename. -func (n *MockInodeOperations) Rename(ctx context.Context, oldParent *Inode, oldName string, newParent *Inode, newName string, replacement bool) error { +func (n *MockInodeOperations) Rename(ctx context.Context, inode *Inode, oldParent *Inode, oldName string, newParent *Inode, newName string, replacement bool) error { n.renameCalled = true return nil } diff --git a/pkg/sentry/fs/ramfs/dir.go b/pkg/sentry/fs/ramfs/dir.go index eb98b59cc..c97ad26f5 100644 --- a/pkg/sentry/fs/ramfs/dir.go +++ b/pkg/sentry/fs/ramfs/dir.go @@ -401,7 +401,7 @@ func (d *Dir) GetFile(ctx context.Context, dirent *fs.Dirent, flags fs.FileFlags } // Rename implements fs.InodeOperations.Rename. -func (*Dir) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { +func (*Dir) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { return Rename(ctx, oldParent.InodeOperations, oldName, newParent.InodeOperations, newName, replacement) } diff --git a/pkg/sentry/fs/tmpfs/inode_file.go b/pkg/sentry/fs/tmpfs/inode_file.go index c90062a22..3fe659543 100644 --- a/pkg/sentry/fs/tmpfs/inode_file.go +++ b/pkg/sentry/fs/tmpfs/inode_file.go @@ -149,7 +149,7 @@ func (f *fileInodeOperations) Mappable(*fs.Inode) memmap.Mappable { } // Rename implements fs.InodeOperations.Rename. -func (*fileInodeOperations) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { +func (*fileInodeOperations) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { return rename(ctx, oldParent, oldName, newParent, newName, replacement) } diff --git a/pkg/sentry/fs/tmpfs/tmpfs.go b/pkg/sentry/fs/tmpfs/tmpfs.go index 6ad5c5adb..263d10cfe 100644 --- a/pkg/sentry/fs/tmpfs/tmpfs.go +++ b/pkg/sentry/fs/tmpfs/tmpfs.go @@ -238,7 +238,7 @@ func (d *Dir) newCreateOps() *ramfs.CreateOps { } // Rename implements fs.InodeOperations.Rename. -func (d *Dir) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { +func (d *Dir) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { return rename(ctx, oldParent, oldName, newParent, newName, replacement) } @@ -271,7 +271,7 @@ func NewSymlink(ctx context.Context, target string, owner fs.FileOwner, msrc *fs } // Rename implements fs.InodeOperations.Rename. -func (s *Symlink) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { +func (s *Symlink) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { return rename(ctx, oldParent, oldName, newParent, newName, replacement) } @@ -301,7 +301,7 @@ func NewSocket(ctx context.Context, socket transport.BoundEndpoint, owner fs.Fil } // Rename implements fs.InodeOperations.Rename. -func (s *Socket) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { +func (s *Socket) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { return rename(ctx, oldParent, oldName, newParent, newName, replacement) } @@ -338,7 +338,7 @@ func NewFifo(ctx context.Context, owner fs.FileOwner, perms fs.FilePermissions, } // Rename implements fs.InodeOperations.Rename. -func (f *Fifo) Rename(ctx context.Context, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { +func (f *Fifo) Rename(ctx context.Context, inode *fs.Inode, oldParent *fs.Inode, oldName string, newParent *fs.Inode, newName string, replacement bool) error { return rename(ctx, oldParent, oldName, newParent, newName, replacement) } -- cgit v1.2.3