diff --git a/pkg/p9/client_file.go b/pkg/p9/client_file.go index 236b230ac..3446c2db4 100644 --- a/pkg/p9/client_file.go +++ b/pkg/p9/client_file.go @@ -763,13 +763,13 @@ func (c *clientFile) UnlinkAt(name string, flags uint32) error { } // Readdir implements File.Readdir. -func (c *clientFile) Readdir(offset uint64, count uint32) ([]Dirent, error) { +func (c *clientFile) Readdir(direntOffset uint64, count uint32) ([]Dirent, error) { if c.closed.Load() != 0 { return nil, unix.EBADF } rreaddir := Rreaddir{} - if err := c.client.sendRecv(&Treaddir{Directory: c.fid, Offset: offset, Count: count}, &rreaddir); err != nil { + if err := c.client.sendRecv(&Treaddir{Directory: c.fid, DirentOffset: direntOffset, Count: count}, &rreaddir); err != nil { return nil, err } diff --git a/pkg/p9/file.go b/pkg/p9/file.go index b209f4ce7..c4a79b4c2 100644 --- a/pkg/p9/file.go +++ b/pkg/p9/file.go @@ -275,10 +275,19 @@ type File interface { // Readdir reads directory entries. // - // This may return io.EOF in addition to unix.Errno values. + // This may return io.EOF in addition to unix.Errno values. count is the + // number of bytes to read. + // + // direntOffset is the directory offset at which the read should happen. + // direntOffset can be set to 0 to start reading the directory from start. + // direntOffset is used more like a cookie. The unit of direntOffset is + // unspecified. Gofers can choose their own unit. The client must set it + // to one of the values returned in Dirent.Offset, preferably the last offset + // returned, which should cause the readdir to continue from where it was + // left off. // // On the server, Readdir has a read concurrency guarantee. - Readdir(offset uint64, count uint32) ([]Dirent, error) + Readdir(direntOffset uint64, count uint32) ([]Dirent, error) // Readlink reads the link target. // diff --git a/pkg/p9/handlers.go b/pkg/p9/handlers.go index a4f1c6f74..c0f5f8b3f 100644 --- a/pkg/p9/handlers.go +++ b/pkg/p9/handlers.go @@ -1093,7 +1093,7 @@ func (t *Treaddir) handle(cs *connState) message { } // Read the entries. - entries, err = ref.file.Readdir(t.Offset, t.Count) + entries, err = ref.file.Readdir(t.DirentOffset, t.Count) if err != nil && err != io.EOF { return err } diff --git a/pkg/p9/messages.go b/pkg/p9/messages.go index 2e7b714b3..044b57a92 100644 --- a/pkg/p9/messages.go +++ b/pkg/p9/messages.go @@ -19,6 +19,7 @@ import ( "math" "gvisor.dev/gvisor/pkg/fd" + "gvisor.dev/gvisor/pkg/log" ) // ErrInvalidMsgType is returned when an unsupported message type is found. @@ -1916,8 +1917,8 @@ type Treaddir struct { // Directory is the directory FID to read. Directory FID - // Offset is the offset to read at. - Offset uint64 + // DirentOffset is the dirent offset to read at. + DirentOffset uint64 // Count is the number of bytes to read. Count uint32 @@ -1926,14 +1927,14 @@ type Treaddir struct { // decode implements encoder.decode. func (t *Treaddir) decode(b *buffer) { t.Directory = b.ReadFID() - t.Offset = b.Read64() + t.DirentOffset = b.Read64() t.Count = b.Read32() } // encode implements encoder.encode. func (t *Treaddir) encode(b *buffer) { b.WriteFID(t.Directory) - b.Write64(t.Offset) + b.Write64(t.DirentOffset) b.Write32(t.Count) } @@ -1944,7 +1945,7 @@ func (*Treaddir) Type() MsgType { // String implements fmt.Stringer. func (t *Treaddir) String() string { - return fmt.Sprintf("Treaddir{DirectoryFID: %d, Offset: %d, Count: %d}", t.Directory, t.Offset, t.Count) + return fmt.Sprintf("Treaddir{DirectoryFID: %d, DirentOffset: %d, Count: %d}", t.Directory, t.DirentOffset, t.Count) } // Rreaddir is a readdir response. @@ -1985,9 +1986,10 @@ func (r *Rreaddir) decode(b *buffer) { func (r *Rreaddir) encode(b *buffer) { entriesBuf := buffer{} payloadSize := 0 - for _, d := range r.Entries { + for i, d := range r.Entries { d.encode(&entriesBuf) if len(entriesBuf.data) > int(r.Count) { + log.Warningf("hit Rreaddir.Count limit while encoding dirents, discarding %d dirents", len(r.Entries)-i) break } payloadSize = len(entriesBuf.data) diff --git a/pkg/p9/messages_test.go b/pkg/p9/messages_test.go index 8da664bf2..858a98c5e 100644 --- a/pkg/p9/messages_test.go +++ b/pkg/p9/messages_test.go @@ -221,9 +221,9 @@ func TestEncodeDecode(t *testing.T) { }, &Rsetxattr{}, &Treaddir{ - Directory: 1, - Offset: 2, - Count: 3, + Directory: 1, + DirentOffset: 2, + Count: 3, }, &Rreaddir{ // Count must be sufficient to encode a dirent. diff --git a/pkg/p9/p9.go b/pkg/p9/p9.go index 34fbf9beb..5db73179a 100644 --- a/pkg/p9/p9.go +++ b/pkg/p9/p9.go @@ -1084,6 +1084,10 @@ func (a *Attr) Apply(mask SetAttrMask, attr SetAttr) { } } +// DirentSizeStatic is the number of bytes required to encode a p9.Dirent +// with an empty name. In other words, it is the static part of its size. +const DirentSizeStatic = 24 + // Dirent is used for readdir. type Dirent struct { // QID is the entry QID. diff --git a/runsc/fsgofer/fsgofer.go b/runsc/fsgofer/fsgofer.go index 6a40b9bd6..bf8c73d05 100644 --- a/runsc/fsgofer/fsgofer.go +++ b/runsc/fsgofer/fsgofer.go @@ -989,7 +989,7 @@ func (l *localFile) UnlinkAt(name string, flags uint32) error { } // Readdir implements p9.File. -func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) { +func (l *localFile) Readdir(direntOffset uint64, count uint32) ([]p9.Dirent, error) { if l.mode != p9.ReadOnly && l.mode != p9.ReadWrite { return nil, unix.EBADF } @@ -1010,18 +1010,18 @@ func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) { // offset is 0, since this is side-effectual (equivalent to rewinddir(3), // which causes the directory stream to resynchronize with the directory's // current contents). - if l.lastDirentOffset != offset || offset == 0 { + if l.lastDirentOffset != direntOffset || direntOffset == 0 { if _, err := unix.Seek(l.file.FD(), 0, 0); err != nil { return nil, extractErrno(err) } - skip = offset + skip = direntOffset } - dirents, err := l.readDirent(l.file.FD(), offset, count, skip) + dirents, err := l.readDirent(l.file.FD(), direntOffset, count, skip) if err == nil { // On success, remember the offset that was returned at the current // position. - l.lastDirentOffset = offset + uint64(len(dirents)) + l.lastDirentOffset = direntOffset + uint64(len(dirents)) } else { // On failure, the state is unknown, force call to seek() next time. l.lastDirentOffset = math.MaxUint64 @@ -1029,57 +1029,62 @@ func (l *localFile) Readdir(offset uint64, count uint32) ([]p9.Dirent, error) { return dirents, err } -func (l *localFile) readDirent(f int, offset uint64, count uint32, skip uint64) ([]p9.Dirent, error) { +func (l *localFile) readDirent(f int, direntOffset uint64, count uint32, skip uint64) ([]p9.Dirent, error) { var dirents []p9.Dirent - // Limit 'count' to cap the slice size that is returned. - const maxCount = 100000 + // p9.Dirent takes 5 extra bytes to be encoded than a unix.Dirent. + // count will be used to count against the number of bytes read from the + // host. So scale it down, so that when encoding the same dirents in p9, we + // don't hit count limit. Scale down to 80%. + count = (count * 8) / 10 + // Limit 'count' to cap the amount of data that is returned. + const maxCount = 102_400 if count > maxCount { count = maxCount } // Pre-allocate buffers that will be reused to get partial results. direntsBuf := make([]byte, 8192) - names := make([]string, 0, 100) - - end := offset + uint64(count) - for offset < end { - dirSize, err := unix.ReadDirent(f, direntsBuf) + for bytesRead := 0; bytesRead < int(count); { + bufEnd := len(direntsBuf) + if remaining := int(count) - bytesRead; remaining < bufEnd { + bufEnd = remaining + } + n, err := unix.Getdents(f, direntsBuf[:bufEnd]) if err != nil { + if err == unix.EINVAL && bufEnd < unixDirentMaxSize { + // getdents64(2) returns EINVAL when the result buffer is too small. If + // bufEnd is smaller than the max size of unix.Dirent, then just break + // here to return all dirents collected till now. + return dirents, nil + } return dirents, err } - if dirSize <= 0 { + if n <= 0 { return dirents, nil } - names := names[:0] - _, _, names = unix.ParseDirent(direntsBuf[:dirSize], -1, names) - - // Skip over entries that the caller is not interested in. - if skip > 0 { - if skip > uint64(len(names)) { - skip -= uint64(len(names)) - names = names[:0] - } else { - names = names[skip:] - skip = 0 - } - } - for _, name := range names { + parseDirents(direntsBuf[:n], func(ino uint64, off int64, ftype uint8, name string, reclen uint16) bool { stat, err := statAt(l.file.FD(), name) if err != nil { log.Warningf("Readdir is skipping file %q with failed stat, err: %v", path.Join(l.hostPath, name), err) - continue + return true + } + if skip > 0 { + skip-- + return true } qid := l.attachPoint.makeQID(&stat) - offset++ + direntOffset++ dirents = append(dirents, p9.Dirent{ QID: qid, Type: qid.Type, Name: name, - Offset: offset, + Offset: direntOffset, }) - } + bytesRead += int(reclen) + return true + }) } return dirents, nil } diff --git a/runsc/fsgofer/fsgofer_test.go b/runsc/fsgofer/fsgofer_test.go index 69c5ee0df..9889807f0 100644 --- a/runsc/fsgofer/fsgofer_test.go +++ b/runsc/fsgofer/fsgofer_test.go @@ -738,7 +738,7 @@ func TestReaddir(t *testing.T) { t.Fatalf("%v: Open(ReadOnly) failed, err: %v", s, err) } - dirents, err := s.file.Readdir(0, 10) + dirents, err := s.file.Readdir(0, 4096) if err != nil { t.Fatalf("%v: Readdir(0, 10) failed, err: %v", s, err) } diff --git a/runsc/fsgofer/fsgofer_unsafe.go b/runsc/fsgofer/fsgofer_unsafe.go index fb4fbe0d2..5af99b5b3 100644 --- a/runsc/fsgofer/fsgofer_unsafe.go +++ b/runsc/fsgofer/fsgofer_unsafe.go @@ -21,7 +21,7 @@ import ( "gvisor.dev/gvisor/pkg/syserr" ) -var unixDirentMaxSize uint32 = uint32(unsafe.Sizeof(unix.Dirent{})) +var unixDirentMaxSize = int(unsafe.Sizeof(unix.Dirent{})) func utimensat(dirFd int, name string, times [2]unix.Timespec, flags int) error { // utimensat(2) doesn't accept empty name, instead name must be nil to make it @@ -83,11 +83,14 @@ func renameat(oldDirFD int, oldName string, newDirFD int, newName string) error return nil } -func parseDirents(buf []byte, handleDirent func(ino uint64, off int64, ftype uint8, name string) bool) { +func parseDirents(buf []byte, handleDirent func(ino uint64, off int64, ftype uint8, name string, reclen uint16) bool) { for len(buf) > 0 { // Interpret the buf populated by unix.Getdents as unix.Dirent. dirent := *(*unix.Dirent)(unsafe.Pointer(&buf[0])) + // Advance buf for the next dirent. + buf = buf[dirent.Reclen:] + // Extracting the name is pretty tedious... var nameBuf [unix.NAME_MAX]byte var nameLen int @@ -101,12 +104,15 @@ func parseDirents(buf []byte, handleDirent func(ino uint64, off int64, ftype uin } name := string(nameBuf[:nameLen]) - // Deliver results to caller. - if !handleDirent(dirent.Ino, dirent.Off, dirent.Type, name) { - return + // Skip `.` and `..` entries. It is anyways ignored by the client. We also + // don't want to leak information about `..`. + if name == "." || name == ".." { + continue } - // Advance buf for the next dirent. - buf = buf[dirent.Reclen:] + // Deliver results to caller. + if !handleDirent(dirent.Ino, dirent.Off, dirent.Type, name, dirent.Reclen) { + return + } } } diff --git a/runsc/fsgofer/lisafs.go b/runsc/fsgofer/lisafs.go index fc96ddbdc..5767dba2c 100644 --- a/runsc/fsgofer/lisafs.go +++ b/runsc/fsgofer/lisafs.go @@ -842,7 +842,7 @@ func (fd *openFDLisa) Getdent64(count uint32, seek0 bool, recordDirent func(lisa } n, err := unix.Getdents(fd.hostFD, direntsBuf[:bufEnd]) if err != nil { - if err == unix.EINVAL && bufEnd < 268 { + if err == unix.EINVAL && bufEnd < unixDirentMaxSize { // getdents64(2) returns EINVAL is returned when the result // buffer is too small. If bufEnd is smaller than the max // size of unix.Dirent, then just break here to return all @@ -854,9 +854,8 @@ func (fd *openFDLisa) Getdent64(count uint32, seek0 bool, recordDirent func(lisa if n <= 0 { break } - bytesRead += n - parseDirents(direntsBuf[:n], func(ino uint64, off int64, ftype uint8, name string) bool { + parseDirents(direntsBuf[:n], func(ino uint64, off int64, ftype uint8, name string, reclen uint16) bool { dirent := lisafs.Dirent64{ Ino: primitive.Uint64(ino), Off: primitive.Uint64(off), @@ -874,6 +873,7 @@ func (fd *openFDLisa) Getdent64(count uint32, seek0 bool, recordDirent func(lisa dirent.DevMinor = primitive.Uint32(unix.Minor(stat.Dev)) dirent.DevMajor = primitive.Uint32(unix.Major(stat.Dev)) recordDirent(dirent) + bytesRead += int(reclen) return true }) }