diff options
author | Fabricio Voznika <fvoznika@google.com> | 2019-01-31 12:53:00 -0800 |
---|---|---|
committer | Shentubot <shentubot@google.com> | 2019-01-31 12:54:00 -0800 |
commit | a497f5ed5f97e4ad49ed60dd46f0146ae45eefd6 (patch) | |
tree | 8673e58e90036bc078eca13b548db123ae0a0c12 | |
parent | f1c1ee8a8ea6c0035aa799af3d7f5733fa2275d0 (diff) |
Invalidate COW mappings when file is truncated
This changed required making fsutil.HostMappable use
a backing file to ensure the correct FD would be used
for read/write operations.
RELNOTES: relnotes is needed for the parent CL.
PiperOrigin-RevId: 231836164
Change-Id: I8ae9639715529874ea7d80a65e2c711a5b4ce254
-rw-r--r-- | pkg/sentry/fs/fsutil/BUILD | 1 | ||||
-rw-r--r-- | pkg/sentry/fs/fsutil/host_mappable.go | 104 | ||||
-rw-r--r-- | pkg/sentry/fs/fsutil/host_mappable_state.go | 22 | ||||
-rw-r--r-- | pkg/sentry/fs/gofer/file.go | 3 | ||||
-rw-r--r-- | pkg/sentry/fs/gofer/inode.go | 6 | ||||
-rw-r--r-- | pkg/sentry/fs/gofer/session.go | 17 |
6 files changed, 91 insertions, 62 deletions
diff --git a/pkg/sentry/fs/fsutil/BUILD b/pkg/sentry/fs/fsutil/BUILD index 7dff970ea..d41fc17cc 100644 --- a/pkg/sentry/fs/fsutil/BUILD +++ b/pkg/sentry/fs/fsutil/BUILD @@ -71,7 +71,6 @@ go_library( "host_file_mapper_state.go", "host_file_mapper_unsafe.go", "host_mappable.go", - "host_mappable_state.go", "inode.go", "inode_cached.go", ], diff --git a/pkg/sentry/fs/fsutil/host_mappable.go b/pkg/sentry/fs/fsutil/host_mappable.go index 4e4bcf4a4..340f8d288 100644 --- a/pkg/sentry/fs/fsutil/host_mappable.go +++ b/pkg/sentry/fs/fsutil/host_mappable.go @@ -15,57 +15,50 @@ package fsutil import ( + "math" "sync" "gvisor.googlesource.com/gvisor/pkg/sentry/context" + "gvisor.googlesource.com/gvisor/pkg/sentry/fs" "gvisor.googlesource.com/gvisor/pkg/sentry/memmap" "gvisor.googlesource.com/gvisor/pkg/sentry/platform" "gvisor.googlesource.com/gvisor/pkg/sentry/safemem" "gvisor.googlesource.com/gvisor/pkg/sentry/usermem" ) -// HostMappable implements memmap.Mappable and platform.File over an arbitrary -// host file descriptor. +// HostMappable implements memmap.Mappable and platform.File over a +// CachedFileObject. +// +// Lock order (compare the lock order model in mm/mm.go): +// truncateMu ("fs locks") +// mu ("memmap.Mappable locks not taken by Translate") +// ("platform.File locks") +// backingFile ("CachedFileObject locks") // // +stateify savable type HostMappable struct { hostFileMapper *HostFileMapper - mu sync.Mutex `state:"nosave"` + backingFile CachedFileObject - // fd is the file descriptor to the host. Protected by mu. - fd int `state:"nosave"` + mu sync.Mutex `state:"nosave"` // mappings tracks mappings of the cached file object into // memmap.MappingSpaces so it can invalidated upon save. Protected by mu. mappings memmap.MappingSet + + // truncateMu protects writes and truncations. See Truncate() for details. + truncateMu sync.RWMutex `state:"nosave"` } // NewHostMappable creates a new mappable that maps directly to host FD. -func NewHostMappable() *HostMappable { +func NewHostMappable(backingFile CachedFileObject) *HostMappable { return &HostMappable{ hostFileMapper: NewHostFileMapper(), - fd: -1, + backingFile: backingFile, } } -func (h *HostMappable) getFD() int { - h.mu.Lock() - defer h.mu.Unlock() - if h.fd < 0 { - panic("HostMappable FD isn't set") - } - return h.fd -} - -// UpdateFD sets the host FD iff FD hasn't been set before or if there are -// no mappings. -func (h *HostMappable) UpdateFD(fd int) { - h.mu.Lock() - defer h.mu.Unlock() - h.fd = fd -} - // AddMapping implements memmap.Mappable.AddMapping. func (h *HostMappable) AddMapping(ctx context.Context, ms memmap.MappingSpace, ar usermem.AddrRange, offset uint64, writable bool) error { // Hot path. Avoid defers. @@ -115,12 +108,12 @@ func (h *HostMappable) InvalidateUnsavable(ctx context.Context) error { // MapInto implements platform.File.MapInto. func (h *HostMappable) MapInto(as platform.AddressSpace, addr usermem.Addr, fr platform.FileRange, at usermem.AccessType, precommit bool) error { - return as.MapFile(addr, h.getFD(), fr, at, precommit) + return as.MapFile(addr, h.backingFile.FD(), fr, at, precommit) } // MapInternal implements platform.File.MapInternal. func (h *HostMappable) MapInternal(fr platform.FileRange, at usermem.AccessType) (safemem.BlockSeq, error) { - return h.hostFileMapper.MapInternal(fr, h.getFD(), at.Write) + return h.hostFileMapper.MapInternal(fr, h.backingFile.FD(), at.Write) } // IncRef implements platform.File.IncRef. @@ -134,3 +127,62 @@ func (h *HostMappable) DecRef(fr platform.FileRange) { mr := memmap.MappableRange{Start: fr.Start, End: fr.End} h.hostFileMapper.DecRefOn(mr) } + +// Truncate truncates the file, invalidating any mapping that may have been +// removed after the size change. +// +// Truncation and writes are synchronized to prevent races where writes make the +// file grow between truncation and invalidation below: +// T1: Calls SetMaskedAttributes and stalls +// T2: Appends to file causing it to grow +// T2: Writes to mapped pages and COW happens +// T1: Continues and wronly invalidates the page mapped in step above. +func (h *HostMappable) Truncate(ctx context.Context, newSize int64) error { + h.truncateMu.Lock() + defer h.truncateMu.Unlock() + + mask := fs.AttrMask{Size: true} + attr := fs.UnstableAttr{Size: newSize} + if err := h.backingFile.SetMaskedAttributes(ctx, mask, attr); err != nil { + return err + } + + // Invalidate COW mappings that may exist beyond the new size in case the file + // is being shrunk. Other mappinsg don't need to be invalidated because + // translate will just return identical mappings after invalidation anyway, + // and SIGBUS will be raised and handled when the mappings are touched. + // + // Compare Linux's mm/truncate.c:truncate_setsize() => + // truncate_pagecache() => + // mm/memory.c:unmap_mapping_range(evencows=1). + h.mu.Lock() + defer h.mu.Unlock() + mr := memmap.MappableRange{ + Start: fs.OffsetPageEnd(newSize), + End: fs.OffsetPageEnd(math.MaxInt64), + } + h.mappings.Invalidate(mr, memmap.InvalidateOpts{InvalidatePrivate: true}) + + return nil +} + +// Write writes to the file backing this mappable. +func (h *HostMappable) Write(ctx context.Context, src usermem.IOSequence, offset int64) (int64, error) { + h.truncateMu.RLock() + n, err := src.CopyInTo(ctx, &writer{ctx: ctx, hostMappable: h, off: offset}) + h.truncateMu.RUnlock() + return n, err +} + +type writer struct { + ctx context.Context + hostMappable *HostMappable + off int64 +} + +// WriteFromBlocks implements safemem.Writer.WriteFromBlocks. +func (w *writer) WriteFromBlocks(src safemem.BlockSeq) (uint64, error) { + n, err := w.hostMappable.backingFile.WriteFromBlocksAt(w.ctx, src, uint64(w.off)) + w.off += int64(n) + return n, err +} diff --git a/pkg/sentry/fs/fsutil/host_mappable_state.go b/pkg/sentry/fs/fsutil/host_mappable_state.go deleted file mode 100644 index 765f1ec87..000000000 --- a/pkg/sentry/fs/fsutil/host_mappable_state.go +++ /dev/null @@ -1,22 +0,0 @@ -// Copyright 2019 Google LLC -// -// 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 fsutil - -// afterLoad is invoked by stateify. -func (h *HostMappable) afterLoad() { - h.mu.Lock() - defer h.mu.Unlock() - h.fd = -1 -} diff --git a/pkg/sentry/fs/gofer/file.go b/pkg/sentry/fs/gofer/file.go index 2181ddc68..2bb25daf1 100644 --- a/pkg/sentry/fs/gofer/file.go +++ b/pkg/sentry/fs/gofer/file.go @@ -215,6 +215,9 @@ func (f *fileOperations) Write(ctx context.Context, file *fs.File, src usermem.I } return n, err } + if f.inodeOperations.fileState.hostMappable != nil { + return f.inodeOperations.fileState.hostMappable.Write(ctx, src, offset) + } return src.CopyInTo(ctx, f.handles.readWriterAt(ctx, offset)) } diff --git a/pkg/sentry/fs/gofer/inode.go b/pkg/sentry/fs/gofer/inode.go index 043705c58..1dc0ca0db 100644 --- a/pkg/sentry/fs/gofer/inode.go +++ b/pkg/sentry/fs/gofer/inode.go @@ -170,9 +170,6 @@ func (i *inodeFileState) setHandlesForCachedIO(flags fs.FileFlags, h *handles) { i.writebackRW = true } } - if i.hostMappable != nil { - i.hostMappable.UpdateFD(i.fdLocked()) - } } // getCachedHandles returns any cached handles which would accelerate @@ -520,6 +517,9 @@ func (i *inodeOperations) Truncate(ctx context.Context, inode *fs.Inode, length if i.session().cachePolicy.useCachingInodeOps(inode) { return i.cachingInodeOps.Truncate(ctx, inode, length) } + if i.session().cachePolicy == cacheRemoteRevalidating { + return i.fileState.hostMappable.Truncate(ctx, length) + } return i.fileState.file.setAttr(ctx, p9.SetAttrMask{Size: true}, p9.SetAttr{Size: uint64(length)}) } diff --git a/pkg/sentry/fs/gofer/session.go b/pkg/sentry/fs/gofer/session.go index b5b1c8202..d626b86f5 100644 --- a/pkg/sentry/fs/gofer/session.go +++ b/pkg/sentry/fs/gofer/session.go @@ -197,17 +197,14 @@ func newInodeOperations(ctx context.Context, s *session, file contextFile, qid p } } - var hm *fsutil.HostMappable - if s.cachePolicy == cacheRemoteRevalidating && fs.IsFile(sattr) { - hm = fsutil.NewHostMappable() - } - fileState := &inodeFileState{ - s: s, - file: file, - sattr: sattr, - key: deviceKey, - hostMappable: hm, + s: s, + file: file, + sattr: sattr, + key: deviceKey, + } + if s.cachePolicy == cacheRemoteRevalidating && fs.IsFile(sattr) { + fileState.hostMappable = fsutil.NewHostMappable(fileState) } uattr := unstable(ctx, valid, attr, s.mounter, s.client) |