From 4cc3894b27e8cfddb507b4c68b95695ec3979eba Mon Sep 17 00:00:00 2001 From: Ayush Ranjan Date: Wed, 28 Oct 2020 11:46:18 -0700 Subject: [vfs] Refactor hostfs mmap into kernfs util. PiperOrigin-RevId: 339505487 --- pkg/sentry/fsimpl/host/BUILD | 1 - pkg/sentry/fsimpl/host/host.go | 24 +---- pkg/sentry/fsimpl/host/mmap.go | 133 ------------------------- pkg/sentry/fsimpl/host/save_restore.go | 8 -- pkg/sentry/fsimpl/kernfs/BUILD | 3 + pkg/sentry/fsimpl/kernfs/mmap_util.go | 164 +++++++++++++++++++++++++++++++ pkg/sentry/fsimpl/kernfs/save_restore.go | 23 +++++ 7 files changed, 194 insertions(+), 162 deletions(-) delete mode 100644 pkg/sentry/fsimpl/host/mmap.go create mode 100644 pkg/sentry/fsimpl/kernfs/mmap_util.go create mode 100644 pkg/sentry/fsimpl/kernfs/save_restore.go (limited to 'pkg/sentry') diff --git a/pkg/sentry/fsimpl/host/BUILD b/pkg/sentry/fsimpl/host/BUILD index dc0f86061..4ae9d6d5e 100644 --- a/pkg/sentry/fsimpl/host/BUILD +++ b/pkg/sentry/fsimpl/host/BUILD @@ -33,7 +33,6 @@ go_library( "host.go", "inode_refs.go", "ioctl_unsafe.go", - "mmap.go", "save_restore.go", "socket.go", "socket_iovec.go", diff --git a/pkg/sentry/fsimpl/host/host.go b/pkg/sentry/fsimpl/host/host.go index eeed0f97d..39b902a3e 100644 --- a/pkg/sentry/fsimpl/host/host.go +++ b/pkg/sentry/fsimpl/host/host.go @@ -48,6 +48,7 @@ type inode struct { kernfs.InodeNoStatFS kernfs.InodeNotDirectory kernfs.InodeNotSymlink + kernfs.CachedMappable kernfs.InodeTemporary // This holds no meaning as this inode can't be Looked up and is always valid. locks vfs.FileLocks @@ -96,16 +97,6 @@ type inode struct { // Event queue for blocking operations. queue waiter.Queue - // mapsMu protects mappings. - mapsMu sync.Mutex `state:"nosave"` - - // If this file is mmappable, mappings tracks mappings of hostFD into - // memmap.MappingSpaces. - mappings memmap.MappingSet - - // pf implements platform.File for mappings of hostFD. - pf inodePlatformFile - // If haveBuf is non-zero, hostFD represents a pipe, and buf contains data // read from the pipe from previous calls to inode.beforeSave(). haveBuf // and buf are protected by bufMu. haveBuf is accessed using atomic memory @@ -135,7 +126,7 @@ func newInode(ctx context.Context, fs *filesystem, hostFD int, savable bool, fil isTTY: isTTY, savable: savable, } - i.pf.inode = i + i.CachedMappable.Init(hostFD) i.EnableLeakCheck() // If the hostFD can return EWOULDBLOCK when set to non-blocking, do so and @@ -439,14 +430,7 @@ func (i *inode) SetStat(ctx context.Context, fs *vfs.Filesystem, creds *auth.Cre oldpgend, _ := usermem.PageRoundUp(oldSize) newpgend, _ := usermem.PageRoundUp(s.Size) if oldpgend != newpgend { - i.mapsMu.Lock() - i.mappings.Invalidate(memmap.MappableRange{newpgend, oldpgend}, memmap.InvalidateOpts{ - // Compare Linux's mm/truncate.c:truncate_setsize() => - // truncate_pagecache() => - // mm/memory.c:unmap_mapping_range(evencows=1). - InvalidatePrivate: true, - }) - i.mapsMu.Unlock() + i.CachedMappable.InvalidateRange(memmap.MappableRange{newpgend, oldpgend}) } } } @@ -797,7 +781,7 @@ func (f *fileDescription) ConfigureMMap(_ context.Context, opts *memmap.MMapOpts return syserror.ENODEV } i := f.inode - i.pf.fileMapperInitOnce.Do(i.pf.fileMapper.Init) + i.CachedMappable.InitFileMapperOnce() return vfs.GenericConfigureMMap(&f.vfsfd, i, opts) } diff --git a/pkg/sentry/fsimpl/host/mmap.go b/pkg/sentry/fsimpl/host/mmap.go deleted file mode 100644 index 3d7eb2f96..000000000 --- a/pkg/sentry/fsimpl/host/mmap.go +++ /dev/null @@ -1,133 +0,0 @@ -// Copyright 2020 The gVisor Authors. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package host - -import ( - "gvisor.dev/gvisor/pkg/context" - "gvisor.dev/gvisor/pkg/safemem" - "gvisor.dev/gvisor/pkg/sentry/fs/fsutil" - "gvisor.dev/gvisor/pkg/sentry/memmap" - "gvisor.dev/gvisor/pkg/sync" - "gvisor.dev/gvisor/pkg/usermem" -) - -// inodePlatformFile implements memmap.File. It exists solely because inode -// cannot implement both kernfs.Inode.IncRef and memmap.File.IncRef. -// -// inodePlatformFile should only be used if inode.canMap is true. -// -// +stateify savable -type inodePlatformFile struct { - *inode - - // fdRefsMu protects fdRefs. - fdRefsMu sync.Mutex `state:"nosave"` - - // fdRefs counts references on memmap.File offsets. It is used solely for - // memory accounting. - fdRefs fsutil.FrameRefSet - - // fileMapper caches mappings of the host file represented by this inode. - fileMapper fsutil.HostFileMapper - - // fileMapperInitOnce is used to lazily initialize fileMapper. - fileMapperInitOnce sync.Once `state:"nosave"` -} - -// IncRef implements memmap.File.IncRef. -// -// Precondition: i.inode.canMap must be true. -func (i *inodePlatformFile) IncRef(fr memmap.FileRange) { - i.fdRefsMu.Lock() - i.fdRefs.IncRefAndAccount(fr) - i.fdRefsMu.Unlock() -} - -// DecRef implements memmap.File.DecRef. -// -// Precondition: i.inode.canMap must be true. -func (i *inodePlatformFile) DecRef(fr memmap.FileRange) { - i.fdRefsMu.Lock() - i.fdRefs.DecRefAndAccount(fr) - i.fdRefsMu.Unlock() -} - -// MapInternal implements memmap.File.MapInternal. -// -// Precondition: i.inode.canMap must be true. -func (i *inodePlatformFile) MapInternal(fr memmap.FileRange, at usermem.AccessType) (safemem.BlockSeq, error) { - return i.fileMapper.MapInternal(fr, i.hostFD, at.Write) -} - -// FD implements memmap.File.FD. -func (i *inodePlatformFile) FD() int { - return i.hostFD -} - -// AddMapping implements memmap.Mappable.AddMapping. -// -// Precondition: i.inode.canMap must be true. -func (i *inode) AddMapping(ctx context.Context, ms memmap.MappingSpace, ar usermem.AddrRange, offset uint64, writable bool) error { - i.mapsMu.Lock() - mapped := i.mappings.AddMapping(ms, ar, offset, writable) - for _, r := range mapped { - i.pf.fileMapper.IncRefOn(r) - } - i.mapsMu.Unlock() - return nil -} - -// RemoveMapping implements memmap.Mappable.RemoveMapping. -// -// Precondition: i.inode.canMap must be true. -func (i *inode) RemoveMapping(ctx context.Context, ms memmap.MappingSpace, ar usermem.AddrRange, offset uint64, writable bool) { - i.mapsMu.Lock() - unmapped := i.mappings.RemoveMapping(ms, ar, offset, writable) - for _, r := range unmapped { - i.pf.fileMapper.DecRefOn(r) - } - i.mapsMu.Unlock() -} - -// CopyMapping implements memmap.Mappable.CopyMapping. -// -// Precondition: i.inode.canMap must be true. -func (i *inode) CopyMapping(ctx context.Context, ms memmap.MappingSpace, srcAR, dstAR usermem.AddrRange, offset uint64, writable bool) error { - return i.AddMapping(ctx, ms, dstAR, offset, writable) -} - -// Translate implements memmap.Mappable.Translate. -// -// Precondition: i.inode.canMap must be true. -func (i *inode) Translate(ctx context.Context, required, optional memmap.MappableRange, at usermem.AccessType) ([]memmap.Translation, error) { - mr := optional - return []memmap.Translation{ - { - Source: mr, - File: &i.pf, - Offset: mr.Start, - Perms: usermem.AnyAccess, - }, - }, nil -} - -// InvalidateUnsavable implements memmap.Mappable.InvalidateUnsavable. -// -// Precondition: i.inode.canMap must be true. -func (i *inode) InvalidateUnsavable(ctx context.Context) error { - // We expect the same host fd across save/restore, so all translations - // should be valid. - return nil -} diff --git a/pkg/sentry/fsimpl/host/save_restore.go b/pkg/sentry/fsimpl/host/save_restore.go index 7e32a8863..8800652a9 100644 --- a/pkg/sentry/fsimpl/host/save_restore.go +++ b/pkg/sentry/fsimpl/host/save_restore.go @@ -68,11 +68,3 @@ func (i *inode) afterLoad() { } } } - -// afterLoad is invoked by stateify. -func (i *inodePlatformFile) afterLoad() { - if i.fileMapper.IsInited() { - // Ensure that we don't call i.fileMapper.Init() again. - i.fileMapperInitOnce.Do(func() {}) - } -} diff --git a/pkg/sentry/fsimpl/kernfs/BUILD b/pkg/sentry/fsimpl/kernfs/BUILD index aaad67ab8..6dbc7e34d 100644 --- a/pkg/sentry/fsimpl/kernfs/BUILD +++ b/pkg/sentry/fsimpl/kernfs/BUILD @@ -92,6 +92,8 @@ go_library( "fstree.go", "inode_impl_util.go", "kernfs.go", + "mmap_util.go", + "save_restore.go", "slot_list.go", "static_directory_refs.go", "symlink.go", @@ -106,6 +108,7 @@ go_library( "//pkg/log", "//pkg/refs", "//pkg/refsvfs2", + "//pkg/safemem", "//pkg/sentry/fs/fsutil", "//pkg/sentry/fs/lock", "//pkg/sentry/kernel/auth", diff --git a/pkg/sentry/fsimpl/kernfs/mmap_util.go b/pkg/sentry/fsimpl/kernfs/mmap_util.go new file mode 100644 index 000000000..bd6a134b4 --- /dev/null +++ b/pkg/sentry/fsimpl/kernfs/mmap_util.go @@ -0,0 +1,164 @@ +// Copyright 2020 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package kernfs + +import ( + "gvisor.dev/gvisor/pkg/context" + "gvisor.dev/gvisor/pkg/safemem" + "gvisor.dev/gvisor/pkg/sentry/fs/fsutil" + "gvisor.dev/gvisor/pkg/sentry/memmap" + "gvisor.dev/gvisor/pkg/sync" + "gvisor.dev/gvisor/pkg/usermem" +) + +// inodePlatformFile implements memmap.File. It exists solely because inode +// cannot implement both kernfs.Inode.IncRef and memmap.File.IncRef. +// +// +stateify savable +type inodePlatformFile struct { + // hostFD contains the host fd that this file was originally created from, + // which must be available at time of restore. + // + // This field is initialized at creation time and is immutable. + // inodePlatformFile does not own hostFD and hence should not close it. + hostFD int + + // fdRefsMu protects fdRefs. + fdRefsMu sync.Mutex `state:"nosave"` + + // fdRefs counts references on memmap.File offsets. It is used solely for + // memory accounting. + fdRefs fsutil.FrameRefSet + + // fileMapper caches mappings of the host file represented by this inode. + fileMapper fsutil.HostFileMapper + + // fileMapperInitOnce is used to lazily initialize fileMapper. + fileMapperInitOnce sync.Once `state:"nosave"` +} + +var _ memmap.File = (*inodePlatformFile)(nil) + +// IncRef implements memmap.File.IncRef. +func (i *inodePlatformFile) IncRef(fr memmap.FileRange) { + i.fdRefsMu.Lock() + i.fdRefs.IncRefAndAccount(fr) + i.fdRefsMu.Unlock() +} + +// DecRef implements memmap.File.DecRef. +func (i *inodePlatformFile) DecRef(fr memmap.FileRange) { + i.fdRefsMu.Lock() + i.fdRefs.DecRefAndAccount(fr) + i.fdRefsMu.Unlock() +} + +// MapInternal implements memmap.File.MapInternal. +func (i *inodePlatformFile) MapInternal(fr memmap.FileRange, at usermem.AccessType) (safemem.BlockSeq, error) { + return i.fileMapper.MapInternal(fr, i.hostFD, at.Write) +} + +// FD implements memmap.File.FD. +func (i *inodePlatformFile) FD() int { + return i.hostFD +} + +// CachedMappable implements memmap.Mappable. This utility can be embedded in a +// kernfs.Inode that represents a host file to make the inode mappable. +// CachedMappable caches the mappings of the host file. CachedMappable must be +// initialized (via Init) with a hostFD before use. +// +// +stateify savable +type CachedMappable struct { + // mapsMu protects mappings. + mapsMu sync.Mutex `state:"nosave"` + + // mappings tracks mappings of hostFD into memmap.MappingSpaces. + mappings memmap.MappingSet + + // pf implements memmap.File for mappings backed by a host fd. + pf inodePlatformFile +} + +var _ memmap.Mappable = (*CachedMappable)(nil) + +// Init initializes i.pf. This must be called before using CachedMappable. +func (i *CachedMappable) Init(hostFD int) { + i.pf.hostFD = hostFD +} + +// AddMapping implements memmap.Mappable.AddMapping. +func (i *CachedMappable) AddMapping(ctx context.Context, ms memmap.MappingSpace, ar usermem.AddrRange, offset uint64, writable bool) error { + i.mapsMu.Lock() + mapped := i.mappings.AddMapping(ms, ar, offset, writable) + for _, r := range mapped { + i.pf.fileMapper.IncRefOn(r) + } + i.mapsMu.Unlock() + return nil +} + +// RemoveMapping implements memmap.Mappable.RemoveMapping. +func (i *CachedMappable) RemoveMapping(ctx context.Context, ms memmap.MappingSpace, ar usermem.AddrRange, offset uint64, writable bool) { + i.mapsMu.Lock() + unmapped := i.mappings.RemoveMapping(ms, ar, offset, writable) + for _, r := range unmapped { + i.pf.fileMapper.DecRefOn(r) + } + i.mapsMu.Unlock() +} + +// CopyMapping implements memmap.Mappable.CopyMapping. +func (i *CachedMappable) CopyMapping(ctx context.Context, ms memmap.MappingSpace, srcAR, dstAR usermem.AddrRange, offset uint64, writable bool) error { + return i.AddMapping(ctx, ms, dstAR, offset, writable) +} + +// Translate implements memmap.Mappable.Translate. +func (i *CachedMappable) Translate(ctx context.Context, required, optional memmap.MappableRange, at usermem.AccessType) ([]memmap.Translation, error) { + mr := optional + return []memmap.Translation{ + { + Source: mr, + File: &i.pf, + Offset: mr.Start, + Perms: usermem.AnyAccess, + }, + }, nil +} + +// InvalidateUnsavable implements memmap.Mappable.InvalidateUnsavable. +func (i *CachedMappable) InvalidateUnsavable(ctx context.Context) error { + // We expect the same host fd across save/restore, so all translations + // should be valid. + return nil +} + +// InvalidateRange invalidates the passed range on i.mappings. +func (i *CachedMappable) InvalidateRange(r memmap.MappableRange) { + i.mapsMu.Lock() + i.mappings.Invalidate(r, memmap.InvalidateOpts{ + // Compare Linux's mm/truncate.c:truncate_setsize() => + // truncate_pagecache() => + // mm/memory.c:unmap_mapping_range(evencows=1). + InvalidatePrivate: true, + }) + i.mapsMu.Unlock() +} + +// InitFileMapperOnce initializes the host file mapper. It ensures that the +// file mapper is initialized just once. +func (i *CachedMappable) InitFileMapperOnce() { + i.pf.fileMapperInitOnce.Do(i.pf.fileMapper.Init) +} diff --git a/pkg/sentry/fsimpl/kernfs/save_restore.go b/pkg/sentry/fsimpl/kernfs/save_restore.go new file mode 100644 index 000000000..1f48de6f1 --- /dev/null +++ b/pkg/sentry/fsimpl/kernfs/save_restore.go @@ -0,0 +1,23 @@ +// Copyright 2020 The gVisor Authors. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package kernfs + +// afterLoad is invoked by stateify. +func (i *inodePlatformFile) afterLoad() { + if i.fileMapper.IsInited() { + // Ensure that we don't call i.fileMapper.Init() again. + i.fileMapperInitOnce.Do(func() {}) + } +} -- cgit v1.2.3