From ea4f0073d4a195c05199e6b56fc6b9e72e755234 Mon Sep 17 00:00:00 2001 From: Jing Chen Date: Tue, 24 Oct 2023 01:40:24 -0700 Subject: [PATCH] Fix data race between getting mount options and updating mount options. It is not ideal to access a mount's Flags directly, gVisor will access a mount's flags via MountOptions() which locks mount's mu to avoid data race. PiperOrigin-RevId: 576059331 --- pkg/sentry/fsimpl/gofer/time.go | 4 +- pkg/sentry/fsimpl/kernfs/inode_impl_util.go | 2 +- pkg/sentry/fsimpl/tmpfs/tmpfs.go | 2 +- pkg/sentry/vfs/mount.go | 47 +++++++++++++-------- pkg/sentry/vfs/vfs.go | 2 +- 5 files changed, 34 insertions(+), 23 deletions(-) diff --git a/pkg/sentry/fsimpl/gofer/time.go b/pkg/sentry/fsimpl/gofer/time.go index e9c791110..0c58172b1 100644 --- a/pkg/sentry/fsimpl/gofer/time.go +++ b/pkg/sentry/fsimpl/gofer/time.go @@ -30,7 +30,7 @@ func dentryTimestampFromUnix(t unix.Timespec) int64 { // Preconditions: d.cachedMetadataAuthoritative() == true. func (d *dentry) touchAtime(mnt *vfs.Mount) { - if mnt.Flags.NoATime || mnt.ReadOnly() { + if opts := mnt.Options(); opts.Flags.NoATime || opts.ReadOnly { return } if err := mnt.CheckBeginWrite(); err != nil { @@ -46,7 +46,7 @@ func (d *dentry) touchAtime(mnt *vfs.Mount) { // Preconditions: d.metadataMu is locked. d.cachedMetadataAuthoritative() == true. func (d *dentry) touchAtimeLocked(mnt *vfs.Mount) { - if mnt.Flags.NoATime || mnt.ReadOnly() { + if opts := mnt.Options(); opts.Flags.NoATime || opts.ReadOnly { return } if err := mnt.CheckBeginWrite(); err != nil { diff --git a/pkg/sentry/fsimpl/kernfs/inode_impl_util.go b/pkg/sentry/fsimpl/kernfs/inode_impl_util.go index 7cb23b668..13b7f1142 100644 --- a/pkg/sentry/fsimpl/kernfs/inode_impl_util.go +++ b/pkg/sentry/fsimpl/kernfs/inode_impl_util.go @@ -255,7 +255,7 @@ func (a *InodeAttrs) Links() uint32 { // TouchAtime updates a.atime to the current time. func (a *InodeAttrs) TouchAtime(ctx context.Context, mnt *vfs.Mount) { - if mnt.Flags.NoATime || mnt.ReadOnly() { + if opts := mnt.Options(); opts.Flags.NoATime || opts.ReadOnly { return } if err := mnt.CheckBeginWrite(); err != nil { diff --git a/pkg/sentry/fsimpl/tmpfs/tmpfs.go b/pkg/sentry/fsimpl/tmpfs/tmpfs.go index 29ce64050..06dcf0bf2 100644 --- a/pkg/sentry/fsimpl/tmpfs/tmpfs.go +++ b/pkg/sentry/fsimpl/tmpfs/tmpfs.go @@ -822,7 +822,7 @@ func (i *inode) isDir() bool { } func (i *inode) touchAtime(mnt *vfs.Mount) { - if mnt.Flags.NoATime { + if mnt.Options().Flags.NoATime { return } if err := mnt.CheckBeginWrite(); err != nil { diff --git a/pkg/sentry/vfs/mount.go b/pkg/sentry/vfs/mount.go index 275423dd2..5d932540c 100644 --- a/pkg/sentry/vfs/mount.go +++ b/pkg/sentry/vfs/mount.go @@ -64,8 +64,9 @@ type Mount struct { ID uint64 // Flags contains settings as specified for mount(2), e.g. MS_NOEXEC, except - // for MS_RDONLY which is tracked in "writers". Immutable. - Flags MountFlags + // for MS_RDONLY which is tracked in "writers". flags is protected by + // VirtualFilesystem.mountMu. + flags MountFlags // key is protected by VirtualFilesystem.mountMu and // VirtualFilesystem.mounts.seq, and may be nil. References are held on @@ -123,7 +124,7 @@ type Mount struct { func newMount(vfs *VirtualFilesystem, fs *Filesystem, root *Dentry, mntns *MountNamespace, opts *MountOptions) *Mount { mnt := &Mount{ ID: vfs.lastMountID.Add(1), - Flags: opts.Flags, + flags: opts.Flags, vfs: vfs, fs: fs, root: root, @@ -144,8 +145,8 @@ func (mnt *Mount) Options() MountOptions { mnt.vfs.lockMounts() defer mnt.vfs.unlockMounts(context.Background()) return MountOptions{ - Flags: mnt.Flags, - ReadOnly: mnt.ReadOnly(), + Flags: mnt.flags, + ReadOnly: mnt.ReadOnlyLocked(), } } @@ -160,7 +161,7 @@ func (mnt *Mount) setMountOptions(opts *MountOptions) error { if err := mnt.setReadOnlyLocked(opts.ReadOnly); err != nil { return err } - mnt.Flags = opts.Flags + mnt.flags = opts.Flags return nil } @@ -169,19 +170,19 @@ func (mnt *Mount) MountFlags() uint64 { mnt.vfs.lockMounts() defer mnt.vfs.unlockMounts(context.Background()) var flags uint64 - if mnt.Flags.NoExec { + if mnt.flags.NoExec { flags |= linux.ST_NOEXEC } - if mnt.Flags.NoATime { + if mnt.flags.NoATime { flags |= linux.ST_NOATIME } - if mnt.Flags.NoDev { + if mnt.flags.NoDev { flags |= linux.ST_NODEV } - if mnt.Flags.NoSUID { + if mnt.flags.NoSUID { flags |= linux.ST_NOSUID } - if mnt.ReadOnly() { + if mnt.ReadOnlyLocked() { flags |= linux.ST_RDONLY } return flags @@ -391,8 +392,8 @@ func (vfs *VirtualFilesystem) cloneMount(mnt *Mount, root *Dentry, mopts *MountO opts := mopts if opts == nil { opts = &MountOptions{ - Flags: mnt.Flags, - ReadOnly: mnt.ReadOnly(), + Flags: mnt.flags, + ReadOnly: mnt.ReadOnlyLocked(), } } clone := vfs.NewDisconnectedMount(mnt.fs, root, opts) @@ -1145,6 +1146,15 @@ func (mnt *Mount) setReadOnlyLocked(ro bool) error { // ReadOnly returns true if mount is readonly. func (mnt *Mount) ReadOnly() bool { + mnt.vfs.lockMounts() + defer mnt.vfs.unlockMounts(context.Background()) + return mnt.writers.Load() < 0 +} + +// ReadOnlyLocked returns true if mount is readonly. +// +// Preconditions: VirtualFilesystem.mountMu must be locked. +func (mnt *Mount) ReadOnlyLocked() bool { return mnt.writers.Load() < 0 } @@ -1224,14 +1234,15 @@ func (vfs *VirtualFilesystem) GenerateProcMounts(ctx context.Context, taskRootDi break } + mntOpts := mnt.Options() opts := "rw" - if mnt.ReadOnly() { + if mntOpts.ReadOnly { opts = "ro" } - if mnt.Flags.NoATime { + if mntOpts.Flags.NoATime { opts = ",noatime" } - if mnt.Flags.NoExec { + if mntOpts.Flags.NoExec { opts += ",noexec" } if mopts := mnt.fs.Impl().MountOptions(); mopts != "" { @@ -1345,10 +1356,10 @@ func (vfs *VirtualFilesystem) GenerateProcMountInfo(ctx context.Context, taskRoo if mnt.ReadOnly() { opts = "ro" } - if mnt.Flags.NoATime { + if mnt.flags.NoATime { opts = ",noatime" } - if mnt.Flags.NoExec { + if mnt.flags.NoExec { opts += ",noexec" } fmt.Fprintf(buf, "%s ", opts) diff --git a/pkg/sentry/vfs/vfs.go b/pkg/sentry/vfs/vfs.go index 9834460ab..b096ea772 100644 --- a/pkg/sentry/vfs/vfs.go +++ b/pkg/sentry/vfs/vfs.go @@ -477,7 +477,7 @@ func (vfs *VirtualFilesystem) OpenAt(ctx context.Context, creds *auth.Credential rp.Release(ctx) if opts.FileExec { - if fd.Mount().Flags.NoExec { + if fd.Mount().Options().Flags.NoExec { fd.DecRef(ctx) return nil, linuxerr.EACCES }