From ee0e725b60f2b343efb583753d50541cba11764e Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Mon, 18 Jul 2022 17:11:44 -0700 Subject: [PATCH] Move all InotifyWithParent() callers in FDImpl to VFS layer. `vfs.FileDescription` in general handles all inotify work. Only SetStat, SetXattr, RemoveXattr and Iterdirents require FDImpls to handle the Inotify work. This leads to repeated code. And for kernfs users, there are too many FD implementors. So it becomes difficult plumbing Inotify consistenly through all FDImpls. So make it consistent that all Inotify work is handled by `vfs.FileDescription`, not FDImpls. PiperOrigin-RevId: 461751122 --- pkg/sentry/fsimpl/gofer/directory.go | 1 - pkg/sentry/fsimpl/gofer/gofer.go | 22 +++-------------- pkg/sentry/fsimpl/overlay/directory.go | 2 -- pkg/sentry/fsimpl/overlay/overlay.go | 24 ++++--------------- pkg/sentry/fsimpl/overlay/regular_file.go | 8 ------- pkg/sentry/fsimpl/tmpfs/directory.go | 2 -- pkg/sentry/fsimpl/tmpfs/tmpfs.go | 29 +++-------------------- pkg/sentry/vfs/file_description.go | 21 +++++++++++++--- 8 files changed, 28 insertions(+), 81 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/directory.go b/pkg/sentry/fsimpl/gofer/directory.go index 70ef06409..6156f4990 100644 --- a/pkg/sentry/fsimpl/gofer/directory.go +++ b/pkg/sentry/fsimpl/gofer/directory.go @@ -181,7 +181,6 @@ func (fd *directoryFD) IterDirents(ctx context.Context, cb vfs.IterDirentsCallba fd.dirents = ds } - d.InotifyWithParent(ctx, linux.IN_ACCESS, 0, vfs.PathEvent) if d.cachedMetadataAuthoritative() { d.touchAtime(fd.vfsfd.Mount()) } diff --git a/pkg/sentry/fsimpl/gofer/gofer.go b/pkg/sentry/fsimpl/gofer/gofer.go index ecc962260..ec8d76551 100644 --- a/pkg/sentry/fsimpl/gofer/gofer.go +++ b/pkg/sentry/fsimpl/gofer/gofer.go @@ -2592,13 +2592,7 @@ func (fd *fileDescription) Stat(ctx context.Context, opts vfs.StatOptions) (linu // SetStat implements vfs.FileDescriptionImpl.SetStat. func (fd *fileDescription) SetStat(ctx context.Context, opts vfs.SetStatOptions) error { - if err := fd.dentry().setStat(ctx, auth.CredentialsFromContext(ctx), &opts, fd.vfsfd.Mount()); err != nil { - return err - } - if ev := vfs.InotifyEventFromStatMask(opts.Stat.Mask); ev != 0 { - fd.dentry().InotifyWithParent(ctx, ev, 0, vfs.InodeEvent) - } - return nil + return fd.dentry().setStat(ctx, auth.CredentialsFromContext(ctx), &opts, fd.vfsfd.Mount()) } // ListXattr implements vfs.FileDescriptionImpl.ListXattr. @@ -2613,22 +2607,12 @@ func (fd *fileDescription) GetXattr(ctx context.Context, opts vfs.GetXattrOption // SetXattr implements vfs.FileDescriptionImpl.SetXattr. func (fd *fileDescription) SetXattr(ctx context.Context, opts vfs.SetXattrOptions) error { - d := fd.dentry() - if err := d.setXattr(ctx, auth.CredentialsFromContext(ctx), &opts); err != nil { - return err - } - d.InotifyWithParent(ctx, linux.IN_ATTRIB, 0, vfs.InodeEvent) - return nil + return fd.dentry().setXattr(ctx, auth.CredentialsFromContext(ctx), &opts) } // RemoveXattr implements vfs.FileDescriptionImpl.RemoveXattr. func (fd *fileDescription) RemoveXattr(ctx context.Context, name string) error { - d := fd.dentry() - if err := d.removeXattr(ctx, auth.CredentialsFromContext(ctx), name); err != nil { - return err - } - d.InotifyWithParent(ctx, linux.IN_ATTRIB, 0, vfs.InodeEvent) - return nil + return fd.dentry().removeXattr(ctx, auth.CredentialsFromContext(ctx), name) } // LockBSD implements vfs.FileDescriptionImpl.LockBSD. diff --git a/pkg/sentry/fsimpl/overlay/directory.go b/pkg/sentry/fsimpl/overlay/directory.go index 2ff25e170..1119f1af6 100644 --- a/pkg/sentry/fsimpl/overlay/directory.go +++ b/pkg/sentry/fsimpl/overlay/directory.go @@ -115,8 +115,6 @@ func (fd *directoryFD) Release(ctx context.Context) { // IterDirents implements vfs.FileDescriptionImpl.IterDirents. func (fd *directoryFD) IterDirents(ctx context.Context, cb vfs.IterDirentsCallback) error { d := fd.dentry() - defer d.InotifyWithParent(ctx, linux.IN_ACCESS, 0, vfs.PathEvent) - fd.mu.Lock() defer fd.mu.Unlock() diff --git a/pkg/sentry/fsimpl/overlay/overlay.go b/pkg/sentry/fsimpl/overlay/overlay.go index eb955c09c..f28f5e456 100644 --- a/pkg/sentry/fsimpl/overlay/overlay.go +++ b/pkg/sentry/fsimpl/overlay/overlay.go @@ -845,31 +845,15 @@ func (fd *fileDescription) GetXattr(ctx context.Context, opts vfs.GetXattrOption // SetXattr implements vfs.FileDescriptionImpl.SetXattr. func (fd *fileDescription) SetXattr(ctx context.Context, opts vfs.SetXattrOptions) error { fs := fd.filesystem() - d := fd.dentry() - fs.renameMu.RLock() - err := fs.setXattrLocked(ctx, d, fd.vfsfd.Mount(), auth.CredentialsFromContext(ctx), &opts) - fs.renameMu.RUnlock() - if err != nil { - return err - } - - d.InotifyWithParent(ctx, linux.IN_ATTRIB, 0, vfs.InodeEvent) - return nil + defer fs.renameMu.RUnlock() + return fs.setXattrLocked(ctx, fd.dentry(), fd.vfsfd.Mount(), auth.CredentialsFromContext(ctx), &opts) } // RemoveXattr implements vfs.FileDescriptionImpl.RemoveXattr. func (fd *fileDescription) RemoveXattr(ctx context.Context, name string) error { fs := fd.filesystem() - d := fd.dentry() - fs.renameMu.RLock() - err := fs.removeXattrLocked(ctx, d, fd.vfsfd.Mount(), auth.CredentialsFromContext(ctx), name) - fs.renameMu.RUnlock() - if err != nil { - return err - } - - d.InotifyWithParent(ctx, linux.IN_ATTRIB, 0, vfs.InodeEvent) - return nil + defer fs.renameMu.RUnlock() + return fs.removeXattrLocked(ctx, fd.dentry(), fd.vfsfd.Mount(), auth.CredentialsFromContext(ctx), name) } diff --git a/pkg/sentry/fsimpl/overlay/regular_file.go b/pkg/sentry/fsimpl/overlay/regular_file.go index ce56b48e8..2e6f5d255 100644 --- a/pkg/sentry/fsimpl/overlay/regular_file.go +++ b/pkg/sentry/fsimpl/overlay/regular_file.go @@ -191,7 +191,6 @@ func (fd *regularFileFD) SetStat(ctx context.Context, opts vfs.SetStatOptions) e // Changing owners or truncating may clear one or both of the setuid and // setgid bits, so we may have to update opts before setting d.mode. - inotifyMask := opts.Stat.Mask if opts.Stat.Mask&(linux.STATX_UID|linux.STATX_GID|linux.STATX_SIZE) != 0 { stat, err := wrappedFD.Stat(ctx, vfs.StatOptions{ Mask: linux.STATX_MODE, @@ -201,16 +200,9 @@ func (fd *regularFileFD) SetStat(ctx context.Context, opts vfs.SetStatOptions) e } opts.Stat.Mode = stat.Mode opts.Stat.Mask |= linux.STATX_MODE - // Don't generate inotify IN_ATTRIB for size-only changes (truncations). - if opts.Stat.Mask&(linux.STATX_UID|linux.STATX_GID) != 0 { - inotifyMask |= linux.STATX_MODE - } } d.updateAfterSetStatLocked(&opts) - if ev := vfs.InotifyEventFromStatMask(inotifyMask); ev != 0 { - d.InotifyWithParent(ctx, ev, 0, vfs.InodeEvent) - } return nil } diff --git a/pkg/sentry/fsimpl/tmpfs/directory.go b/pkg/sentry/fsimpl/tmpfs/directory.go index 49c173970..1383a1043 100644 --- a/pkg/sentry/fsimpl/tmpfs/directory.go +++ b/pkg/sentry/fsimpl/tmpfs/directory.go @@ -117,8 +117,6 @@ func (fd *directoryFD) IterDirents(ctx context.Context, cb vfs.IterDirentsCallba fs := fd.filesystem() dir := fd.inode().impl.(*directory) - defer fd.dentry().InotifyWithParent(ctx, linux.IN_ACCESS, 0, vfs.PathEvent) - // fs.mu is required to read d.parent and dentry.name. fs.mu.RLock() defer fs.mu.RUnlock() diff --git a/pkg/sentry/fsimpl/tmpfs/tmpfs.go b/pkg/sentry/fsimpl/tmpfs/tmpfs.go index e9fb3b90f..57922fa97 100644 --- a/pkg/sentry/fsimpl/tmpfs/tmpfs.go +++ b/pkg/sentry/fsimpl/tmpfs/tmpfs.go @@ -887,16 +887,7 @@ func (fd *fileDescription) Stat(ctx context.Context, opts vfs.StatOptions) (linu // SetStat implements vfs.FileDescriptionImpl.SetStat. func (fd *fileDescription) SetStat(ctx context.Context, opts vfs.SetStatOptions) error { - creds := auth.CredentialsFromContext(ctx) - d := fd.dentry() - if err := d.inode.setStat(ctx, creds, &opts); err != nil { - return err - } - - if ev := vfs.InotifyEventFromStatMask(opts.Stat.Mask); ev != 0 { - d.InotifyWithParent(ctx, ev, 0, vfs.InodeEvent) - } - return nil + return fd.dentry().inode.setStat(ctx, auth.CredentialsFromContext(ctx), &opts) } // StatFS implements vfs.FileDescriptionImpl.StatFS. @@ -916,26 +907,12 @@ func (fd *fileDescription) GetXattr(ctx context.Context, opts vfs.GetXattrOption // SetXattr implements vfs.FileDescriptionImpl.SetXattr. func (fd *fileDescription) SetXattr(ctx context.Context, opts vfs.SetXattrOptions) error { - d := fd.dentry() - if err := d.inode.setXattr(auth.CredentialsFromContext(ctx), &opts); err != nil { - return err - } - - // Generate inotify events. - d.InotifyWithParent(ctx, linux.IN_ATTRIB, 0, vfs.InodeEvent) - return nil + return fd.dentry().inode.setXattr(auth.CredentialsFromContext(ctx), &opts) } // RemoveXattr implements vfs.FileDescriptionImpl.RemoveXattr. func (fd *fileDescription) RemoveXattr(ctx context.Context, name string) error { - d := fd.dentry() - if err := d.inode.removeXattr(auth.CredentialsFromContext(ctx), name); err != nil { - return err - } - - // Generate inotify events. - d.InotifyWithParent(ctx, linux.IN_ATTRIB, 0, vfs.InodeEvent) - return nil + return fd.dentry().inode.removeXattr(auth.CredentialsFromContext(ctx), name) } // Sync implements vfs.FileDescriptionImpl.Sync. It does nothing because all diff --git a/pkg/sentry/vfs/file_description.go b/pkg/sentry/vfs/file_description.go index c210a52b8..a19773076 100644 --- a/pkg/sentry/vfs/file_description.go +++ b/pkg/sentry/vfs/file_description.go @@ -548,7 +548,13 @@ func (fd *FileDescription) SetStat(ctx context.Context, opts SetStatOptions) err rp.Release(ctx) return err } - return fd.impl.SetStat(ctx, opts) + if err := fd.impl.SetStat(ctx, opts); err != nil { + return err + } + if ev := InotifyEventFromStatMask(opts.Stat.Mask); ev != 0 { + fd.Dentry().InotifyWithParent(ctx, ev, 0, InodeEvent) + } + return nil } // StatFS returns metadata for the filesystem containing the file represented @@ -673,6 +679,7 @@ func (fd *FileDescription) Write(ctx context.Context, src usermem.IOSequence, op // IterDirents has been called since the last call to Seek, it continues // iteration from the end of the last call. func (fd *FileDescription) IterDirents(ctx context.Context, cb IterDirentsCallback) error { + defer fd.Dentry().InotifyWithParent(ctx, linux.IN_ACCESS, 0, PathEvent) return fd.impl.IterDirents(ctx, cb) } @@ -760,7 +767,11 @@ func (fd *FileDescription) SetXattr(ctx context.Context, opts *SetXattrOptions) rp.Release(ctx) return err } - return fd.impl.SetXattr(ctx, *opts) + if err := fd.impl.SetXattr(ctx, *opts); err != nil { + return err + } + fd.Dentry().InotifyWithParent(ctx, linux.IN_ATTRIB, 0, InodeEvent) + return nil } // RemoveXattr removes the given extended attribute from the file represented @@ -776,7 +787,11 @@ func (fd *FileDescription) RemoveXattr(ctx context.Context, name string) error { rp.Release(ctx) return err } - return fd.impl.RemoveXattr(ctx, name) + if err := fd.impl.RemoveXattr(ctx, name); err != nil { + return err + } + fd.Dentry().InotifyWithParent(ctx, linux.IN_ATTRIB, 0, InodeEvent) + return nil } // SyncFS instructs the filesystem containing fd to execute the semantics of