Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions pkg/safemem/io.go
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,20 @@ func (r ToIOReader) Read(dst []byte) (int, error) {
return int(n), err
}

// ToIOWriter implements io.Writer for a (safemem.)Writer.
//
// ToIOWriter will return a successful partial write iff Writer.WriteFromBlocks
// does so.
type ToIOWriter struct {
Writer Writer
}

// Write implements io.Writer.Write.
func (w ToIOWriter) Write(src []byte) (int, error) {
n, err := w.Writer.WriteFromBlocks(BlockSeqOf(BlockFromSafeSlice(src)))
return int(n), err
}

// FromIOReader implements Reader for an io.Reader by repeatedly invoking
// io.Reader.Read until it returns an error or partial read. This is not
// thread-safe.
Expand Down
14 changes: 14 additions & 0 deletions pkg/safemem/io_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -197,3 +197,17 @@ func TestWriteFullToBlocks(t *testing.T) {
t.Errorf("dst: got %q, wanted %q", got, want)
}
}

func TestToIOWriter(t *testing.T) {
dsts := makeBlocks(make([]byte, 3), make([]byte, 3))
w := ToIOWriter{&BlockSeqWriter{BlockSeqFromSlice(dsts)}}
n, err := w.Write([]byte("foobar"))
if wantN := 6; n != wantN || err != nil {
t.Errorf("Write: got (%v, %v), wanted (%v, nil)", n, err, wantN)
}
for i, want := range [][]byte{[]byte("foo"), []byte("bar")} {
if got := dsts[i].ToSlice(); !bytes.Equal(got, want) {
t.Errorf("dsts[%d]: got %q, wanted %q", i, got, want)
}
}
}
27 changes: 9 additions & 18 deletions pkg/sentry/fsimpl/tmpfs/fscheckpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -114,11 +114,10 @@ func (cb *fsckptTarWriterCallbacks) regularFileWrite(ctx context.Context, rf *re
// fsckptTarReaderCallbacks implements tarReaderCallbacks by storing MemoryFile
// offsets containing regular file data in the tar archive.
type fsckptTarReaderCallbacks struct {
fs *filesystem
regularFiles map[*tar.Header]*fsckptRegularFile
fs *filesystem
}

func (cb *fsckptTarReaderCallbacks) regularFileRead(ctx context.Context, hdr *tar.Header, tr *tar.Reader) error {
func (cb *fsckptTarReaderCallbacks) regularFileRead(ctx context.Context, hdr *tar.Header, tr *tar.Reader, rf *regularFile) error {
if hdr.Size < 8 {
return fmt.Errorf("header size %d too small for regular file size", hdr.Size)
}
Expand All @@ -131,29 +130,21 @@ func (cb *fsckptTarReaderCallbacks) regularFileRead(ctx context.Context, hdr *ta
if _, err := io.ReadFull(tr, buf[:]); err != nil {
return fmt.Errorf("failed to read file size from tar: %w", err)
}
crf := &fsckptRegularFile{
size: binary.LittleEndian.Uint64(buf[:]),
data: make([]fsckptRegularFileSegment, remSize/segSize),
}
if _, err := ReadCheckpointRegularFileSegmentSlice(tr, crf.data); err != nil {
size := binary.LittleEndian.Uint64(buf[:])
segs := make([]fsckptRegularFileSegment, remSize/segSize)
if _, err := ReadCheckpointRegularFileSegmentSlice(tr, segs); err != nil {
return fmt.Errorf("failed to read file segments from tar: %w", err)
}
cb.regularFiles[hdr] = crf
return nil
}

func (cb *fsckptTarReaderCallbacks) regularFileSetContents(ctx context.Context, hdr *tar.Header, rf *regularFile) error {
crf := cb.regularFiles[hdr]
rf.inode.mu.Lock()
defer rf.inode.mu.Unlock()
rf.dataMu.Lock()
defer rf.dataMu.Unlock()
rf.size.Store(uint64(crf.size))
rf.size.Store(size)
gap := rf.data.FirstGap()
n := uint64(0)
for _, rfseg := range crf.data {
gap = rf.data.Insert(gap, memmap.MappableRange{rfseg.Start, rfseg.End}, rfseg.Value).NextGap()
n += (rfseg.End - rfseg.Start) / hostarch.PageSize
for _, seg := range segs {
gap = rf.data.Insert(gap, memmap.MappableRange{seg.Start, seg.End}, seg.Value).NextGap()
n += (seg.End - seg.Start) / hostarch.PageSize
}
if !cb.fs.accountPages(n) {
return fmt.Errorf("restored filesystem would exceed size limit of %d pages", cb.fs.maxSizeInPages)
Expand Down
107 changes: 44 additions & 63 deletions pkg/sentry/fsimpl/tmpfs/tar.go

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only fixes the OOM issue on the checkpoint writing side. But on restore, we have the same bug. tarDefaultReaderCallbacks.regularFileRead copies into bytes.Buffer per file. Then this is later copied into file via mknodFromTar() => regularFileSetContents(). Restoring a rootfs tar therefore allocates the sum of all regular file sizes in the sentry. And also whatever extra capacity bytes.Buffer needs since it doubles its size. We need to fix that too.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restore side streams now too - added a basically identical writer function to mirror the safemem.ToIOReader func, and added a unit test there

There's a little bit of complexity here, in that the tar stream is sequential, so the content of the stream has to be consumed while the reader is sitting on that entry

What I did is create the regular file inode so it's detached w/ no parent yet, and then copy the entry straight into it through a new safemem.ToIOWriter. After that, we can just have mknodFromTar attach the pre-created inode once all the directories exist, which has the same ordering as before

One simplification we get out of this is we can then collapse th reader callback interface to just 1 regularFileRead(ctx, hdr, tr, rf)

Updated the allocation test so that it covers the restore path now, with that same 1 MiB bound on a 4 MiB file as we talked about above

Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ package tmpfs

import (
"archive/tar"
"bytes"
"encoding/base64"
"fmt"
"io"
Expand All @@ -29,9 +28,9 @@ import (
"gvisor.dev/gvisor/pkg/abi/linux"
"gvisor.dev/gvisor/pkg/context"
"gvisor.dev/gvisor/pkg/log"
"gvisor.dev/gvisor/pkg/safemem"
"gvisor.dev/gvisor/pkg/sentry/kernel/auth"
"gvisor.dev/gvisor/pkg/sentry/vfs"
"gvisor.dev/gvisor/pkg/usermem"
)

// tarRead creates the corresponding dentry and its children from the given
Expand All @@ -56,6 +55,16 @@ func (fs *filesystem) readFromTar(ctx context.Context, tr *tar.Reader, cb tarRea
fileToHeader := map[string]*tar.Header{}
symlinkToHeader := map[string]*tar.Header{}
linkToHeader := map[string]*tar.Header{}
// Regular file contents are streamed from tr into a detached inode as each
// entry is read, since tr is sequential. The inode is attached to its parent
// directory below, once all directories exist.
headerToRegularFile := map[*tar.Header]*inode{}
defer func() {
// Release regular files that were not attached because of an error.
for _, ino := range headerToRegularFile {
ino.decRef(ctx)
}
}()
for {
header, err := tr.Next()
if err != nil {
Expand All @@ -69,9 +78,14 @@ func (fs *filesystem) readFromTar(ctx context.Context, tr *tar.Reader, cb tarRea
case tar.TypeDir:
directoryToHeader[header.Name] = header
case tar.TypeReg:
if err := cb.regularFileRead(ctx, header, tr); err != nil {
ino, err := fs.newRegularFile(auth.KUID(header.Uid), auth.KGID(header.Gid), linux.FileMode(header.Mode), nil /* parentDir */)
if err != nil {
return err
}
headerToRegularFile[header] = ino
if err := cb.regularFileRead(ctx, header, tr, ino.impl.(*regularFile)); err != nil {
return fmt.Errorf("failed to read file %v: %w", header.Name, err)
}
fileToHeader[header.Name] = header
case tar.TypeFifo, tar.TypeBlock, tar.TypeChar:
fileToHeader[header.Name] = header
Expand All @@ -91,9 +105,10 @@ func (fs *filesystem) readFromTar(ctx context.Context, tr *tar.Reader, cb tarRea
}
// Re-create all regular files, FIFOs, block devices, and character devices.
for path, hdr := range fileToHeader {
if err := fs.mknodFromTar(ctx, hdr, pathToInode, cb); err != nil {
if err := fs.mknodFromTar(hdr, pathToInode, headerToRegularFile); err != nil {
return fmt.Errorf("failed to make file %v: %w", path, err)
}
delete(headerToRegularFile, hdr)
}
// Re-create all symlinks.
for path, hdr := range symlinkToHeader {
Expand Down Expand Up @@ -194,10 +209,10 @@ func (fs *filesystem) mkdirFromTar(hdr *tar.Header, pathToInode map[string]*inod
return childDir.dentry.inode, nil
}

// mknodFromTar creates a regular file,FIFO, block device, or character device file using
// the provided header. It also writes the file content to the corresponding regular file if it
// exists.
func (fs *filesystem) mknodFromTar(ctx context.Context, hdr *tar.Header, pathToInode map[string]*inode, cb tarReaderCallbacks) error {
// mknodFromTar creates a FIFO, block device, or character device file using
// the provided header, or attaches the regular file already created for it in
// headerToRegularFile.
func (fs *filesystem) mknodFromTar(hdr *tar.Header, pathToInode map[string]*inode, headerToRegularFile map[*tar.Header]*inode) error {
dir, name := filepath.Split(hdr.Name)
parentInode, ok := pathToInode[dir]
if !ok {
Expand All @@ -211,7 +226,7 @@ func (fs *filesystem) mknodFromTar(ctx context.Context, hdr *tar.Header, pathToI
var err error
switch hdr.Typeflag {
case tar.TypeReg:
childInode, err = fs.newRegularFile(auth.KUID(hdr.Uid), auth.KGID(hdr.Gid), linux.FileMode(hdr.Mode), nil /* parentDir */)
childInode = headerToRegularFile[hdr]
case tar.TypeFifo:
childInode, err = fs.newNamedPipe(auth.KUID(hdr.Uid), auth.KGID(hdr.Gid), linux.FileMode(hdr.Mode), nil /* parentDir */)
case tar.TypeBlock:
Expand All @@ -235,14 +250,6 @@ func (fs *filesystem) mknodFromTar(ctx context.Context, hdr *tar.Header, pathToI
child := fs.newDentry(childInode)
parentDir.insertChildLocked(child, name)
pathToInode[hdr.Name] = childInode

// Write file contents to the corresponding regular files.
if rf, _ := childInode.impl.(*regularFile); rf != nil {
if err := cb.regularFileSetContents(ctx, hdr, rf); err != nil {
return err
}
}

return nil
}

Expand Down Expand Up @@ -311,53 +318,29 @@ func (fs *filesystem) symlinkFromTar(hdr *tar.Header, pathToInode map[string]*in
}

type tarReaderCallbacks interface {
// regularFileRead reads information about the regular file with header hdr
// from tr.
regularFileRead(ctx context.Context, hdr *tar.Header, tr *tar.Reader) error

// regularFileSetContents sets the contents and size of rf, using what was
// previously read for hdr.
regularFileSetContents(ctx context.Context, hdr *tar.Header, rf *regularFile) error
// regularFileRead reads the regular file with header hdr from tr, and sets
// the contents and size of rf accordingly.
regularFileRead(ctx context.Context, hdr *tar.Header, tr *tar.Reader, rf *regularFile) error
}

// tarDefaultReaderCallbacks implements tarReaderCallbacks by reading regular
// file contents from the tar archive.
type tarDefaultReaderCallbacks struct {
headerToContent map[*tar.Header]*bytes.Buffer
}
type tarDefaultReaderCallbacks struct{}

func (cb *tarDefaultReaderCallbacks) regularFileRead(ctx context.Context, hdr *tar.Header, tr *tar.Reader) error {
var buf bytes.Buffer
n, err := io.Copy(&buf, tr)
if err != nil {
return fmt.Errorf("failed to read file content: %w", err)
}
if n != hdr.Size {
return fmt.Errorf("failed to read all file content, got %d bytes, want %d", n, hdr.Size)
}
if hdr.Size > 0 {
cb.headerToContent[hdr] = &buf
}
return nil
}

func (cb *tarDefaultReaderCallbacks) regularFileSetContents(ctx context.Context, hdr *tar.Header, rf *regularFile) error {
buf, ok := cb.headerToContent[hdr]
if !ok {
return nil
}
func (tarDefaultReaderCallbacks) regularFileRead(ctx context.Context, hdr *tar.Header, tr *tar.Reader, rf *regularFile) error {
rf.inode.mu.Lock()
defer rf.inode.mu.Unlock()
src := usermem.BytesIOSequence(buf.Bytes())
rw := getRegularFileReadWriter(rf, 0, 0)
n, err := src.CopyInTo(ctx, rw)
// Copy in fixed-size chunks, so that memory use does not depend on the size
// of the file.
n, err := io.Copy(safemem.ToIOWriter{Writer: rw}, tr)
putRegularFileReadWriter(rw)
if err != nil {
return fmt.Errorf("failed to write file content: %w", err)
}
if size := int64(len(buf.Bytes())); n != size {
return fmt.Errorf("failed to write all file content to %v, got %d bytes, want %d", hdr.Name, n, size)
if n != hdr.Size {
return fmt.Errorf("failed to write all file content, got %d bytes, want %d", n, hdr.Size)
}
putRegularFileReadWriter(rw)
return nil
}

Expand Down Expand Up @@ -534,20 +517,18 @@ func (tarDefaultWriterCallbacks) regularFileWrite(ctx context.Context, rf *regul
// it is safe to lock here to ensure no concurrent writes occur.
rf.inode.mu.Lock()
defer rf.inode.mu.Unlock()
data := make([]byte, rf.size.RacyLoad())
dst := usermem.BytesIOSequence(data)
size := int64(rf.size.RacyLoad())
rw := getRegularFileReadWriter(rf, 0, 0)
n, err := dst.CopyOutFrom(ctx, rw)
putRegularFileReadWriter(rw)
if err != nil && err != io.EOF {
return fmt.Errorf("failed to read file content: %w", err)
}
if n != int64(len(data)) {
return fmt.Errorf("failed to read all file content, got %d bytes, want %d", n, len(data))
}
if _, err := tw.Write(data); err != nil {
defer putRegularFileReadWriter(rw)
// Copy in fixed-size chunks, so that memory use does not depend on the size
// of the file. ReadToBlocks stops at the file size.
n, err := io.Copy(tw, safemem.ToIOReader{Reader: rw})
if err != nil {
return fmt.Errorf("failed to write file content to tar: %w", err)
}
if n != size {
return fmt.Errorf("failed to read all file content, got %d bytes, want %d", n, size)
}
return nil
}

Expand Down
Loading